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

Re: [Xen-devel] [PATCH 21/57] ARM: GICv2: extend LR read/write functions to cover EOI and source





On 03/08/2018 04:41 PM, Julien Grall wrote:
Hi,

On 08/03/18 16:25, Andre Przywara wrote:
On 06/03/18 16:06, Julien Grall wrote:
On 05/03/18 16:03, Andre Przywara wrote:
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 8fab458d7f..89a07ae6b4 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -223,9 +223,11 @@ struct gic_lr {
      /* Virtual IRQ */
      uint32_t virq;
      uint8_t priority;
-   uint8_t state;
-   uint8_t hw_status;
-   uint8_t grp;
+   uint8_t source;
+   uint8_t state:2;
+   uint8_t hw_status:1;
+   uint8_t grp:1;
+   uint8_t eoi:1;

I would much prefer to introduce an union with specific information for
the physical interrupt (i.e GICH_LR.HW = 1) in one side and purely
virtual in the other side.

Feel free to send a patch ;-)

Feel free to find someone else acking your patch ;).

For my part I just wanted to add the two bits I need, without
introducing any fragile assumptions and code changes about which fields
are used exclusively with others and which not.
I figured that I can just be a *user* of this existing interface,
extending it as needed, but not tinkering too much. Which was what
Stefano asked for.
To that extent one could argue to just reuse the existing GICv3 LR
format directly, which should be a superset of the GICv2 format. Not
sure that this is useful, though.

While I agree that interface should not be changed too much, this new change just does not make sense. It makes more complicate for a reader (even after having read the spec...) to understand why some fields are only set in certain circumstance. So from my side, this is a Nack.

For clarity:

Nacked-by: Julien Grall <julien.grall@xxxxxxx>

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

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