Skip to content

[CI-NO-BUILD] WIP: Viostor: Introduce action-on-reset feature from vioscsi #1305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MartinDrab
Copy link
Contributor

The action-on-reset determines what the driver should do when a bus reset request arrives. It can be configured to:

  • complete pending requests,
  • do nothing,
  • bug check.

This feature is already present in vioscsi but not in viostor.

Needs to be merged after #1297.

benyamin-codez and others added 2 commits February 17, 2025 15:55
Introduces a Registry read capability:

1. Implemented in VioStorReadRegistryParameter()
2. Requires neo helper CopyBufferToAnsiString()
3. Adds capacity to both detect and read DWORD values in:
   (a) HKLM\SYSTEM\CurrentControlSet\Services\viostor\Paramaters\Device
   (b) HKLM\SYSTEM\CurrentControlSet\Services\viostor\Paramaters\Device(d) NOT WORKING
4. Also supports per-HBA values for \Parameters\Device\Valuename_123
   when the \Parameters\Device(d) key is unavailable (presently broken)
5. Adds TRACE_REGISTRY WPP tracing flag

Signed-off-by: benyamin-codez <[email protected]>
The action-on-reset determines what the driver should do when a bus reset request arrives. It can be configured to:
- complete pending requests,
- do nothing,
- bug check.

This feature is already present in vioscsi but not in viostor.

Signed-Off-By: Martin Drab <[email protected]>
@MartinDrab MartinDrab marked this pull request as draft February 17, 2025 22:32
@benyamin-codez
Copy link
Contributor

👍

More registry love.
Nice..!

It looks like clang-format is upset.
You can find usage for the Tools/clang-format-helper.sh script here.
Probably line 490 wanting to look something like:

VioStorReadRegistryParameter(DeviceExtension,
                             REGISTRY_ACTION_ON_RESET,
                             FIELD_OFFSET(ADAPTER_EXTENSION, action_on_reset));

...but other lines look close to the column boundary too, so there might be more.

P.S.:
Just checked and it reports lines 490 & 1213 in virtio_stor.c and lines 209 and 267 in virtio_stor.h.
That mod for line 490 above is correct.

@MartinDrab
Copy link
Contributor Author

👍

More registry love. Nice..!

It looks like clang-format is upset. You can find usage for the Tools/clang-format-helper.sh script here. Probably line 490 wanting to look something like:

VioStorReadRegistryParameter(DeviceExtension,
                             REGISTRY_ACTION_ON_RESET,
                             FIELD_OFFSET(ADAPTER_EXTENSION, action_on_reset));

...but other lines look close to the column boundary too, so there might be more.

P.S.: Just checked and it reports lines 490 & 1213 in virtio_stor.c and lines 209 and 267 in virtio_stor.h. That mod for line 490 above is correct.

Yeah, I expected clang-format-check to complain and plan to fix that when #1297 gets merged (and I rebase above it).

@lixianming19951001
Copy link

I would like to ask another question: We control the behavior of action_on_reset through the registry, but by default, it still completes all pending requests. Does this not pose a risk of memory access when the host processes requests that have already been completed?

@benyamin-codez
Copy link
Contributor

I would like to ask another question: We control the behavior of action_on_reset through the registry, but by default, it still completes all pending requests. Does this not pose a risk of memory access when the host processes requests that have already been completed?

Thanks Li.

Again, Martin might know best, but viostor has the additional function CompleteRequestWithStatus() (as pointed out in my comment here - not that I'm saying here that it does need to be removed - just that it is different to vioscsi), which will complete the requests using the specified status. StorPort is eventually notified in CompleteSRB(). I would have thought that if StorPort has been notified, provided the host process makes the request via StorPort, then it should be safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants