Re: [PATCH v4 08/12] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 07 September 2017 16:17
> To: Roger Pau Monne <roger.pau@citrix.com>; Juergen Gross
> <jgross@suse.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v4 08/12] x86/hvm/ioreq: maintain an array
> of ioreq servers rather than a list
> 
> >>> On 07.09.17 at 16:51, <jgross@suse.com> wrote:
> > On 07/09/17 16:41, Roger Pau Monné wrote:
> >> On Tue, Sep 05, 2017 at 12:37:12PM +0100, Paul Durrant wrote:
> >>> A subsequent patch will remove the current implicit limitation on
> creation
> >>> of ioreq servers which is due to the allocation of gfns for the ioreq
> >>> structures and buffered ioreq ring.
> >>>
> >>> It will therefore be necessary to introduce an explicit limit and, since
> >>> this limit should be small, it simplifies the code to maintain an array of
> >>> that size rather than using a list.
> >>>
> >>> Also, by reserving an array slot for the default server and populating
> >>> array slots early in create, the need to pass an 'is_default' boolean
> >>> to sub-functions can be avoided.
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >>
> >> LGTM, just a couple of nitpicks, I think they can be fixed upon commit
> >> if desired.
> >>
> >> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >>> ---
> >>> Cc: Jan Beulich <jbeulich@suse.com>
> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>
> >>> v4:
> >>>  - Introduced more helper macros and relocated them to the top of the
> >>>    code.
> >>>
> >>> v3:
> >>>  - New patch (replacing "move is_default into struct hvm_ioreq_server")
> in
> >>>    response to review comments.
> >>> ---
> >>>  xen/arch/x86/hvm/ioreq.c         | 491
> > ++++++++++++++++++---------------------
> >>>  xen/include/asm-x86/hvm/domain.h |  11 +-
> >>>  2 files changed, 235 insertions(+), 267 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> >>> index f2e0b3f74a..287572bd1f 100644
> >>> --- a/xen/arch/x86/hvm/ioreq.c
> >>> +++ b/xen/arch/x86/hvm/ioreq.c
> >>> @@ -33,6 +33,22 @@
> >>>
> >>>  #include <public/hvm/ioreq.h>
> >>>
> >>> +#define SET_IOREQ_SERVER(d, id, s) \
> >>> +    (d)->arch.hvm_domain.ioreq_server.server[id] = (s)
> >>
> >> Are the parentheses around s required?
> >>
> >>> +
> >>> +#define GET_IOREQ_SERVER(d, id) \
> >>> +    (((id) < MAX_NR_IOREQ_SERVERS) ? \
> >>> +     (d)->arch.hvm_domain.ioreq_server.server[id] : \
> >>> +     NULL)
> >>> +
> >>> +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
> >>> +    for ( (id) = 0, (s) = GET_IOREQ_SERVER((d), (id)); \
> >>> +          (id) < MAX_NR_IOREQ_SERVERS; \
> >>> +          (id)++, (s) = GET_IOREQ_SERVER((d), (id)) )
> >>
> >> Same here about the parentheses around s, d and id in the
> >> GET_IOREQ_SERVER calls. In fact you could compact the afterthought as:
> >>
> >> s = GET_IOREQ_SERVER(d, ++(id))
> >
> > Uuh, this would be wrong: id is used twice in GET_IOREQ_SERVER(), so it
> > would be incremented twice...

Ouch! Thanks for spotting that.

> 
> Which suggests that GET_IOREQ_SERVER() might better use a local
> variable.

I'll turn it into a function.

  Paul

> 
> Btw, somewhere on the path here Paul and Roger have got dropped
> from the list of recipients...
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel