Re: [PATCH 1/4 v2] XenDisk: Report Discard support by issuing an Inquiry

[Owen Smith <owen.smith@xxxxxxxxxx>]

It does seem a little redundant to patch xendisk, then remove xendisk in your 
series.

I'll drop my series for now, and wait until your fixes to xenvbd / removal of 
xendisk is
applied before reworking some of these patches to tidy up xenvbd

Owen

________________________________________
From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
Sent: 04 March 2026 11:53 AM
To: Owen Smith; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Owen Smith
Subject: Re: [PATCH 1/4 v2] XenDisk: Report Discard support by issuing an 
Inquiry

On 03/03/2026 10:36, Owen Smith wrote:
> From: Owen Smith <owen.smith@xxxxxxxxx>
>
> When XenDisk starts, issue an inquiry to determine the status of discard
> support in the backend. If the backend exposes discard support, allow
> XenDisk to respond to the appropriate property requests, and forward the
> trim requests to XenVbd.
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

Shouldn't XenDisk report unmaps as being unconditionally supported so
that unmap requests could be passed to XenVbd for processing (as pointed
out in "Report VPD 0xB2 Logical Block Provisioning" in the XenDisk
removal series)?

Otherwise, when booting on a backend that doesn't support discards,
XenDisk will initially report unmaps as unsupported. This will cause
Windows to not attempt unmaps even if the VM is later migrated to a
backend that does support discards.

> ---
>   src/xendisk/pdo.c | 88 ++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/src/xendisk/pdo.c b/src/xendisk/pdo.c
> index f714efb..5d40a13 100644
> --- a/src/xendisk/pdo.c
> +++ b/src/xendisk/pdo.c
> @@ -63,6 +63,7 @@ struct _XENDISK_PDO {
>       PXENDISK_FDO                Fdo;
>
>       BOOLEAN                     InterceptTrim;
> +    BOOLEAN                     DiscardSupported;
>       ULONG                       SectorSize;
>       ULONG                       PhysSectorSize;
>   };
> @@ -519,6 +520,72 @@ fail1:
>       return status;
>   }
>
> +static NTSTATUS
> +PdoSendInquiryB0Synchronous(
> +    IN  PXENDISK_PDO        Pdo,
> +    OUT PBOOLEAN            Supported
> +    )
> +{
> +    SCSI_REQUEST_BLOCK      Srb;
> +    PCDB                    Cdb;
> +    PVPD_BLOCK_LIMITS_PAGE  BlockLimits;
> +    ULONG                   Length;
> +    NTSTATUS                status;
> +
> +    Trace("====>\n");
> +
> +    Length = sizeof(VPD_BLOCK_LIMITS_PAGE);
> +    *Supported = FALSE;
> +
> +    status = STATUS_NO_MEMORY;
> +    BlockLimits = __PdoAllocate(Length);
> +    if (BlockLimits == NULL)
> +        goto fail1;
> +
> +    RtlZeroMemory(&Srb, sizeof(SCSI_REQUEST_BLOCK));
> +    Srb.Length = sizeof(SCSI_REQUEST_BLOCK);
> +    Srb.SrbFlags = 0;
> +    Srb.Function = SRB_FUNCTION_EXECUTE_SCSI;
> +    Srb.DataBuffer = BlockLimits;
> +    Srb.DataTransferLength = Length;
> +    Srb.TimeOutValue = (ULONG)-1;
> +    Srb.CdbLength = 6;
> +
> +    Cdb = (PCDB)&Srb.Cdb[0];
> +    Cdb->CDB6INQUIRY3.OperationCode = SCSIOP_INQUIRY;
> +    Cdb->CDB6INQUIRY3.PageCode = 0xB0;
> +    Cdb->CDB6INQUIRY3.EnableVitalProductData = 1;
> +    Cdb->CDB6INQUIRY3.AllocationLength = (UCHAR)Length;
> +
> +    status = PdoSendAwaitSrb(Pdo, &Srb);
> +    if (!NT_SUCCESS(status))
> +        goto fail2;
> +
> +    status = STATUS_UNSUCCESSFUL;
> +    if (Srb.DataTransferLength < Length)
> +        goto fail3;
> +
> +    *Supported = BlockLimits->UGAValid;
> +
> +    __PdoFree(BlockLimits);
> +
> +    Trace("<====\n");
> +    return STATUS_SUCCESS;
> +
> +fail3:
> +    Error("fail3\n");
> +
> +fail2:
> +    Error("fail2\n");
> +
> +    __PdoFree(BlockLimits);
> +
> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return status;
> +}
> +
>   static NTSTATUS
>   PdoSendTrimSynchronous(
>       IN  PXENDISK_PDO            Pdo,
> @@ -674,7 +741,7 @@ PdoQueryProperty(
>
>       switch (Query->PropertyId) {
>       case StorageDeviceTrimProperty:
> -        if (!Pdo->InterceptTrim) {
> +        if (!Pdo->InterceptTrim || !Pdo->DiscardSupported) {
>               status = PdoForwardIrpAndForget(Pdo, Irp);
>               break;
>           }
> @@ -753,7 +820,7 @@ PdoManageDataSetAttributes(
>
>       switch (Attributes->Action) {
>       case DeviceDsmAction_Trim:
> -        if (!Pdo->InterceptTrim) {
> +        if (!Pdo->InterceptTrim || !Pdo->DiscardSupported) {
>               status = PdoForwardIrpAndForget(Pdo, Irp);
>               break;
>           }
> @@ -830,6 +897,7 @@ PdoStartDevice(
>       ULONG               PhysSectorSize;
>       ULONG64             SectorCount;
>       ULONG64             Size;
> +    BOOLEAN             DiscardSupported;
>       POWER_STATE         PowerState;
>       NTSTATUS            status;
>
> @@ -848,14 +916,22 @@ PdoStartDevice(
>       if (!NT_SUCCESS(status))
>           goto fail3;
>
> +    status = PdoSendInquiryB0Synchronous(Pdo,
> +                                         &DiscardSupported);
> +    if (!NT_SUCCESS(status))
> +        goto fail4;
> +
>       Pdo->SectorSize = SectorSize;
>       Pdo->PhysSectorSize = PhysSectorSize;
> +    Pdo->DiscardSupported = DiscardSupported;
>
>       Size = SectorSize * SectorCount;
>       Size >>= 20; // Scale to megabytes
>
> -    Verbose("%s: %luMB (%uB sectors)\n",
> -            __PdoGetName(Pdo), Size, SectorSize);
> +    Verbose("%s: %luMB (%uB sectors)%s%s\n",
> +            __PdoGetName(Pdo), Size, SectorSize,
> +            Pdo->DiscardSupported ? " DISCARD" : "",
> +            Pdo->InterceptTrim ? "" : " (VETOED)");
>
>       __PdoSetSystemPowerState(Pdo, PowerSystemWorking);
>       __PdoSetDevicePowerState(Pdo, PowerDeviceD0);
> @@ -874,6 +950,9 @@ PdoStartDevice(
>
>       return STATUS_SUCCESS;
>
> +fail4:
> +    Error("fail4\n");
> +
>   fail3:
>       Error("fail3\n");
>
> @@ -1904,6 +1983,7 @@ PdoDestroy(
>       Dx->Pdo = NULL;
>
>       Pdo->InterceptTrim = FALSE;
> +    Pdo->DiscardSupported = FALSE;
>
>       RtlZeroMemory(Pdo->Name, sizeof (Pdo->Name));
>



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech