search

virtio-win / kvm-guest-drivers-windows

Windows paravirtualized drivers for QEMU\KVM
https://www.linux-kvm.org/page/WindowsGuestDrivers
BSD 3-Clause "New" or "Revised" License
2.01k stars 385 forks source link

[netkvm] does netkvm can support NDIS_MINIPORT_ATTRIBUTES_SURPRISE_REMOVE_OK? #1002

Open zjmletang opened 10 months ago

zjmletang commented 10 months ago

Is your feature request related to a problem? Please describe.

For the current version, Windows displays a pop-up for devices, and sometimes users accidentally click on eject, making the device invisible. I'm considering whether it's possible to prevent network card devices from appearing in the eject device list, so users won't accidentally click on them.

on the other hand, as the msdn(https://learn.microsoft.com/en-us/windows-hardware/drivers/network/handling-the-surprise-removal-of-a-nic) says: Miniport drivers for Windows Vista and later versions of the operating system should be able to handle surprise removals. In particular, NDIS miniport drivers with a Windows Driver Model (WDM) lower edge should be able to handle such events. If an NDIS-WDM miniport driver does not handle a surprise removal, any pending IRPs that the miniport driver sent to the underlying bus driver before the surprise removal cannot be completed.

image

Describe the solution you'd like

solution: netkvm support NDIS_MINIPORT_ATTRIBUTES_SURPRISE_REMOVE_OK

One area that needs to be modified is as follows，But I haven't studied yet whether there are other places that need to be modified.

miniportAttributes.RegistrationAttributes.AttributeFlags = // actual for USB NDIS_MINIPORT_ATTRIBUTES_SURPRISE_REMOVE_OK| NDIS_MINIPORT_ATTRIBUTES_HARDWARE_DEVICE | NDIS_MINIPORT_ATTRIBUTES_BUS_MASTER;

ifndef NO_VISTA_POWER_MANAGEMENT

    miniportAttributes.RegistrationAttributes.AttributeFlags |= NDIS_MINIPORT_ATTRIBUTES_NO_HALT_ON_SUSPEND;

endif

if NDIS_SUPPORT_NDIS630

    miniportAttributes.RegistrationAttributes.AttributeFlags |= NDIS_MINIPORT_ATTRIBUTES_NO_PAUSE_ON_SUSPEND;

endif

YanVugenfirer commented 10 months ago

@vrozenfe I think we handle it on the host side for the storage devices, right?

vrozenfe commented 10 months ago

Hi Yan, I think you are referencing QEMU PCI root port feature hotplug=on/off https://bugzilla.redhat.com/show_bug.cgi?id=1790899 ?

Cheers, Vadim.

YanVugenfirer commented 10 months ago

Hi Yan, I think you are referencing QEMU PCI root port feature hotplug=on/off https://bugzilla.redhat.com/show_bug.cgi?id=1790899 ?

Cheers, Vadim.

@zjmletang please check Vadim's comment.

ybendito commented 9 months ago

For further discussion: IMO, it is good to add it if it does not break anything including HCK and hot plug/unplug functionality (IMO it should not). For netkvm adding this attribute effectively removes it from the "eject" list. IMO again we do not need to add any code except of this attribute declaration as we anyway support the surprise removal.

zjmletang commented 9 months ago

Hi Yan, I think you are referencing QEMU PCI root port feature hotplug=on/off https://bugzilla.redhat.com/show_bug.cgi?id=1790899 ? Cheers, Vadim.

@zjmletang please check Vadim's comment.

@YanVugenfirer ok， thanks.

zjmletang commented 9 months ago

For further discussion: IMO, it is good to add it if it does not break anything including HCK and hot plug/unplug functionality (IMO it should not). For netkvm adding this attribute effectively removes it from the "eject" list. IMO again we do not need to add any code except of this attribute declaration as we anyway support the surprise removal.

@ybendito ,If that's the case, may I submit a PR (just to add this attribute) directly?

ybendito commented 9 months ago

@YanVugenfirer @vrozenfe I see for storage drivers it was discussed earlier https://github.com/virtio-win/kvm-guest-drivers-windows/issues/366

YanVugenfirer commented 9 months ago

For further discussion: IMO, it is good to add it if it does not break anything including HCK and hot plug/unplug functionality (IMO it should not). For netkvm adding this attribute effectively removes it from the "eject" list. IMO again we do not need to add any code except of this attribute declaration as we anyway support the surprise removal.

I think it was breaking HCK. So before doing it, please run full HCK test and check that it is OK

ybendito commented 9 months ago

@zjmletang Yes, you welcome to submit such PR. First of all we'll see that the HCK is ok with it. Later we'll need to check that this does not affect hot unplugging flow.

zjmletang commented 8 months ago

For further discussion: IMO, it is good to add it if it does not break anything including HCK and hot plug/unplug functionality (IMO it should not). For netkvm adding this attribute effectively removes it from the "eject" list. IMO again we do not need to add any code except of this attribute declaration as we anyway support the surprise removal.

@ybendito Over the past two days, while retesting, I've discovered an issue: after adding this feature, the network adapter does disappear from the "eject" list following the installation of a new driver. However, after each VM restart, the network adapter reappears in the "eject" list. At this point, if I reinstall the driver, or first disable and then enable the driver, the network adapter once again disappears from the list. I am unable to explain this point at the moment. (´；д；`)

ybendito commented 8 months ago

Probably the first appearance after VM start depends on data collected before the driver starts (on root port hotplug support). If you disable the driver in the device manager and restart the VM - the adapter appears in the list, enable the driver - disappears, disable the driver - does not appear... I agree, this OS behavior is kind of inconsistent, but we did what we're able to.