Re: [Xen-devel] [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 29 September 2017 16:35
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
> servers rather than a list
> 
> On 29/09/17 15:51, 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.
> >
> > Some function return values are changed by this patch: Specifically, in
> > the case where the id of the default ioreq server is passed in, -
> EOPNOTSUPP
> > is now returned rather than -ENOENT.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >
> > v8:
> >  - Addressed various comments from Jan.
> >
> > v7:
> >  - Fixed assertion failure found in testing.
> >
> > v6:
> >  - Updated according to comments made by Roger on v4 that I'd missed.
> >
> > v5:
> >  - Switched GET/SET_IOREQ_SERVER() macros to get/set_ioreq_server()
> >    functions to avoid possible double-evaluation issues.
> >
> > 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         | 525 ++++++++++++++++++++--------------
> -----
> >  xen/include/asm-x86/hvm/domain.h |  10 +-
> >  2 files changed, 270 insertions(+), 265 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> > index f2e0b3f74a..e655d2eab3 100644
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -33,6 +33,41 @@
> >
> >  #include <public/hvm/ioreq.h>
> >
> > +static void set_ioreq_server(struct domain *d, unsigned int id,
> > +                             struct hvm_ioreq_server *s)
> > +{
> > +    ASSERT(id < MAX_NR_IOREQ_SERVERS);
> > +    ASSERT(!s || !d->arch.hvm_domain.ioreq_server.server[id]);
> > +
> > +    d->arch.hvm_domain.ioreq_server.server[id] = s;
> > +}
> > +
> > +#define GET_IOREQ_SERVER(d, id) \
> > +    (d)->arch.hvm_domain.ioreq_server.server[id]
> > +
> > +static struct hvm_ioreq_server *get_ioreq_server(const struct domain
> *d,
> > +                                                 unsigned int id)
> > +{
> > +    if ( id >= MAX_NR_IOREQ_SERVERS )
> > +        return NULL;
> > +
> > +    return GET_IOREQ_SERVER(d, id);
> > +}
> > +
> > +#define IS_DEFAULT(s) \
> > +    ((s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
> > +
> > +/*
> > + * Iterate over all possible ioreq servers. The use of inline function
> > + * get_ioreq_server() in the increment is deliberate as use of the
> > + * GET_IOREQ_SERVER() macro will cause gcc to complain about an array
> > + * overflow.
> > + */
> > +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
> > +    for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
> > +          (id) < MAX_NR_IOREQ_SERVERS; \
> > +          (s) = get_ioreq_server(d, ++(id)) )
> 
> I'm guessing from the various constructs, the list of ioreq servers
> might have embedded NULLs in the middle?
> 
> If so, how about this?
> 
> #define FOR_EACH_IOREQ_SERVER(d, id, s) \
>     for ( (id) = 0, (s) = GET_IOREQ_SERVER(d, 0); \
>           (id) < MAX_NR_IOREQ_SERVERS; \
>           (s) = get_ioreq_server(d, ++(id)) ) \
>     if ( !s ) \
>         continue; \
>     else

I'm ok with it but I'll wait for others opinion on whether this is taking the 
macro magic too far.

> 
> Every single use of this loop has the continue clause, which will go
> subtly wrong if someone typos continue as break.  This construct will
> work correctly with or without braces in the main body of code.
> 
> (For very brave people,
> https://www.chiark.greenend.org.uk/~sgtatham/mp/
> is an interesting read for quite what is possible by taking the above to
> extremes.)
> 
> Beyond that, why is GET_IOREQ_SERVER() needed?  All it appears to do is
> complicate code which could perfectly easily use get_ioreq_server().
> 

It omits a bounds check in cases where one is not needed. I could open code the 
array dereference but I think that's less tidy.

  Paul

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