RE: [PATCH] Clear unplug keys if Active device is not the Vendor device

[Owen Smith <owen.smith@xxxxxxxxxx>]

> -----Original Message-----
> From: Paul Durrant [mailto:xadimgnik@xxxxxxxxx]
> Sent: 20 July 2020 10:18
> To: Owen Smith <owen.smith@xxxxxxxxxx>; win-pv-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] Clear unplug keys if Active device is not the Vendor
> device
> 
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments
> unless you have verified the sender and know the content is safe.
> 
> > -----Original Message-----
> > From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On
> > Behalf Of Owen Smith
> > Sent: 16 July 2020 09:38
> > To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> > Subject: [PATCH] Clear unplug keys if Active device is not the Vendor
> > device
> >
> > If the Vendor device is distributed by Windows Update, the update
> > process will queue a reboot.
> 
> Well, the device is not distributed by Windows Update. Do you mean "If a
> version of XENBUS supporting a vendor device"? (Bear in mind there could
> theoretically be vendor devices other than the Citrix one).

The message is a little unclear here. If a version of XenBus is distributed by 
Windows Update for a given vendor device ID

> 
> > If the vendor device is not the active device, then the unplug keys
> > will not be removed before the next reboot.
> > The reboot will also replace the driver on the active device which
> > requires rebinding child devices, which cannot happen during the early
> > boot phase. This will lead to a 7B bugcheck because XenVbd has not
> > been rebound to the new version of XenBus
> 
> What is *supposed* to happen is that the, when a vendor device appears in
> the system necessitating a new XENBUS binding, the co-installer clears the
> unplug keys because the PDOs beneath the non-vendor device will
> disappear at next boot because XENBUS's active device will change. This
> could equally happen if a vendor device subsequently disappears, which I
> don't think this patch covers.
> 

If the XenBus bound to the vendor device is updated, this will replace the 
driver on any other device, which will necessitate clearing of the unplug keys. 
This doesn't happen at the moment, so on the reboot to complete installation of 
XenBus on the non-active vendor device ID, the unplug keys are not removed and 
the bindings of the child devices on the XenBus on the active non-vendor device 
ID are not regenerated. This hits DriverSetActive's fail2 case, where 
__DriverIsDeviceLegacy(non-vendor-device) and __DriverIsVendorDevicePresent().

This patch doesn't cover the case where the vendor device disappears, primarily 
because I didn't hit this edge case trying to resolve the original issue

>   Paul
> 
> >
> > Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> > ---
> >  src/coinst/coinst.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/coinst/coinst.c b/src/coinst/coinst.c index
> > 7b96f59..8e57a10 100644
> > --- a/src/coinst/coinst.c
> > +++ b/src/coinst/coinst.c
> > @@ -1361,7 +1361,8 @@ static BOOLEAN
> >  IsActiveDevice(
> >      IN  HDEVINFO            DeviceInfoSet,
> >      IN  PSP_DEVINFO_DATA    DeviceInfoData,
> > -    OUT PBOOLEAN            ActiveDevice
> > +    OUT PBOOLEAN            ActiveDevice,
> > +    OUT PBOOLEAN            VendorIsActive
> >      )
> >  {
> >      PTCHAR                  ActiveDeviceID;
> > @@ -1392,6 +1393,20 @@ IsActiveDevice(
> >          TRUE :
> >          FALSE;
> >
> > +#ifdef VENDOR_DEVICE_ID_STR
> > +
> > +#define DRIVER_VENDOR_DEVICE_ID "PCI\\VEN_5853&DEV_" ##
> > +VENDOR_DEVICE_ID_STR ##
> > "&SUBSYS_C0005853&REV_01"
> > +
> > +    *VendorIsActive = (_stricmp(ActiveDeviceID,
> DRIVER_VENDOR_DEVICE_ID) == 0) ?
> > +        TRUE :
> > +        FALSE;
> > +
> > +#undef DRIVER_VENDOR_DEVICE_ID
> > +
> > +#else
> > +    *VendorIsActive = FALSE;
> > +#endif
> > +
> >      free(DeviceID);
> >      free(InstanceID);
> >
> > @@ -1778,21 +1793,25 @@ DifInstallPostProcess(  {
> >      BOOLEAN                         NewBinding;
> >      BOOLEAN                         Active;
> > +    BOOLEAN                         VendorIsActive;
> >
> >      Log("====>");
> >
> >      NewBinding = (BOOLEAN)(ULONG_PTR)Context->PrivateData;
> >
> >      Active = TRUE;
> > +    VendorIsActive = FALSE;
> >
> >      (VOID) IsActiveDevice(DeviceInfoSet,
> >                            DeviceInfoData,
> > -                          &Active);
> > +                          &Active,
> > +                          &VendorIsActive);
> >
> >      Log("Active = %s", Active ? "TRUE" : "FALSE");
> >      Log("NewBinding = %s", NewBinding ? "TRUE" : "FALSE");
> >
> > -    if (Active && NewBinding) {
> > +    if ((Active && NewBinding) ||
> > +        !VendorIsActive) {
> >          (VOID) ClearUnplugRequest("DISKS");
> >          (VOID) ClearUnplugRequest("NICS");
> >      }
> > --
> > 2.16.2.windows.1
> >
> 
>