Re: [edk2-devel] [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory
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
> ---
> 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
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 TransactionThe 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 TransactionThe 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;
> - }
> -
> -
[edk2-devel] [PATCH 08/11] OvmfPkg/XenBus: Change XENBUS_PROTOCOL to not return allocated memory
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
---
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
+
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 TransactionThe 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 TransactionThe 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, , 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
)
{
-
