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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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().

~Andrew

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