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

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On Mon, Sep 18, 2017 at 12:11:04PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > Sent: 18 September 2017 12:56
> > 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 v3 1/1] public/io/netif.h: add gref mapping control
> > messages
> > 
> > On Mon, Sep 18, 2017 at 09:53:18AM +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Joao Martins [mailto:joao.m.martins@xxxxxxxxxx]
> > > > Sent: 13 September 2017 19:11
> > > > 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 v3 1/1] public/io/netif.h: add gref mapping control
> > messages

[snip]

> > > > + * 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
> > > > + *            (r/w and 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
> > > > + *  data   = number of entries that were mapped
> > > > + *
> > > > + * NOTE: Each entry in the input table has the format outlined
> > > > + *       in struct xen_netif_gref.
> > >
> > > You may want to put words here about the 'all or nothing' semantics of
> > > this operation vs. the semantics of the 'del' operation below.
> > >
> > Good point I'll add a paragraph about that.
> > 
> > For the unmap it is clear that status should be per-entry for reasons
> > discussed on v2. Do you think ADD 'all or nothing' like I had on v2 ?
> > If so I should remove the 'data' return part since it is not really
> > useful here.
> 
> The 'all or nothing' semantic is easier for the frontend to deal with,
> so I think that's the way to go. Otherwise you'd need the per-entry
> status, as you say. Either way, I don't think the data return is
> particularly useful.
> 
Yeap.

The 'data' return was to allow both cases but leaving the decision to
implementors meaning if number of mapped entries was the same as the
input size (data[2]) then frontend wouldn't need to check all entries.
But it would still need to unmap on partial success, as that
is not guaranteed by design. On a 'all or nothing', 'data' doesn't really has
any meaning and definitely makes life easier for frontend.

> > 
> > > > + *
> > > > + * XEN_NETIF_CTRL_TYPE_DEL_GREF_MAPPING
> > > > + * ------------------------------------
> > > > + *
> > > > + * This is sent by the frontend for backend to unmap a list of grant
> > > > + * references.
> > > > + *
> > > > + * Request:
> > > > + *
> > > > + *  type    = XEN_NETIF_CTRL_TYPE_DEL_GREF_MAPPING
> > > > + *  data[0] = queue index
> > > > + *  data[1] = grant reference of page containing the mapping list
> > > > + *            (r/w and 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
> > > > + *  data   = number of entries that were unmapped
> > > > + *
> > > > + * NOTE: Each entry in the input table has the format outlined in 
> > > > struct
> > > > + *       xen_netif_gref. The only valid entries are those previously 
> > > > added
> > with
> > > > + *       message XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING are valid,
> > > > otherwise
> > > > + *       entries status are set to
> > > > XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER.
> > > > + *       It will also have the same error for entries inflight i.e. 
> > > > used in
> > > > + *       requests for which haven't been sent responses to the the
> > frontend.
> > >
> > > This last paragraph is a bit garbled.
> > 
> > Hmm, sorry for the twisted english. I removed the inflight part which
> > was making it more confusing and made it clear in the paragraph that the
> > only valid entries in this message are those added with
> > ADD_GREF_MAPPING. Hopefully the text below it's better?
> > 
> > NOTE: Each entry in the input table has the format outlined in struct
> > xen_netif_gref. The mappings hereby used are only the ones previously
> > added with message XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING. Any
> > other
> > mappings used here will deliver an
> > XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER.
> 
> I think that last sentence might be better as something like:
> 
> 'The entries used are only the ones representing grant references that
> were previously the subject of a XEN_NETIF_CTRL_TYPE_ADD_GREF_MAPPING
> operation. Any other entries will have their status set to
> XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER upon completion.'
> 
> Does that sound ok?
> 
Yeap, sounds great :)

> Cheers,
> 
>    Paul
> 
> > 
> > Thank you!

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