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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.09.17 at 17:38, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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.

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.

Jan

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