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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.17 at 16:51, <jgross@xxxxxxxx> wrote:
> 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...

Which suggests that GET_IOREQ_SERVER() might better use a local
variable.

Btw, somewhere on the path here Paul and Roger have got dropped
from the list of recipients...

Jan

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