Re: [Xen-devel] [PATCH v8 07/27] ARM: vGICv3: handle virtual LPI pending and property tables

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 12/04/17 14:13, Julien Grall wrote:
> 
> 
> On 12/04/17 14:12, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>>
>> On 12/04/17 11:55, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 12/04/17 01:44, Andre Przywara wrote:
>>>> Allow a guest to provide the address and size for the memory regions
>>>> it has reserved for the GICv3 pending and property tables.
>>>> We sanitise the various fields of the respective redistributor
>>>> registers.
>>>> The MMIO read and write accesses are protected by locks, to avoid any
>>>> changing of the property or pending table address while a redistributor
>>>> is live and also to protect the non-atomic vgic_reg64_extract()
>>>> function
>>>> on the MMIO read side.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>>> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
>>>
>>> Whilst I gave my reviewed-by it is very rude to ignore a comment.
>>
>> Yeah, sorry about that! I was unsure about that as well, so thought
>> about it and eventually forgot to answer.
>>
>>> It would have been nicer to answer even if it is just saying "I can add
>>> a TODO and address it in a follow-up patch".
>>>
>>> Please get use to mention all the changes (e.g the spin_*lock ->
>>> spin_*lock_irq* change) you made in a patch. Mainly if you keep a
>>> reviewed-by.
>>
>> I was really unsure about keeping or dropping it, but since you
>> complained about me dropping it last time I tried it the other way this
>> time ;-)
>>
>>>> diff --git a/xen/include/asm-arm/domain.h
>>>> b/xen/include/asm-arm/domain.h
>>>> index ebaea35..b2d98bb 100644
>>>> --- a/xen/include/asm-arm/domain.h
>>>> +++ b/xen/include/asm-arm/domain.h
>>>> @@ -109,11 +109,15 @@ struct arch_domain
>>>>          } *rdist_regions;
>>>>          int nr_regions;                     /* Number of rdist
>>>> regions */
>>>>          uint32_t rdist_stride;              /* Re-Distributor
>>>> stride */
>>>> +        unsigned long int nr_lpis;
>>>> +        uint64_t rdist_propbase;
>>>>          struct rb_root its_devices;         /* Devices mapped to an
>>>> ITS */
>>>>          spinlock_t its_devices_lock;        /* Protects the
>>>> its_devices tree */
>>>>          struct radix_tree_root pend_lpi_tree; /* Stores struct
>>>> pending_irq's */
>>>>          rwlock_t pend_lpi_tree_lock;        /* Protects the
>>>> pend_lpi_tree */
>>>>          unsigned int intid_bits;
>>>> +        bool rdists_enabled;                /* Is any redistributor
>>>> enabled? */
>>>> +        bool has_its;
>>>
>>> The comment you ignore was the one about consolidating rdists_enabled
>>> and has_its in a single field and use flags.
>>
>> Yes, I had the idea myself before, but decided against it as IMHO it
>> looks much nicer this way (compared to using a flags variable. which I
>> guess is what you mean).
>>
>> Are you OK with that?
> 
> Why is it nicer?

Because I think it's more readable to have: "if ( has_its )" than
"if ( flags & HAS_ITS )".

> You only need 1 bit to represent rdist_enabled and
> another for has_its. This would save a bit a space in a resource limited
> structure.

But because of the padding it wouldn't, so I'd keep it as it is.

Cheers,
Andre.

> I would be OK with a TODO so we know we can save space here
> in the future...
> 
> Although, my main point was you not ignore comment and say no if you
> disagree.
> 
> Cheers,
> 

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