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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> 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))

.

> +/*
> + * 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?

> @@ -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.

Everything else looks fine to me now.

Jan


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