|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 11/12] livepatch: Add metadata runtime retrieval mechanism
> On 25. Sep 2019, at 17:47, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> 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.
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |