Re: [PATCH v9 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: 09 October 2017 13:40
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v9 01/11] x86/hvm/ioreq: maintain an array of ioreq
> servers rather than a list
> 
> >>> On 06.10.17 at 14:25, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -33,6 +33,44 @@
> >
> >  #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))
> 
> While at this point it looks like all users of this macro either
> explicitly check s to be non-NULL before invoking the macro or
> are being called with s guaranteed non-NULL, going forward it
> may easily be that NULL might be handed here. Therefore I
> think it would be better to either add an ASSERT() or do
> 
> #define IS_DEFAULT(s) \
>     ((s) && (s) == get_ioreq_server((s)->domain, DEFAULT_IOSERVID))
> 

Ok, I'll code it as you suggest.

> .
> 
> > +/*
> > + * 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.
> 
> I think you shouldn't accuse gcc of complaining, but simply state
> that there _will be_ an array overflow otherwise.
> 
> > + */
> > +#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 ) \
> 
> Alternatively, how about folding both macro and inline function
> invocation be putting them in the if() here?

I guess that now the if is there I could do that... probably will look neater.

> 
> > @@ -685,52 +688,64 @@ int hvm_create_ioreq_server(struct domain *d,
> domid_t domid,
> >                              ioservid_t *id)
> >  {
> >      struct hvm_ioreq_server *s;
> > +    unsigned int i;
> >      int rc;
> >
> >      if ( bufioreq_handling > HVM_IOREQSRV_BUFIOREQ_ATOMIC )
> >          return -EINVAL;
> >
> > -    rc = -ENOMEM;
> >      s = xzalloc(struct hvm_ioreq_server);
> >      if ( !s )
> > -        goto fail1;
> > +        return -ENOMEM;
> >
> >      domain_pause(d);
> >      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
> >
> > -    rc = -EEXIST;
> > -    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
> > -        goto fail2;
> > -
> > -    rc = hvm_ioreq_server_init(s, d, domid, is_default, bufioreq_handling,
> > -                               next_ioservid(d));
> > -    if ( rc )
> > -        goto fail3;
> > -
> > -    list_add(&s->list_entry,
> > -             &d->arch.hvm_domain.ioreq_server.list);
> > -
> >      if ( is_default )
> >      {
> > -        d->arch.hvm_domain.default_ioreq_server = s;
> > -        hvm_ioreq_server_enable(s, true);
> > +        i = DEFAULT_IOSERVID;
> > +
> > +        rc = -EEXIST;
> > +        if ( GET_IOREQ_SERVER(d, i) )
> > +            goto fail;
> >      }
> > +    else
> > +    {
> > +        for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
> > +        {
> > +            if ( i != DEFAULT_IOSERVID && !GET_IOREQ_SERVER(d, i) )
> > +                break;
> > +        }
> 
> Strictly speaking the braces here are pointless. But you're the
> maintainer, so you know what you like.

Yes, I prefer to keep them.

> 
> Everything else looks fine to me now.
> 

Cool. Thanks,

  Paul

> Jan


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