Re: [PATCH v8 01/11] 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: 04 October 2017 11:16
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Subject: RE: [PATCH v8 01/11] x86/hvm/ioreq: maintain an array of ioreq
> servers rather than a list
> 
> >>> On 29.09.17 at 17:38, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Andrew Cooper
> >> Sent: 29 September 2017 16:35
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> 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@citrix.com>
> >> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >> > ---
> >> > Cc: Jan Beulich <jbeulich@suse.com>
> >> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >
> >> > 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.
> 
> I'm fine with Andrew's suggestion; I'm surprised though trickery
> like this is being suggested at all, as commonly games I happen
> to be trying to play once in a while don't seem to be really liked.
> 

Yeah, I'm always nervous of macros getting too large... mainly because they end up being a pain to debug in my experience, but I'll re-do the macro as Andrew suggests.

  Paul

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