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

[Julien Grall <julien.grall@xxxxxxx>]

On 03/14/2018 06:19 PM, Andre Przywara wrote:
> Hi,
Hi Andre,

Thank you for the review.

> On 09/03/18 16:35, julien.grall@xxxxxxx wrote:
> > From: Andre Przywara <andre.przywara@xxxxxxxxxx>
> I think this is quite different from what I ever wrote, so please drop
> my authorship here.
Fine, I wasn't sure given that you were the original author or extending 
the LR. I can pointing that in the commit message :).
> > So far our LR read/write functions do not handle the EOI bit and the 
> > source CPUID bits in an LR, because the current VGIC implementation does
> > not use them.
> > Extend the gic_lr data structure to hold these bits of information by
> > using a union to differentiate field used depending on whether the vIRQ
> > has a corresponding pIRQ.
> As mentioned before, I am not sure this is really necessary. The idea of
> struct gic_lr is to provide a hardware agnostic view of an LR. So the
> actual read_lr/write_lr function take care of reading/populating pINTID,
> iff the HW bit is set (as done in your patch 5/6).
> Given that, I don't think we need to change the current code in this
> respect, as this is just a small internal interface.
Even if I know the vGIC, I find easier with this solution to 
differentiate what is for the HW IRQ or not.
The size of Xen Arm is becoming quite significant for me to remember 
every single line/structure. So the main goal here is to help the 
reviewer to spend less time on patch review as it helps to spot directly 
misusage.
> But then again I don't care enough, so if that makes you happy ....
>
> > Note that source is not covered by GICv3 LR.
> I was thinking the same, but Marc pointed me to that inconspicuous note
> on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).
Doh :/. I will drop the comment and update the GICv3 code then.

> > This allows the new VGIC to use this information.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >   xen/arch/arm/gic-v2.c     | 22 +++++++++++++++++++---
> >   xen/arch/arm/gic-v3.c     | 11 +++++++++--
> >   xen/include/asm-arm/gic.h | 16 ++++++++++++++--
> >   3 files changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index daf8c61258..69f8d6044a 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
> >   
> >       if ( lr_reg->hw_status )
> >       {
> > -        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> > -        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> > +        lr_reg->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> > +        lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
> > +    }
> > +    else
> > +    {
> > +        lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == 
> > GICH_V2_LR_MAINTENANCE_IRQ;
> I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
> bool. Avoids the linebreak.
The '==' was more to avoid assuming GIC_V2_LR_MAINTENANCE_IRQ is a 
single bit. But I was probably too cautious, I will drop it.
> >       writel_gich(lrv, GICH_LR + lr * 4);
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index f73d386df1..a855069111 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
> >       lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
> >   
> >       if ( lr_reg->hw_status )
> > -        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> > +        lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> > +    else
> > +        lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == 
> > ICH_LR_MAINTENANCE_IRQ;
> Same here.
Ditto.

> >   }
> >   
> >   static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> > @@ -1033,7 +1035,12 @@ static void gicv3_write_lr(int lr_reg, const struct 
> > gic_lr *lr)
> >       if ( lr->hw_status )
> >       {
> >           lrv |= ICH_LR_HW;
> > -        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> > +        lrv |= (uint64_t)lr->h.pirq << ICH_LR_PHYSICAL_SHIFT;
> > +    }
> > +    else
> > +    {
> > +        if ( lr->v.eoi )
> > +            lrv |= ICH_LR_MAINTENANCE_IRQ;
> >       }
> >   
> >       /*
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 545901b120..4cf5bb385d 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -204,14 +204,26 @@ union gic_state_data {
> >    * The LR register format is different for GIC HW version
> >    */
> >   struct gic_lr {
> > -   /* Physical IRQ -> Only set when hw_status is set. */
> > -   uint32_t pirq;
> >      /* Virtual IRQ */
> >      uint32_t virq;
> >      uint8_t priority;
> >      bool active;
> >      bool pending;
> >      bool hw_status;
> > +   union
> > +   {
> > +       /* Only filled when there are a corresponding pIRQ (hw_state = true) */
> > +       struct
> > +       {
> > +           uint32_t pirq;
> > +       } h;
> > +       /* Only filled when there are no corresponding pIRQ (hw_state = false) 
> > */
> > +       struct
> > +       {
> > +           bool eoi;
> > +           uint8_t source;      /* GICv2 only */
> > +       } v;
> That looks somewhat confusing to me. So either you use "hw" and "virt"
> for the struct identifiers, or, preferably, just drop them altogether:
>
> union {
>         uint32_t pirq;  /* Contains the associated hardware IRQ. */
>         struct          /* Only used for virtual interrupts. */
>         {
>                 bool eoi;
>                 uint8_t source;
>         };
> };
I would prefer to keep a name for each structure. It is not obvious for 
me that eoi is only for virtual IRQ. I mostly want to help code review 
in the future.
Cheers,

--
Julien Grall

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