[vioscsi] New ProcessQueue() routine

[benyamin-codez]

Eliminates VioScsiMSInterruptWorker() by refactoring contents into new ProcessQueue() routine.
This consolidates the child processes of:

• HW_INTERRUPT
• HW_MESSAGE_SIGNALLED_INTERRUPT_ROUTINE

Implemented using new tracing functionality.
This includes use of the new ENTER_INL_FN and EXIT_INL_FN functions when FORCEINLINE is used.

Implemented new VIRTIO_SCSI_MSI_CONTROL_Q_OFFSET definition
Updated QUEUE_TO_MESSAGE() and MESSAGE_TO_QUEUE() to use it.

DispatchQueue() now returns a BOOLEAN and uses new tracing functionality, as above.

[JonKohler]

As always, I really like your approach to these changes, the whole thing seems good to me, but for the sake of being pedantic, I did add one nit.

LGTM otherwise

[JonKohler]

personal nit: can we wrap such long lines, like:

return blah(long_name_arg1,
            long_name_arg2,
            long_name_arg3)

[YanVugenfirer]

@JonKohler @kostyanf14
Clang formater is coming soon (already added for Balloon and FWCfg drivers).

The long list of parameters will be wrapped

[benyamin-codez]

@YanVugenfirer @JonKohler @kostyanf14

Clang formatter is coming soon ...

Will it also remove the indent on the logger macros, e.g. #if !defined(RUN_UNCHECKED)...?
It would be good if it didn't.
Same for line / ASCII art as mentioned elsewhere...

It would seem I'm used to the wider screen...
...but can understand it helps to shrink the cols when using GitHub to review. ;^D

[kostyanf14]

@benyamin-codez

Will it also remove the indent on the logger macros, e.g. #if !defined(RUN_UNCHECKED)...?

Yes, and this is as MS does in driver examples.

Same for line / ASCII art as mentioned elsewhere...

clang-format will be disabled for these specific lines.

[benyamin-codez]

@kostyanf14 @JonKohler

Will it also remove the indent on the logger macros, e.g. #if !defined(RUN_UNCHECKED)...?

Yes, and this is as MS does in driver examples.

Technically correct, but so aesthetically displeasing... 8^(

[benyamin-codez]

Placeholder to kick off discussion re tracing issues...

Hot path considerations.
Presently RUN_UNCHECKED and RUN_MIN_CHECKED are only implemented for DEBUG due to ETW / WPP limitations.
Probably need a new PR to introduce WPP taxonomies for WPP configs in trace.h.
This PR should also create any necessary hot and cold path variants.
Consideration to implementing warm path variants should also be given.
My thoughts are that RUN_UNCHECKED is useful for testing base logging overhead or in extreme performance cases.
...and RUN_MIN_CHECKED should essentially be cold path only.
Having said all that, it may still not work. IIRC, I had issues attempting this last round.
I'm minded to make this my next piece of work so as to progress this and other PRs in the pipeline...

EDIT: I have raised PR #1228 to refactor tracing to use cold and hot paths. Please continue the discussion there.

[YanVugenfirer]

It is better to control debug level through the registry, similar to NetKVM.

We haven't had a debug compilation option in the VS solution for a long time, so the printout will be only for very motivated developers. Also, there is a "regression" in behavior. Driver load printout was unconditional; now, it is not.

[benyamin-codez]

@YanVugenfirer

It is better to control debug level through the registry, similar to NetKVM.

Very interesting... I'll have another look with this example in mind.

I had abandoned this because:

1. One cannot usually get the registry value early enough; and
2. For WHCP reasons, StorPort requires you to only use one of the two inbuilt registry read functions.

These two functions are StorPortRegistryRead and StorPortRegistryReadAdapterKey.

IIRC, neither can be called until FindAdapter(), but I'll have another look anyway.

[benyamin-codez]

@YanVugenfirer

Also, there is a "regression" in behavior. Driver load printout was unconditional; now, it is not.

Just thought I'd best point out that these are compile-time conditional macros. I'm not proposing that it should be unprinted in "normal" operations. Using RUN_UNCHECKED, where it would not print, would be a a very unusual fringe case.

[benyamin-codez]

@YanVugenfirer

Just wanted to confirm my understanding was as above, i.e.:

1. One must load the driver to open the registry to enable debug, which leaves a window where there may not be any debugging occurring and this might be a concern for a driver loading at boot-time;
2. The necessary registry manipulation routines, e.g. ZwQueryValueKey(), are indeed part of the StorPort banned HwStorPortProhibitedDDIs rule-set.

Notwithstanding (1) above, to overcome the issue in (2) above, we could use a similar solution to NetKVM and enable the necessary key via the vioscsi.inf file and then use StorPortRegistryReadAdapterKey() to read the relevant value in the ENUM key path and switch to enable debug. This would require loading at VioScsiFindAdapter() and potentially require a driver restart. That seemed a bit too messy for my liking...

That being said, we presently don't compile debug support, only ETW via the WPP compile-time mechanism. The driver currently supports a choice between the two at compile time only, and only via direct manipulation of the trace.h file.

In any case, please note I have raised PR #1228 to refactor tracing to use cold and hot paths.

This included changing RUN_MIN_CHECKED to RUN_COLD_PATH_ONLY.
This results in the if defined(RUN_MIN_CHECKED) conditional for the cold path being removed...
...and the hot path using if !defined(RUN_COLD_PATH_ONLY).
I will update this and other effected PRs accordingly.

It also introduces tracing and debug controls via compile-time environment variables.

Please check it out.

Best regards,
Ben