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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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