Re: [Xen-devel] [PATCH v4 08/12] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

[Juergen Gross <jgross@xxxxxxxx>]

On 07/09/17 16:41, Roger Pau Monné wrote:
> On Tue, Sep 05, 2017 at 12:37:12PM +0100, 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.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> LGTM, just a couple of nitpicks, I think they can be fixed upon commit
> if desired.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
>> ---
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> 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         | 491 
>> ++++++++++++++++++---------------------
>>  xen/include/asm-x86/hvm/domain.h |  11 +-
>>  2 files changed, 235 insertions(+), 267 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index f2e0b3f74a..287572bd1f 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -33,6 +33,22 @@
>>  
>>  #include <public/hvm/ioreq.h>
>>  
>> +#define SET_IOREQ_SERVER(d, id, s) \
>> +    (d)->arch.hvm_domain.ioreq_server.server[id] = (s)
> 
> Are the parentheses around s required?
> 
>> +
>> +#define GET_IOREQ_SERVER(d, id) \
>> +    (((id) < MAX_NR_IOREQ_SERVERS) ? \
>> +     (d)->arch.hvm_domain.ioreq_server.server[id] : \
>> +     NULL)
>> +
>> +#define FOR_EACH_IOREQ_SERVER(d, id, s) \
>> +    for ( (id) = 0, (s) = GET_IOREQ_SERVER((d), (id)); \
>> +          (id) < MAX_NR_IOREQ_SERVERS; \
>> +          (id)++, (s) = GET_IOREQ_SERVER((d), (id)) )
> 
> Same here about the parentheses around s, d and id in the
> GET_IOREQ_SERVER calls. In fact you could compact the afterthought as:
> 
> s = GET_IOREQ_SERVER(d, ++(id))

Uuh, this would be wrong: id is used twice in GET_IOREQ_SERVER(), so it
would be incremented twice...


Juergen

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