Re: [PATCH] Add XENFILT_EMULATED_OBJECT_TYPE override

[Owen Smith <owen.smith@xxxxxxxxxx>]

________________________________________
From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> on behalf of 
Paul Durrant <xadimgnik@xxxxxxxxx>
Sent: 26 March 2021 08:10
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] Add XENFILT_EMULATED_OBJECT_TYPE override

[CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
unless you have verified the sender and know the content is safe.

On 15/03/2021 11:25, Owen Smith wrote:
> EMULATED(IsDiskPresent) currently only reports if IDE disks are present.
> Add the option to override the emulated type of any given PDO to allow
> the emulated NVMe device to report its emulated device type as NVME.
> Adds the override for the QEMU emulated NVMe controller
> (PCI\VEN_8086&DEV_5845) to indicate it is a NVMe controller, and
> exposes disk 0.
> Also extends XENFILT_EMULATED_OBJECT_TYPE to include NVME
>
> This is to fix an issue where XenVbd can incorrectly identify that disk
> 0 is not present, when disk 0 is infact the present emulated NVMe
> device. This is problematic if the VM boots off the emulated device, not
> the PV device (due to the missing unplug) and then rebinds XENVBD which
> is unable to determine the presense of the emulated disk and attempts to
> connect to an in-use ring.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenbus.inf         |  1 +
>   src/xenfilt/driver.c   | 24 +++++++++++++++---
>   src/xenfilt/driver.h   |  7 ++++++
>   src/xenfilt/emulated.c | 40 ++++++++++++++++++++++++++++++
>   src/xenfilt/emulated.h |  3 ++-
>   src/xenfilt/pdo.c      | 55 ++++++++++++++++++++++++++++++++++++++++++
>   6 files changed, 125 insertions(+), 5 deletions(-)
>
> diff --git a/src/xenbus.inf b/src/xenbus.inf
> index f9a5475..d2f2477 100644
> --- a/src/xenbus.inf
> +++ b/src/xenbus.inf
> @@ -119,6 +119,7 @@ AddReg = XenFilt_Parameters
>   HKR,"Parameters",,0x00000010
>   HKR,"Parameters","*PNP0A03",0x00000000,"PCI"
>   HKR,"Parameters","Internal_IDE_Channel",0x00000000,"IDE"
> +HKR,"Parameters\Override","PCI\VEN_8086&DEV_5845&SUBSYS_11001AF4&REV_02",0x00000000,"NVME"

That's a very strict match. How confident are we that the QEMU device
won't need to deal with e.g. multiple revisions of the QEMU device?
Perhaps it would be better to look at class code or is there something
else that may be a more stable identifier?

   Paul


My intention here was to make the match as specific as possible given the 
information immidiately available at the time the override is applied. By 
keeping it specific, its reducing the chances of wrongly overriding a device.

Using device class code may catch devices that are passed through, or are not 
otherwise the emulated device that corresponds to the PV device.

Owen

>
>   [Monitor_Service]
>   DisplayName=%MonitorName%
> diff --git a/src/xenfilt/driver.c b/src/xenfilt/driver.c
> index e9e6673..eb2ca42 100644
> --- a/src/xenfilt/driver.c
> +++ b/src/xenfilt/driver.c
> @@ -723,6 +723,25 @@ fail1:
>       return status;
>   }
>
> +XENFILT_EMULATED_OBJECT_TYPE
> +DriverParseEmulatedType(
> +    IN  PANSI_STRING                Ansi
> +    )
> +{
> +    XENFILT_EMULATED_OBJECT_TYPE    Type;
> +
> +    Type = XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN;
> +
> +    if (_strnicmp(Ansi->Buffer, "PCI", Ansi->Length) == 0)
> +        Type = XENFILT_EMULATED_OBJECT_TYPE_PCI;
> +    else if (_strnicmp(Ansi->Buffer, "IDE", Ansi->Length) == 0)
> +        Type = XENFILT_EMULATED_OBJECT_TYPE_IDE;
> +    else if (_strnicmp(Ansi->Buffer, "NVME", Ansi->Length) == 0)
> +        Type = XENFILT_EMULATED_OBJECT_TYPE_NVME;
> +
> +    return Type;
> +}
> +
>   static XENFILT_EMULATED_OBJECT_TYPE
>   DriverGetEmulatedType(
>       IN  PCHAR                       Id
> @@ -753,10 +772,7 @@ DriverGetEmulatedType(
>           if (NT_SUCCESS(status)) {
>               Info("MATCH: %s -> %Z\n", &Id[Index], Ansi);
>
> -            if (_strnicmp(Ansi->Buffer, "PCI", Ansi->Length) == 0)
> -                Type = XENFILT_EMULATED_OBJECT_TYPE_PCI;
> -            else if (_strnicmp(Ansi->Buffer, "IDE", Ansi->Length) == 0)
> -                Type = XENFILT_EMULATED_OBJECT_TYPE_IDE;
> +            Type = DriverParseEmulatedType(Ansi);
>
>               RegistryFreeSzValue(Ansi);
>           } else {
> diff --git a/src/xenfilt/driver.h b/src/xenfilt/driver.h
> index 286580b..2129054 100644
> --- a/src/xenfilt/driver.h
> +++ b/src/xenfilt/driver.h
> @@ -32,6 +32,8 @@
>   #ifndef _XENFILT_DRIVER_H
>   #define _XENFILT_DRIVER_H
>
> +#include "emulated.h"
> +
>   extern PDRIVER_OBJECT
>   DriverGetDriverObject(
>       VOID
> @@ -58,6 +60,11 @@ DriverGetActive(
>       OUT PCHAR       *Value
>       );
>
> +extern XENFILT_EMULATED_OBJECT_TYPE
> +DriverParseEmulatedType(
> +    IN  PANSI_STRING                Ansi
> +    );
> +
>   typedef enum _XENFILT_FILTER_STATE {
>       XENFILT_FILTER_ENABLED = 0,
>       XENFILT_FILTER_PENDING,
> diff --git a/src/xenfilt/emulated.c b/src/xenfilt/emulated.c
> index 827c905..97da61b 100644
> --- a/src/xenfilt/emulated.c
> +++ b/src/xenfilt/emulated.c
> @@ -87,6 +87,33 @@ __EmulatedFree(
>       __FreePoolWithTag(Buffer, XENFILT_EMULATED_TAG);
>   }
>
> +static NTSTATUS
> +EmulatedSetObjectNvmeData(
> +    IN  PXENFILT_EMULATED_OBJECT        EmulatedObject,
> +    IN  XENFILT_EMULATED_OBJECT_TYPE    Type,
> +    IN  PCHAR                           DeviceID,
> +    IN  PCHAR                           InstanceID
> +    )
> +{
> +    NTSTATUS                            status;
> +
> +    UNREFERENCED_PARAMETER(DeviceID);
> +    UNREFERENCED_PARAMETER(InstanceID);
> +
> +    status = STATUS_INVALID_PARAMETER;
> +    if (Type != XENFILT_EMULATED_OBJECT_TYPE_NVME)
> +        goto fail1;
> +
> +    EmulatedObject->Data.Disk.Index = 0;
> +
> +    return STATUS_SUCCESS;
> +
> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return status;
> +}
> +
>   static NTSTATUS
>   EmulatedSetObjectDeviceData(
>       IN  PXENFILT_EMULATED_OBJECT        EmulatedObject,
> @@ -210,6 +237,13 @@ EmulatedAddObject(
>           goto fail1;
>
>       switch (Type) {
> +    case XENFILT_EMULATED_OBJECT_TYPE_NVME:
> +        status = EmulatedSetObjectNvmeData(*EmulatedObject,
> +                                           Type,
> +                                           DeviceID,
> +                                           InstanceID);
> +        break;
> +
>       case XENFILT_EMULATED_OBJECT_TYPE_PCI:
>           status = EmulatedSetObjectDeviceData(*EmulatedObject,
>                                                Type,
> @@ -338,6 +372,12 @@ EmulatedIsDiskPresent(
>               Trace("FOUND\n");
>               break;
>           }
> +        if (EmulatedObject->Type == XENFILT_EMULATED_OBJECT_TYPE_NVME &&
> +            Index == EmulatedObject->Data.Disk.Index) {
> +            Trace("FOUND\n");
> +            break;
> +        }
> +
>
>           ListEntry = ListEntry->Flink;
>       }
> diff --git a/src/xenfilt/emulated.h b/src/xenfilt/emulated.h
> index 57edee2..499e43c 100644
> --- a/src/xenfilt/emulated.h
> +++ b/src/xenfilt/emulated.h
> @@ -41,7 +41,8 @@ typedef struct _XENFILT_EMULATED_CONTEXT 
> XENFILT_EMULATED_CONTEXT, *PXENFILT_EMU
>   typedef enum _XENFILT_EMULATED_OBJECT_TYPE {
>       XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN = 0,
>       XENFILT_EMULATED_OBJECT_TYPE_PCI,
> -    XENFILT_EMULATED_OBJECT_TYPE_IDE
> +    XENFILT_EMULATED_OBJECT_TYPE_IDE,
> +    XENFILT_EMULATED_OBJECT_TYPE_NVME
>   } XENFILT_EMULATED_OBJECT_TYPE, *PXENFILT_EMULATED_OBJECT_TYPE;
>
>   typedef struct _XENFILT_EMULATED_OBJECT XENFILT_EMULATED_OBJECT, 
> *PXENFILT_EMULATED_OBJECT;
> diff --git a/src/xenfilt/pdo.c b/src/xenfilt/pdo.c
> index 0f6e6ce..41b0b10 100644
> --- a/src/xenfilt/pdo.c
> +++ b/src/xenfilt/pdo.c
> @@ -42,6 +42,7 @@
>   #include "pdo.h"
>   #include "thread.h"
>   #include "driver.h"
> +#include "registry.h"
>   #include "dbg_print.h"
>   #include "assert.h"
>   #include "util.h"
> @@ -251,6 +252,58 @@ __PdoGetFdo(
>       return Pdo->Fdo;
>   }
>
> +static VOID
> +__PdoGetEmulatedTypeOverride(
> +    IN  PXENFILT_PDO                Pdo,
> +    IN  PCHAR                       DeviceID
> +    )
> +{
> +    HANDLE                          ParametersKey;
> +    HANDLE                          OverrideKey;
> +    ULONG                           ValueType;
> +    PANSI_STRING                    Ansi;
> +    XENFILT_EMULATED_OBJECT_TYPE    Type;
> +    NTSTATUS                        status;
> +
> +    ParametersKey = DriverGetParametersKey();
> +
> +    status = RegistryOpenSubKey(ParametersKey,
> +                                "Override",
> +                                GENERIC_READ,
> +                                &OverrideKey);
> +    if (!NT_SUCCESS(status))
> +        goto fail1;
> +
> +    status = RegistryQuerySzValue(OverrideKey,
> +                                  DeviceID,
> +                                  &ValueType,
> +                                  &Ansi);
> +    if (!NT_SUCCESS(status))
> +        goto fail2;
> +
> +    if (ValueType != REG_SZ)
> +        goto fail3;
> +
> +    Type = DriverParseEmulatedType(Ansi);
> +    if (Type != XENFILT_EMULATED_OBJECT_TYPE_UNKNOWN)
> +        Pdo->Type = Type;
> +
> +    RegistryFreeSzValue(Ansi);
> +
> +    RegistryCloseKey(OverrideKey);
> +
> +    return;
> +
> +fail3:
> +    RegistryFreeSzValue(Ansi);
> +
> +fail2:
> +    RegistryCloseKey(OverrideKey);
> +
> +fail1:
> +    return;
> +}
> +
>   static NTSTATUS
>   PdoSetDeviceInformation(
>       IN  PXENFILT_PDO    Pdo
> @@ -310,6 +363,8 @@ PdoSetDeviceInformation(
>               LocationInformation = NULL;
>       }
>
> +    __PdoGetEmulatedTypeOverride(Pdo, DeviceID);
> +
>       Dx->DeviceID = DeviceID;
>       Dx->InstanceID = InstanceID;
>       Dx->LocationInformation = LocationInformation;
>