| Message ID | 20170901145057.17414-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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'). > + * 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? > + 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 ;-) > + * > + * 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*'? > + * 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) or would it be better to have a per-ref status code in the structure, and allow partial success? > + * 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? Cheers, Paul > + * > */ > > DEFINE_RING_TYPES(xen_netif_ctrl, > -- > 2.11.0
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). >> + * 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? >> + 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]. >> + * 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. > Cheers, > > Paul > Thanks for all the comments! Cheers, Joao
[Accidentally dropped list and other ccs] > -----Original Message----- > From: Paul Durrant > Sent: 06 September 2017 15:49 > To: 'Joao Martins' <joao.m.martins@oracle.com> > Subject: RE: [PATCH v2 1/1] public/io/netif.h: add gref mapping control > messages > > > -----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. > > > > > >> + * 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
> -----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
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 + * 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 { + 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). + * + * 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) + * 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. + * + * 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. + * */ DEFINE_RING_TYPES(xen_netif_ctrl,
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(+)