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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Owen Smith <owen.smith@xxxxxxxxxx>
> Sent: 20 July 2020 11:18
> To: paul@xxxxxxx; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] Clear unplug keys if Active device is not the Vendor 
> device
> 
> 
> > -----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().
> 

Is that a problem in itself? If the vendor device is present but the legacy 
device is already active, then things should be ok,
shouldn't they? Ah, maybe I see the issue...

A new XENBUS comes from Windows Update but, for some reason, the vendor device 
was not present when the PV drivers were previously
installed. So the PDO tree hangs off the legacy device and hence no driver 
apart from XENBUS ever gets Windows updates, and then we
run into a lack of compatibility issue?

> 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
> 

It seems that, when XENBUS binds to the vendor device, we need to clear the 
active device as well as the unplug keys such that, on
the next reboot, the PDOs move over onto the vendor device. Would that work?

As for the vendor device disappearing, I guess the condition in 
DriverSetActive() will deal with making the legacy device active and
I *think* DriverSetFilterState() will DTRT WRT to unplugging (or not).

  Paul

> >   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
> > >
> >
> >