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

[Andre Przywara <andre.przywara@arm.com>]

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@arm.com>
>> Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> 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?

Cheers,
Andre.

> 
>>  #endif
>>      } vgic;
>>
>> @@ -260,6 +264,7 @@ struct arch_vcpu
>>
>>          /* GICv3: redistributor base and flags for this vCPU */
>>          paddr_t rdist_base;
>> +        uint64_t rdist_pendbase;
>>  #define VGIC_V3_RDIST_LAST      (1 << 0)        /* last vCPU of the
>> rdist */
>>  #define VGIC_V3_LPIS_ENABLED    (1 << 1)
>>          uint8_t flags;
>>
> 
> Cheers,
> 

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