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

[Julien Grall <julien.grall@xxxxxxx>]

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.

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.
> 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.
>  #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,

--
Julien Grall

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