[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.