On 9/25/19 5:34 PM, Wieczorkiewicz, Pawel wrote:
On 25. Sep 2019, at 17:47, Ross Lagerwall <ross.lagerw...@citrix.com> wrote:
On 9/16/19 11:59 AM, Pawel Wieczorkiewicz wrote:
Extend the livepatch list operation to fetch also payloads' metadata.
This is achieved by extending the sysctl list interface with 2 extra
guest handles:
* metadata - an array of arbitrary size strings
* metadata_len - an array of metadata strings' lengths (uin32_t each)
uint32_t
ACK
Payloads' metadata is a string of arbitrary size and does not have an
upper bound limit. It may also vary in size between payloads.
In order to let the userland allocate enough space for the incoming
data add a metadata total size field to the list sysctl operation and
fill it with total size of all payloads' metadata.
snip> + * `metadata` - Virtual address of where to write the metadata of the
payloads.
+ Caller *MUST* allocate enough space to be able to store all received data
+ (i.e. total allocated space *MUST* match the `metadata_total_size` value
+ provided by the hypervisor). Individual payload metadata string can be of
+ arbitrary length. The metadata string format is: key=value\0...key=value\0.
+ * `metadata_len` - Virtual address of where to write the length of each
metadata
+ string of the payload. Caller *MUST* allocate up to `nr` of them. Each
*MUST*
+ be of sizeof(uint32_t) (4 bytes).
If the hypercall returns an positive number, it is the number (upto `nr`
provided to the hypercall) of the payloads returned, along with `nr` updated
with the number of remaining payloads, `version` updated (it may be the same
across hypercalls - if it varies the data is stale and further calls could
-fail) and the `name_total_size` containing total size of transfered data for
-the array. The `status`, `name`, and `len` are updated at their designed index
-value (`idx`) with the returned value of data.
+fail), `name_total_size` and `metadata_total_size` containing total sizes of
+transfered data for both the arrays.
transferred
ACK
+The `status`, `name`, `len`, `metadata` and `metadata_len` are updated at their
+designed index value (`idx`) with the returned value of data.
If the hypercall returns -XEN_E2BIG the `nr` is too big and should be
lowered.
@@ -780,6 +790,7 @@ The structure is as follow:
OUT: How many payloads
left. */
uint32_t pad; /* IN: Must be zero. */
uint64_t name_total_size; /* OUT: Total size of all
transfer names */
+ uint64_t metadata_total_size; /* OUT: Total size of all
transfer metadata */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must
have enough
space allocate for nr of
them. */
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each
member
@@ -788,6 +799,12 @@ The structure is as follow:
nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of
name's.
Must have nr of them. */
+ XEN_GUEST_HANDLE_64(char) metadata; /* OUT: Array of metadata
strings. Each
+ member may have an
arbitrary length.
+ Must have nr of them. */
+ XEN_GUEST_HANDLE_64(uint32) metadata_len; /* OUT: Array of lengths of
metadata's.
+ Must have nr of them. */
+
};
snip
@@ -744,6 +753,8 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int
max,
struct xen_livepatch_status *info,
char *name, uint32_t *len,
const uint64_t name_total_size,
+ char *metadata, uint32_t *metadata_len,
+ const uint64_t metadata_total_size,
unsigned int *done, unsigned int *left)
{
int rc;
@@ -752,13 +763,16 @@ int xc_livepatch_list(xc_interface *xch, const unsigned
int max,
DECLARE_HYPERCALL_BOUNCE(info, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(name, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(len, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(metadata, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+ DECLARE_HYPERCALL_BOUNCE(metadata_len, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
uint32_t max_batch_sz, nr;
uint32_t version = 0, retries = 0;
uint32_t adjust = 0;
- off_t name_off = 0;
- uint64_t name_sz;
+ off_t name_off = 0, metadata_off = 0;
+ uint64_t name_sz, metadata_sz;
As with the previous patch, I think uint32_t would be more appropriate here as
I can't imagine a reason why the metadata would exceed 4 GiB?
And the same suggestion as with the previous patch to then change off_t
(probably to uint32_t).
Ok, I will apply both suggestions.
- if ( !max || !info || !name || !len || !done || !left )
+ if ( !max || !info || !name || !len ||
+ !metadata || !metadata_len || !done || !left )
{
errno = EINVAL;
return -1;
snip
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 503be68059..7786864926 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -920,16 +920,17 @@ struct xen_sysctl_livepatch_get {
};
/*
- * Retrieve an array of abbreviated status and names of payloads that are
- * loaded in the hypervisor.
+ * Retrieve an array of abbreviated status, names and metadata of payloads that
+ * are loaded in the hypervisor.
*
* If the hypercall returns an positive number, it is the number (up to `nr`)
* of the payloads returned, along with `nr` updated with the number of
remaining
* payloads, `version` updated (it may be the same across hypercalls. If it
varies
- * the data is stale and further calls could fail) and the name_total_size
- * containing total size of transfered data for the array.
- * The `status`, `name`, `len` are updated at their designed index value
(`idx`)
- * with the returned value of data.
+ * the data is stale and further calls could fail), `name_total_size` and
+ * `metadata_total_size` containing total sizes of transfered data for both the
transferred
ACK
+ * arrays.
+ * The `status`, `name`, `len`, `metadata` and `metadata_len` are updated at
their
+ * designed index value (`idx`) with the returned value of data.
*
* If the hypercall returns E2BIG the `nr` is too big and should be
* lowered. The upper limit of `nr` is left to the implemention.
@@ -953,6 +954,7 @@ struct xen_sysctl_livepatch_list {
OUT: How many payloads left. */
uint32_t pad; /* IN: Must be zero. */
uint64_t name_total_size; /* OUT: Total size of all
transfer names */
+ uint64_t metadata_total_size; /* OUT: Total size of all transfer
metadata */
XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status; /* OUT. Must have
enough
space allocate for nr of them.
*/
XEN_GUEST_HANDLE_64(char) name; /* OUT: Array of names. Each
member
@@ -961,6 +963,11 @@ struct xen_sysctl_livepatch_list {
nr of them. */
XEN_GUEST_HANDLE_64(uint32) len; /* OUT: Array of lengths of
name's.
Must have nr of them. */
+ XEN_GUEST_HANDLE_64(char) metadata; /* OUT: Array of metadata strings.
Each
+ member may have an arbitrary
length.
+ Must have nr of them. */
+ XEN_GUEST_HANDLE_64(uint32) metadata_len; /* OUT: Array of lengths of
metadata's.
+ Must have nr of them. */
};
/*
Do you think it would be useful for the metadata to be an optional OUT
parameter? I could imagine a caller wanting to get a list of live patches
without needing/wanting to get all the metadata as well.
Hmm… that would complicate the code to some extent, because we would have to
handle 3 request types: names+metadata, names, invalid.
The latter worries me most, as we would have to check all the conditions.
Not sure if it is worth it, since the metadata can just be retrieved and
ignored (of course assuming it is not too heavy).
Alternatively we could add an independent interface to retrieve just the
metadata.
But, when I was looking at this, it seemed like adding a lot of redundant code
(because the list operation already has all what’s needed).
Could the optional bits be done on top of this change as a separate patch?
I would play with this first, to sense how complicated this is.
Sure, it doesn't have to be done right now. It was just wondering if it
might be useful or for potential improvement. I agree that it might add
some complications for little benefit.
Secondly, there should also be (optional) metadata retrieval to the
XEN_SYSCTL_LIVEPATCH_GET call since a caller may want to get status & metadata
for a particular live patch without having to list all of them. That should be done
as a separate patch from this one, I think.
Yes, that definitely makes sense and can be useful. I also agree that this
should be done as a separate patch. Adding to my TODO.
Thanks,
--
Ross Lagerwall
Thanks for looking at the changes!
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
