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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@oracle.com]
> Sent: 06 September 2017 17:07
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Subject: Re: [PATCH v2 1/1] public/io/netif.h: add gref mapping control
> messages
> 
> [Is it meant to be offlist?]

Nope. My mistake.

> 
> On 09/06/2017 03:49 PM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Joao Martins [mailto:joao.m.martins@oracle.com]
> >> Sent: 06 September 2017 15:37
> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> Cc: Xen-devel <xen-devel@lists.xen.org>; Wei Liu <wei.liu2@citrix.com>;
> >> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> 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@oracle.com]
> >>>> Sent: 01 September 2017 15:51
> >>>> To: Xen-devel <xen-devel@lists.xen.org>
> >>>> Cc: Wei Liu <wei.liu2@citrix.com>; Paul Durrant
> >> <Paul.Durrant@citrix.com>;
> >>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Joao Martins
> >>>> <joao.m.martins@oracle.com>
> >>>> 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@oracle.com>
> >>>> ---
> >>>>  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.
> >
> OK. In case you prefer a slightly less verbose/long name like
> XEN_NETIF_CTRL_TYPE_{MAP,UNMAP}_GREF let me know :)
> 
> >>
> >>>> + * 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.
> >
> OK, got it.
> 
> >>>> + *  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'.
> >
> OK, good point. And given what you mention right after, the map/unmap of
> individual grefs might make more sense. And this return code is useful for
> both
> cases such that on individual gref case we could avoid transversing the list to
> see if it was successfully mapped (albeit probably a benefit is tiny)
> 
> >>>> + * 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.
> >
> Oh well, the all-or-nothing approach might be bad for the unmap case. So I
> think
> I'll go with your status code. Really wanna make sure I am not adding a
> side-channel for attacking/breaking up the backend.

Oh definitely. The backend needs to be coded defensively. It needs to be able to fail the frontends requests gracefully.

Cheers,

  Paul

> 
> Cheers,
> Joao

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