Re: [Xen-devel] [PATCH v4 11/34] xsplice: Design document

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Mar 23, 2016 at 05:18:39AM -0600, Jan Beulich wrote:
> >>> On 15.03.16 at 18:56, <konrad.wilk@xxxxxxxxxx> wrote:
> > +### 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 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
> > +the responsibility of the toolstack to use the `version` field to check
> > +between each invocation. if the version differs it should discard the stale
> > +data and start from scratch. It is OK for the toolstack to use the new
> > +`version` field.
> > +
> > +The `struct xen_xsplice_status` structure contains an status of payload 
> > which includes:
> > +
> > + * `status` - indicates the current status of the payload:
> > +   * *XSPLICE_STATUS_CHECKED*  (1) loaded and the ELF payload safety 
> > checks passed.
> > +   * *XSPLICE_STATUS_APPLIED* (2) loaded, checked, and applied.
> > +   *  No other value is possible.
> > + * `rc` - -XEN_EXX type errors encountered while performing the last
> > +   XSPLICE_ACTION_* operation. The normal values can be zero or 
> > -XEN_EAGAIN which
> > +   respectively mean: success or operation in progress. Other values
> > +   imply an error occurred. If there is an error in `rc`, `status` will 
> > **NOT**
> > +   have changed.
> > +
> > +The structure is as follow:
> > +
> > +<pre>
> > +struct xen_sysctl_xsplice_list {  
> > +    uint32_t version;                       /* IN/OUT: Initially *MUST* be 
> > zero.  
> > +                                               On subsequent calls reuse 
> > value.  
> > +                                               If varies between calls, we 
> > are  
> > +                                             * getting stale data. */  
> > +    uint32_t idx;                           /* IN/OUT: Index into array. 
> > */ 
> > +    uint32_t nr;                            /* IN: How many status, names, 
> > and len  
> > +                                               should fill out.  
> > +                                               OUT: How many payloads 
> > left. */  
> 
> I think there's an ambiguity left in both the description above and
> the comments here: With idx required to be zero upon first
> invocation (which I'm not clear why that is), which parts of the

That is actually a stale design choice. Initially the "How many payloads left"
was going to be stamped in 'idx'. But it is now in 'nr'.

The value can be arbitrary, albeit on first invocation it should be 0 otherwise
you won't get 'nr' telling you how many payloads there left. Unless your
'idx' falls below the amount of payloads.

As in, say we have 20 payloads.
If the first hypercall for 'idx' has 30, then the hypercall will return -EINVAL.
If the first hypercall 'idx' has 19, then the hypercall will populate
->name,->len,->status, ->version and write ->nr with 1.

> three arrays get filled when idx is non-zero: [0, idx) or [nr, nr + idx)?

I am going to assume the you are filling the two /*IN*/ entries, so ->idx
and ->nr.

[0, idx]:

If there is data and the amount of payloads is greater than idx (0), and there
are no hypercall preemptions, then:

->nr = remaining amount
->version = version value
->name[0..idx]
->len[0..idx]
->status[0..idx]


[nr, nr + idx]:

If there is data and the amount of payloads is less than nr, then -EINVAL
is returned.

If there is data and the amount of payloads is greater than 'nr', and there
are no hypercall preemptions, then:

->nr = remaining amount
->version = version value
->name[nr..nr + idx]
->len[nr..nr+idx]
->status[nr..nr+idx]

Let me update the design doc to remove the /*IN*/ part about the 'idx'.


diff --git a/docs/misc/xsplice.markdown b/docs/misc/xsplice.markdown
index 6aa5a27..8252e6c 100644
--- a/docs/misc/xsplice.markdown
+++ b/docs/misc/xsplice.markdown
@@ -487,7 +487,9 @@ hypervisor.
 The caller provides:
 
  * `version`. Initially (on first hypercall) *MUST* be zero.
- * `idx` index iterator. On first call *MUST* be zero, subsequent calls varies.
+ * `idx` index iterator. The index into the hypervisor's payload count. It is
+    recommended that on first invocation zero be used so that `nr` (which the
+    hypervisor will update with the remaining payload count) be provided.
  * `nr` the max number of entries to populate.
  * `pad` - *MUST* be zero.
  * `status` virtual address of where to write `struct xen_xsplice_status`
@@ -538,9 +540,9 @@ struct xen_sysctl_xsplice_list {
                                                On subsequent calls reuse 
value.  
                                                If varies between calls, we are 
 
                                              * getting stale data. */  
-    uint32_t idx;                           /* IN/OUT: Index into array. */  
+    uint32_t idx;                           /* IN: Index into array. */  
     uint32_t nr;                            /* IN: How many status, names, and 
len  
-                                               should fill out.  
+                                               should be filled out.  
                                                OUT: How many payloads left. */ 
 
     uint32_t pad;                           /* IN: Must be zero. */  
     XEN_GUEST_HANDLE_64(xen_xsplice_status_t) status;  /* OUT. Must have 
enough  
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel