Re: [Xen-devel] [PATCH v2 1/1] public/io/netif.h: add gref mapping control messages

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

[Accidentally dropped list and other ccs]

> -----Original Message-----
> From: Paul Durrant
> Sent: 06 September 2017 15:49
> To: 'Joao Martins' <joao.m.martins@xxxxxxxxxx>
> Subject: RE: [PATCH v2 1/1] public/io/netif.h: add gref mapping control
> messages
> 
> > -----Original Message-----
> > From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > Sent: 06 September 2017 15:37
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: Xen-devel <xen-devel@xxxxxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>;
> > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Subject: Re: [PATCH v2 1/1] public/io/netif.h: add gref mapping control
> > messages
> >
> > On 09/06/2017 02:49 PM, Paul Durrant wrote:
> > >> -----Original Message-----
> > >> From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > >> Sent: 01 September 2017 15:51
> > >> To: Xen-devel <xen-devel@xxxxxxxxxxxxx>
> > >> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; Paul Durrant
> > <Paul.Durrant@xxxxxxxxxx>;
> > >> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Joao Martins
> > >> <joao.m.martins@xxxxxxxxxx>
> > >> Subject: [PATCH v2 1/1] public/io/netif.h: add gref mapping control
> > messages
> > >>
> > >> Adds 3 messages to allow guest to let backend keep grants mapped,
> > >> such that 1) guests allowing fast recycling of pages can avoid doing
> > >> grant ops for those cases, or otherwise 2) preferring copies over
> > >> grants and 3) always using a fixed set of pages for network I/O.
> > >>
> > >> The three control ring messages added are:
> > >>  - Add grefs to be mapped by backend
> > >>  - Remove grefs mappings (If they are not in use)
> > >>  - Get maximum amount of grefs kept mapped.
> > >>
> > >> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
> > >> ---
> > >>  xen/include/public/io/netif.h | 114
> > >> ++++++++++++++++++++++++++++++++++++++++++
> > >>  1 file changed, 114 insertions(+)
> > >>
> > >> diff --git a/xen/include/public/io/netif.h 
> > >> b/xen/include/public/io/netif.h
> > >> index ca0061410d..264c317471 100644
> > >> --- a/xen/include/public/io/netif.h
> > >> +++ b/xen/include/public/io/netif.h
> > >> @@ -353,6 +353,9 @@ struct xen_netif_ctrl_request {
> > >>  #define XEN_NETIF_CTRL_TYPE_SET_HASH_MAPPING_SIZE 5
> > >>  #define XEN_NETIF_CTRL_TYPE_SET_HASH_MAPPING      6
> > >>  #define XEN_NETIF_CTRL_TYPE_SET_HASH_ALGORITHM    7
> > >> +#define XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE 8
> > >> +#define XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING      9
> > >> +#define XEN_NETIF_CTRL_TYPE_PUT_GREF_MAPPING     10
> > >>
> > >>      uint32_t data[3];
> > >>  };
> > >> @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response {
> > >>  };
> > >>
> > >>  /*
> > >> + * Static Grants (struct xen_netif_gref_alloc)
> > >> + * ===========================================
> > >> + *
> > >> + * A frontend may provide a fixed set of grant references to be
> mapped
> > on
> > >> + * the backend. The message of type
> > >> XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > >> + * prior its usage in the command ring allows for creation of these
> > mappings.
> > >> + * The backend will maintain a fixed amount of these mappings.
> > >> + *
> > >> + * XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE lets a frontend
> > >> query how many
> > >> + * of these mappings can be kept.
> > >> + *
> > >> + * Each entry in the
> > XEN_NETIF_CTRL_TYPE_{ADD,PUT}_GREF_MAPPING
> > >> input table has
> > >
> > > ADD and PUT are slightly odd choices for opposites. Normally you'd have
> > 'get' and 'put' or 'add' and 'remove' (or 'delete').
> > >
> > That's true - I probably was too obsessed into fitting in 3 characters to 
> > avoid
> > realigning the earlier chunk listing all ctrl messages types. ADD, DEL
> probably
> > is a better one (GET would sound a bit strange for these ops).
> 
> ADD/DEL sounds fine to me.
> 
> >
> > >> + * the following format:
> > >> + *
> > >> + *    0     1     2     3     4     5     6     7  octet
> > >> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > >> + * | grant ref             |  flags    |  padding  |
> > >> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> > >> + *
> > >> + * grant ref: grant reference
> > >> + * flags: flags describing the control operation
> > >> + *
> > >> + */
> > >> +
> > >> +struct xen_netif_gref_alloc {
> > >
> > > Is 'alloc' really desirable here? What's being allocated?
> > >
> > Probably not my best choice of naming, but given that we aren't actually
> > mapping
> > on the frontend but rather the backend hence I choose 'alloc'. But as you
> hint
> > it might be misleading. Would 'map' or 'mapping' be better candidates?
> 
> I would just call it 'xen_netif_gref'. It's used for mapping and unmapping.
> 
> >
> > >> +       grant_ref_t ref;
> > >> +       uint16_t flags;
> > >> +
> > >> +#define _XEN_NETIF_CTRLF_GREF_readonly    0
> > >> +#define XEN_NETIF_CTRLF_GREF_readonly
> > >> (1U<<_XEN_NETIF_CTRLF_GREF_readonly)
> > >> +
> > >> +       uint8_t pad[2];
> > >> +};
> > >> +
> > >> +/*
> > >>   * Control messages
> > >>   * ================
> > >>   *
> > >> @@ -609,6 +647,82 @@ struct xen_netif_ctrl_response {
> > >>   *       invalidate any table data outside that range.
> > >>   *       The grant reference may be read-only and must remain valid 
> > >> until
> > >>   *       the response has been processed.
> > >> + *
> > >> + * XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE
> > >> + * -----------------------------------------
> > >> + *
> > >> + * This is sent by the frontend to fetch the number of grefs that can be
> > kept
> > >> + * mapped in the backend.
> > >> + *
> > >> + * Request:
> > >> + *
> > >> + *  type    = XEN_NETIF_CTRL_TYPE_GET_GREF_MAPPING_SIZE
> > >> + *  data[0] = queue index (assumed 0 for single queue)
> > >> + *  data[1] = 0
> > >> + *  data[2] = 0
> > >> + *
> > >> + * Response:
> > >> + *
> > >> + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation
> > not
> > >> + *                                                     supported
> > >> + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - The queue
> > index
> > >> is
> > >> + *                                                     out of range
> > >> + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation
> successful
> > >> + *  data   = maximum number of entries allowed in the gref mapping
> > table
> > >> + *           (if operation was successful) or zero if a mapping table is
> > >> + *           not supported (i.e. hash mapping is done only by modular
> > >> + *           arithmetic).
> > >
> > > Too much cut'n'paste here methinks ;-)
> > >
> > Oh gosh :( this wasn't intended. Sorry - will remove the last three lines.
> >
> > >> + *
> > >> + * XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > >> + * ------------------------------------
> > >> + *
> > >> + * This is sent by the frontend for backend to map a list of grant
> > >> + * references.
> > >> + *
> > >> + * Request:
> > >> + *
> > >> + *  type    = XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > >> + *  data[0] = queue index
> > >> + *  data[1] = grant reference of page containing the mapping list
> > >> + *            (assumed to start at beginning of grant)
> > >
> > > Should then be 'assumed to start at beginning of *page*'?
> > >
> > Yeap.
> >
> > >> + *  data[2] = size of list in entries
> > >> + *
> > >> + * Response:
> > >> + *
> > >> + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation
> > not
> > >> + *                                                     supported
> > >> + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - Operation
> > failed
> > >> + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation
> successful
> > >> + *
> > >> + * NOTE: Each entry in the input table has the format outlined
> > >> + *       in struct xen_netif_gref_alloc.
> > >> + *
> > >
> > > What happens if the backend can successfully map some of the refs, but
> > not all?
> > > Does the whole operation fail (the backend being required to unmap
> > anything that
> > > it successfully mapped)
> >
> > Right now, I am doing all-or-nothing approach meaning the whole
> operation
> > fails
> > (and backend unmaps everything)
> >
> > > or would it be better to have a per-ref status code in
> > > the structure, and allow partial success?
> > >
> > There's two bytes in padding, I could cram a status field there (8 bytes
> should
> > suffice?). Do you think it's worth it? The usefulness I see is allowing
> > unbounded mappings i.e. without knowing before the operation how
> many
> > it has
> > left - but while it would be a nicer interface, it would add overhead on the
> > backend, either 1) a second copy to the table gref 2) or else backend could
> > map
> > the table directly and unmap afterwards [with care to avoid things like XSA-
> > 155].
> >
> 
> I'm fine with an 'all or nothing' semantic as long as it's clearly documented 
> so
> that backends and frontends know what to expect. You may also want a
> distinct failure code for 'failed to map the page containing the
> xen_netif_grefs' vs. 'failed to map/unmap and individual xen_netif_gref'.
> 
> > >> + * XEN_NETIF_CTRL_TYPE_PUT_GREF_MAPPING
> > >> + * ------------------------------------
> > >> + *
> > >> + * This is sent by the frontend for backend to unmap a list of grant
> > >> +         * references.
> > >> + *
> > >> + * Request:
> > >> + *
> > >> + *  type    = XEN_NETIF_CTRL_TYPE_PUT_GREF_MAPPING
> > >> + *  data[0] = queue index
> > >> + *  data[1] = grant reference of page containing the mapping list
> > >> + *            (assumed to start at beginning of page)
> > >> + *  data[2] = size of list in entries
> > >> + *
> > >> + * Response:
> > >> + *
> > >> + *  status = XEN_NETIF_CTRL_STATUS_NOT_SUPPORTED     - Operation
> > not
> > >> + *                                                     supported
> > >> + *           XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER - Operation
> > failed
> > >> + *           XEN_NETIF_CTRL_STATUS_SUCCESS           - Operation
> successful
> > >> + *
> > >> + * NOTE: Each entry in the input table has the format outlined in
> > >> + *       struct xen_netif_gref_alloc. The only valid entries are those
> > >> + *       previously added with message
> > >> XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> > >> + *       are valid. Additionally, entries in inflight will deliver an 
> > >> error.
> > >
> > > Could you elaborate on what 'inflight' means?
> > >
> > 'inflight' refers to grefs already submitted in requests by the frontend for
> > which we haven't received responses yet. This mention is to avoid
> malicious
> > frontend playing games with the state of the gref. We could use the status
> > you
> > suggested above and let the frontend know that the gref is in use - though
> I
> > am
> > not sure if this is the right behaviour i.e. in case we are giving too much
> > information for the guest.
> 
> Well, failures are more problematic here since for an 'all or nothing' 
> semantic
> you'd need to have the backend re-map what it may have already
> unmapped, and a malicious frontend may revoke the grant before it can do
> so.
> 
> Cheers,
> 
>   Paul
> 
> >
> > > Cheers,
> > >
> > >   Paul
> > >
> >
> > Thanks for all the comments!
> >
> > Cheers,
> > Joao

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