| Message ID | 20170913181034.28527-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: 13 September 2017 19:11 > 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 v3 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> > --- > v3: > * Use DEL for unmapping grefs instead of PUT > * Rname from xen_netif_gref_alloc to xen_netif_gref > * Add 'status' field on xen_netif_gref > * Clarify what 'inflight' means > * Use "beginning of the page" instead of "beginning of the grant" > * Mention that page needs to be r/w (as it will have to modify \.status) > * `data` on ADD|PUT returns number of entries mapped/unmapped. > --- > xen/include/public/io/netif.h | 115 > ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 115 insertions(+) > > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h > index ca0061410d..0080a260fd 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_DEL_GREF_MAPPING 10 > > uint32_t data[3]; > }; > @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response { > }; > > /* > + * Static Grants (struct xen_netif_gref) > + * ===================================== > + * > + * 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,DEL}_GREF_MAPPING > input table has > + * the following format: > + * > + * 0 1 2 3 4 5 6 7 octet > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * | grant ref | flags | status | > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > + * > + * grant ref: grant reference > + * flags: flags describing the control operation > + * status: XEN_NETIF_CTRL_STATUS_* > + */ You may want to add some words here pointing out that the status is an 'out' field, and also whether it should be initialized to zero or not. > + > +struct xen_netif_gref { > + 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) > + > + uint16_t status; > +}; > + > +/* > * Control messages > * ================ > * > @@ -609,6 +647,83 @@ 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 it is not supported. > + * > + * 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. > + * > + * 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. Paul > */ > > DEFINE_RING_TYPES(xen_netif_ctrl, > -- > 2.11.0
On Mon, Sep 18, 2017 at 09:53:18AM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Joao Martins [mailto:joao.m.martins@oracle.com] > > Sent: 13 September 2017 19:11 > > 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 v3 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> > > --- > > v3: > > * Use DEL for unmapping grefs instead of PUT > > * Rname from xen_netif_gref_alloc to xen_netif_gref > > * Add 'status' field on xen_netif_gref > > * Clarify what 'inflight' means > > * Use "beginning of the page" instead of "beginning of the grant" > > * Mention that page needs to be r/w (as it will have to modify \.status) > > * `data` on ADD|PUT returns number of entries mapped/unmapped. > > --- > > xen/include/public/io/netif.h | 115 > > ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 115 insertions(+) > > > > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h > > index ca0061410d..0080a260fd 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_DEL_GREF_MAPPING 10 > > > > uint32_t data[3]; > > }; > > @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response { > > }; > > > > /* > > + * Static Grants (struct xen_netif_gref) > > + * ===================================== > > + * > > + * 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,DEL}_GREF_MAPPING > > input table has > > + * the following format: > > + * > > + * 0 1 2 3 4 5 6 7 octet > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * | grant ref | flags | status | > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > + * > > + * grant ref: grant reference > > + * flags: flags describing the control operation > > + * status: XEN_NETIF_CTRL_STATUS_* > > + */ > > You may want to add some words here pointing out that the status is an > 'out' field, and also whether it should be initialized to zero or not. > OK. > > + > > +struct xen_netif_gref { > > + 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) > > + > > + uint16_t status; > > +}; > > + > > +/* > > * Control messages > > * ================ > > * > > @@ -609,6 +647,83 @@ 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 it is not supported. > > + * > > + * 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. > > + * > > + * 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. Thank you!
> -----Original Message----- > From: Joao Martins [mailto:joao.m.martins@oracle.com] > Sent: 18 September 2017 12:56 > 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 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@oracle.com] > > > Sent: 13 September 2017 19:11 > > > 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 v3 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> > > > --- > > > v3: > > > * Use DEL for unmapping grefs instead of PUT > > > * Rname from xen_netif_gref_alloc to xen_netif_gref > > > * Add 'status' field on xen_netif_gref > > > * Clarify what 'inflight' means > > > * Use "beginning of the page" instead of "beginning of the grant" > > > * Mention that page needs to be r/w (as it will have to modify \.status) > > > * `data` on ADD|PUT returns number of entries mapped/unmapped. > > > --- > > > xen/include/public/io/netif.h | 115 > > > ++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 115 insertions(+) > > > > > > diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h > > > index ca0061410d..0080a260fd 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_DEL_GREF_MAPPING 10 > > > > > > uint32_t data[3]; > > > }; > > > @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response { > > > }; > > > > > > /* > > > + * Static Grants (struct xen_netif_gref) > > > + * ===================================== > > > + * > > > + * 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,DEL}_GREF_MAPPING > > > input table has > > > + * the following format: > > > + * > > > + * 0 1 2 3 4 5 6 7 octet > > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > > + * | grant ref | flags | status | > > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > > + * > > > + * grant ref: grant reference > > > + * flags: flags describing the control operation > > > + * status: XEN_NETIF_CTRL_STATUS_* > > > + */ > > > > You may want to add some words here pointing out that the status is an > > 'out' field, and also whether it should be initialized to zero or not. > > > OK. > > > > + > > > +struct xen_netif_gref { > > > + 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) > > > + > > > + uint16_t status; > > > +}; > > > + > > > +/* > > > * Control messages > > > * ================ > > > * > > > @@ -609,6 +647,83 @@ 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 it is not supported. > > > + * > > > + * 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. > > > > + * > > > + * 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? Cheers, Paul > > Thank you!
On Mon, Sep 18, 2017 at 12:11:04PM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Joao Martins [mailto:joao.m.martins@oracle.com] > > Sent: 18 September 2017 12:56 > > 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 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@oracle.com] > > > > Sent: 13 September 2017 19:11 > > > > 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 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!
diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h index ca0061410d..0080a260fd 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_DEL_GREF_MAPPING 10 uint32_t data[3]; }; @@ -391,6 +394,41 @@ struct xen_netif_ctrl_response { }; /* + * Static Grants (struct xen_netif_gref) + * ===================================== + * + * 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,DEL}_GREF_MAPPING input table has + * the following format: + * + * 0 1 2 3 4 5 6 7 octet + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * | grant ref | flags | status | + * +-----+-----+-----+-----+-----+-----+-----+-----+ + * + * grant ref: grant reference + * flags: flags describing the control operation + * status: XEN_NETIF_CTRL_STATUS_* + */ + +struct xen_netif_gref { + 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) + + uint16_t status; +}; + +/* * Control messages * ================ * @@ -609,6 +647,83 @@ 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 it is not supported. + * + * 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. + * + * 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. */ 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> --- v3: * Use DEL for unmapping grefs instead of PUT * Rname from xen_netif_gref_alloc to xen_netif_gref * Add 'status' field on xen_netif_gref * Clarify what 'inflight' means * Use "beginning of the page" instead of "beginning of the grant" * Mention that page needs to be r/w (as it will have to modify \.status) * `data` on ADD|PUT returns number of entries mapped/unmapped. --- xen/include/public/io/netif.h | 115 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+)