[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 9/25/19 5:34 PM, Wieczorkiewicz, Pawel wrote:


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.

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.