Re: [edk2-devel] [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory

["Laszlo Ersek" <lersek@redhat.com>]

On 09/13/19 16:50, Anthony PERARD wrote:
> XsRead and XsBackendRead of the XENBUS_PROTOCOL return allocated
> memory but this isn't allowed during the ExitBootServices call. We
> need XsRead and XsBackendRead to disconnect from the device so
> XENBUS_PROTOCOL is changed to use a buffer supplied by a child driver.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2190
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/Include/Protocol/XenBus.h | 32 ++++++++++++----------
>  OvmfPkg/XenBusDxe/XenStore.c      | 44 +++++-------------------------
>  OvmfPkg/XenBusDxe/XenStore.h      |  6 +++--
>  OvmfPkg/XenPvBlkDxe/BlockFront.c  | 45 ++++++++++++++++++-------------
>  4 files changed, 54 insertions(+), 73 deletions(-)
> 
> diff --git a/OvmfPkg/Include/Protocol/XenBus.h b/OvmfPkg/Include/Protocol/XenBus.h
> index 8ff5ca3575..c22bdfb368 100644
> --- a/OvmfPkg/Include/Protocol/XenBus.h
> +++ b/OvmfPkg/Include/Protocol/XenBus.h
> @@ -35,6 +35,12 @@ typedef struct
>  
>  #define XST_NIL ((XENSTORE_TRANSACTION *) NULL)
>  
> +//
> +// When reading a node from xenstore, if the size of the data to be read is
> +// unknown, this value can be use for the size of the buffer.
> +//
> +#define XENSTORE_PAYLOAD_MAX 4096
> +

This macro is already defined in "IndustryStandard/Xen/io/xs_wire.h".
Can we get it from there?

The extra documentation is OK, of course (replacing "this value" with
"XENSTORE_PAYLOAD_MAX").

Other than that, I'm going to have to ACK this after a brief skim only.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

>  typedef enum {
>    XENSTORE_STATUS_SUCCESS = 0,
>    XENSTORE_STATUS_FAIL,
> @@ -64,19 +70,17 @@ typedef enum {
>  ///
>  
>  /**
> -  Get the contents of the node Node of the PV device. Returns the contents in
> -  *Result which should be freed after use.
> +  Get the contents of the node Node of the PV device.
>  
>    @param This           A pointer to XENBUS_PROTOCOL instance.
>    @param Transaction    The XenStore transaction covering this request.
>    @param Node           The basename of the file to read.
> -  @param Result         The returned contents from this file.
> +  @param BufferSize     On input, a pointer to the size of the buffer at Buffer.
> +                        On output, the size of the data written to Buffer.
> +  @param Buffer         A pointer to a buffer into which the data read will be saved.
>  
>    @return  On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
>             indicating the type of failure.
> -
> -  @note The results buffer is malloced and should be free'd by the
> -        caller.
>  **/
>  typedef
>  XENSTORE_STATUS
> @@ -84,23 +88,22 @@ XENSTORE_STATUS
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Result
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    );
>  
>  /**
> -  Get the contents of the node Node of the PV device's backend. Returns the
> -  contents in *Result which should be freed after use.
> +  Get the contents of the node Node of the PV device's backend.
>  
>    @param This           A pointer to XENBUS_PROTOCOL instance.
>    @param Transaction    The XenStore transaction covering this request.
>    @param Node           The basename of the file to read.
> -  @param Result         The returned contents from this file.
> +  @param BufferSize     On input, a pointer to the size of the buffer at Buffer.
> +                        On output, the size of the data written to Buffer.
> +  @param Buffer         A pointer to a buffer into which the data read will be saved.
>  
>    @return  On success, XENSTORE_STATUS_SUCCESS. Otherwise an errno value
>             indicating the type of failure.
> -
> -  @note The results buffer is malloced and should be free'd by the
> -        caller.
>  **/
>  typedef
>  XENSTORE_STATUS
> @@ -108,7 +111,8 @@ XENSTORE_STATUS
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Result
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    );
>  
>  /**
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index b9588bb8c6..cb2d9e1215 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1336,27 +1336,11 @@ XenBusXenStoreRead (
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Value
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    )
>  {
> -  XENSTORE_STATUS Status;
> -  UINTN           BufferSize;
> -  VOID            *Buffer;
> -
> -  BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> -  Buffer = AllocatePool (BufferSize);
> -  if (Buffer == NULL) {
> -    return XENSTORE_STATUS_ENOMEM;
> -  }
> -
> -  Status = XenStoreRead (Transaction, This->Node, Node, &BufferSize, Buffer);
> -
> -  if (Status == XENSTORE_STATUS_SUCCESS) {
> -    *Value = Buffer;
> -  } else {
> -    FreePool (Buffer);
> -  }
> -  return Status;
> +  return XenStoreRead (Transaction, This->Node, Node, BufferSize, Buffer);
>  }
>  
>  XENSTORE_STATUS
> @@ -1365,27 +1349,11 @@ XenBusXenStoreBackendRead (
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Value
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    )
>  {
> -  XENSTORE_STATUS Status;
> -  UINTN           BufferSize;
> -  VOID            *Buffer;
> -
> -  BufferSize = XENSTORE_PAYLOAD_MAX + 1;
> -  Buffer = AllocatePool (BufferSize);
> -  if (Buffer == NULL) {
> -    return XENSTORE_STATUS_ENOMEM;
> -  }
> -
> -  Status = XenStoreRead (Transaction, This->Backend, Node, &BufferSize, Buffer);
> -
> -  if (Status == XENSTORE_STATUS_SUCCESS) {
> -    *Value = Buffer;
> -  } else {
> -    FreePool (Buffer);
> -  }
> -  return Status;
> +  return XenStoreRead (Transaction, This->Backend, Node, BufferSize, Buffer);
>  }
>  
>  XENSTORE_STATUS
> diff --git a/OvmfPkg/XenBusDxe/XenStore.h b/OvmfPkg/XenBusDxe/XenStore.h
> index 13f7d132e6..ca8c080433 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.h
> +++ b/OvmfPkg/XenBusDxe/XenStore.h
> @@ -289,7 +289,8 @@ XenBusXenStoreRead (
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Value
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    );
>  
>  XENSTORE_STATUS
> @@ -298,7 +299,8 @@ XenBusXenStoreBackendRead (
>    IN  XENBUS_PROTOCOL       *This,
>    IN  CONST XENSTORE_TRANSACTION *Transaction,
>    IN  CONST CHAR8           *Node,
> -  OUT VOID                  **Value
> +  IN OUT UINTN              *BufferSize,
> +  OUT VOID                  *Buffer
>    );
>  
>  XENSTORE_STATUS
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> index 8dca4c82f0..25a398ccc4 100644
> --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> @@ -41,19 +41,22 @@ XenBusReadUint64 (
>    )
>  {
>    XENSTORE_STATUS Status;
> -  CHAR8 *Ptr;
> +  CHAR8           Buffer[22];
> +  UINTN           BufferSize;
> +
> +  BufferSize = sizeof (Buffer) - 1;
>  
>    if (!FromBackend) {
> -    Status = This->XsRead (This, XST_NIL, Node, (VOID**)&Ptr);
> +    Status = This->XsRead (This, XST_NIL, Node, &BufferSize, Buffer);
>    } else {
> -    Status = This->XsBackendRead (This, XST_NIL, Node, (VOID**)&Ptr);
> +    Status = This->XsBackendRead (This, XST_NIL, Node, &BufferSize, Buffer);
>    }
>    if (Status != XENSTORE_STATUS_SUCCESS) {
>      return Status;
>    }
> +  Buffer[BufferSize] = '\0';
>    // AsciiStrDecimalToUint64 will ASSERT if Ptr overflow UINT64.
> -  *ValuePtr = AsciiStrDecimalToUint64 (Ptr);
> -  FreePool (Ptr);
> +  *ValuePtr = AsciiStrDecimalToUint64 (Buffer);
>    return Status;
>  }
>  
> @@ -143,43 +146,47 @@ XenPvBlockFrontInitialization (
>    OUT XEN_BLOCK_FRONT_DEVICE  **DevPtr
>    )
>  {
> -  XENSTORE_TRANSACTION Transaction;
> -  CHAR8 *DeviceType;
> -  blkif_sring_t *SharedRing;
> -  XENSTORE_STATUS Status;
> +  XENSTORE_TRANSACTION   Transaction;
> +  CHAR8                  Buffer[XENSTORE_PAYLOAD_MAX + 1];
> +  UINTN                  BufferSize;
> +  blkif_sring_t          *SharedRing;
> +  XENSTORE_STATUS        Status;
>    XEN_BLOCK_FRONT_DEVICE *Dev;
> -  XenbusState State;
> -  UINT64 Value;
> -  CHAR8 *Params;
> +  XenbusState            State;
> +  UINT64                 Value;
>  
>    ASSERT (NodeName != NULL);
>  
> +  BufferSize = sizeof (Buffer) - 1;
> +
>    Dev = AllocateZeroPool (sizeof (XEN_BLOCK_FRONT_DEVICE));
>    Dev->Signature = XEN_BLOCK_FRONT_SIGNATURE;
>    Dev->NodeName = NodeName;
>    Dev->XenBusIo = XenBusIo;
>    Dev->DeviceId = XenBusIo->DeviceId;
>  
> -  XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", (VOID**)&DeviceType);
> -  if (AsciiStrCmp (DeviceType, "cdrom") == 0) {
> +  BufferSize = sizeof (Buffer) - 1;
> +  XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", &BufferSize, Buffer);
> +  Buffer[BufferSize] = '\0';
> +  if (AsciiStrCmp (Buffer, "cdrom") == 0) {
>      Dev->MediaInfo.CdRom = TRUE;
>    } else {
>      Dev->MediaInfo.CdRom = FALSE;
>    }
> -  FreePool (DeviceType);
>  
>    if (Dev->MediaInfo.CdRom) {
> -    Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params", (VOID**)&Params);
> +    BufferSize = sizeof (Buffer) - 1;
> +    Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params",
> +      &BufferSize, Buffer);
>      if (Status != XENSTORE_STATUS_SUCCESS) {
>        DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n", __FUNCTION__, Status));
>        goto Error;
>      }
> -    if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
> -      FreePool (Params);
> +    Buffer[BufferSize] = '\0';
> +    if (AsciiStrLen (Buffer) == 0 || AsciiStrCmp (Buffer, "aio:") == 0) {
>        DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
>        goto Error;
>      }
> -    FreePool (Params);
>    }
>  
>    Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value);
>