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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 07, 2017 at 04:51:53PM +0200, Juergen Gross 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@xxxxxxxxxx>
> > 
> > LGTM, just a couple of nitpicks, I think they can be fixed upon commit
> > if desired.
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > 
> >> ---
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>
> >> 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...

Heh, right, the dangers of macro expansion. GET_IOREQ_SERVER does more
than simply fetching the struct from the array.

Thanks, Roger.

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