From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v3 MISSING/23] xsplice: Design document (v7). Date: Fri, 19 Feb 2016 13:36:57 -0500 Message-ID: <20160219183652.GA11420@localhost.localdomain> References: <1455300361-13092-1-git-send-email-konrad.wilk@oracle.com> <1455314229-22155-1-git-send-email-konrad.wilk@oracle.com> <1455314229-22155-2-git-send-email-konrad.wilk@oracle.com> <56C5FD4002000078000D3C51@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aWpvX-00063W-FO for xen-devel@lists.xenproject.org; Fri, 19 Feb 2016 18:37:15 +0000 Content-Disposition: inline In-Reply-To: <56C5FD4002000078000D3C51@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Ian Campbell , jinsong.liu@alibaba-inc.com, Ian Jackson , Tim Deegan , mpohlack@amazon.de, ross.lagerwall@citrix.com, andrew.cooper3@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Thu, Feb 18, 2016 at 09:20:00AM -0700, Jan Beulich wrote: > >>> On 12.02.16 at 22:57, wrote: > > +struct xsplice_patch_func { > > + const char *name; > > + Elf64_Xwordnew_addr; > > Missing space. > > > + Elf64_Xword old_addr; > > + Elf64_Word new_size; > > + Elf64_Word long old_size; > > There are still two types left here. That wouldn't compile very well. > > > +### XEN_SYSCTL_XSPLICE_GET (1) > > + > > +Retrieve an status of an specific payload. This caller provides: > > + > > + * A `struct xen_xsplice_name` called `name` which has the unique name. > > + * A `struct xen_xsplice_status` structure which has all members > > + set to zero: That is: > > + * `status` *MUST* be set to zero. > > + * `rc` *MUST* be set to zero. > > Why is this? .. It had an _pad entry in earlier versions. Let me remove the whole 'set to zero.." > > > +The structure is as follow: > > + > > +
> > +struct xen_xsplice_status {  
> > +#define XSPLICE_STATUS_LOADED       1  
> > +#define XSPLICE_STATUS_CHECKED      2  
> > +#define XSPLICE_STATUS_APPLIED      3  
> > +    int32_t state;                  /* OUT: XSPLICE_STATE_*. IN: MUST be zero. */  
> > +    int32_t rc;                     /* OUT: 0 if no error, otherwise -XEN_EXX. */  
> > +                                    /* IN: MUST be zero. */
> > +};  
> > +
> > +struct xen_sysctl_xsplice_summary {  
> > +    xen_xsplice_name_t name;        /* IN, the name of the payload. */  
> > +    xen_xsplice_status_t status;    /* IN/OUT: status of the payload. */  
> > +};  
> 
> With the operation being named XEN_SYSCTL_XSPLICE_GET, shouldn't
> the structure tag be xen_sysctl_xsplice_get?

Yes!
> 
> > +### XEN_SYSCTL_XSPLICE_LIST (2)
> > +
> > +Retrieve an array of abbreviated status and names of payloads that are 
> > loaded in the
> > +hypervisor.
> > +
> > +The caller provides:
> > +
> > + * `version`. Initially (on first hypercall) *MUST* be zero.
> > + * `idx` index iterator. On first call *MUST* be zero, subsequent calls varies.
> > + * `nr` the max number of entries to populate.
> > + * `pad` - *MUST* be zero.
> > + * `status` virtual address of where to write `struct xen_xsplice_status`
> > +   structures. Caller *MUST* allocate up to `nr` of them.
> > + * `name` - virtual address of where to write the unique name of the payload.
> > +   Caller *MUST* allocate up to `nr` of them. Each *MUST* be of
> > +   **XEN_XSPLICE_NAME_SIZE** size.
> > + * `len` - virtual address of where to write the length of each unique name
> > +   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). The `status`, `name`, and `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.
> > +
> > +If the hypercall returns an zero value that means there are no payloads.
> 
> Maybe worth changing to "... there are no (more) payloads",
> considering the iterative nature of the operation?

Yes.

This is the change I did:


diff --git a/docs/misc/xsplice.markdown b/docs/misc/xsplice.markdown
index 9a95243..69c5176 100644
--- a/docs/misc/xsplice.markdown
+++ b/docs/misc/xsplice.markdown
@@ -289,10 +289,10 @@ describing the functions to be patched:
 
 struct xsplice_patch_func {  
     const char *name;  
-    Elf64_Xwordnew_addr;  
+    Elf64_Xword new_addr;  
     Elf64_Xword old_addr;  
     Elf64_Word new_size;  
-    Elf64_Word long old_size;  
+    Elf64_Word old_size;  
     uint8_t pad[32];  
 };  
 

@@ -425,10 +425,8 @@ struct xen_sysctl_xsplice_upload {
 Retrieve an status of an specific payload. This caller provides:
 
  * A `struct xen_xsplice_name` called `name` which has the unique name.
- * A `struct xen_xsplice_status` structure which has all members
-   set to zero: That is:
-   * `status` *MUST* be set to zero.
-   * `rc` *MUST* be set to zero.
+ * A `struct xen_xsplice_status` structure. The member values will
+   be over-written upon completion.
 
 Upon completion the `struct xen_xsplice_status` is updated.
 
@@ -473,12 +471,11 @@ struct xen_xsplice_status {
 #define XSPLICE_STATUS_LOADED       1  
 #define XSPLICE_STATUS_CHECKED      2  
 #define XSPLICE_STATUS_APPLIED      3  
-    int32_t state;                  /* OUT: XSPLICE_STATE_*. IN: MUST be zero. */  
+    int32_t state;                  /* OUT: XSPLICE_STATE_*. */  
     int32_t rc;                     /* OUT: 0 if no error, otherwise -XEN_EXX. */  
-                                    /* IN: MUST be zero. */
 };  
 
-struct xen_sysctl_xsplice_summary {  
+struct xen_sysctl_xsplice_get {  
     xen_xsplice_name_t name;        /* IN, the name of the payload. */  
     xen_xsplice_status_t status;    /* IN/OUT: status of the payload. */  
 };  
@@ -514,7 +511,7 @@ value (`idx`) with the returned value of data.
 If the hypercall returns -XEN_E2BIG the `nr` is too big and should be
 lowered.
 
-If the hypercall returns an zero value that means there are no payloads.
+If the hypercall returns an zero value there are no more payloads.
 
 Note that due to the asynchronous nature of hypercalls the control domain might
 have added or removed a number of payloads making this information stale. It is
> 
> Jan
>