| Message ID | 20180305160415.16760-22-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, Andre Przywara wrote: > 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 as > well, packing it on the way to avoid it to grow. Not sure if it matter that much as you will always allocate gic_lr on the stack. > Then extract and assemble those bits from/to an LR. > > This allows the new VGIC to use this information. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - new patch > > xen/arch/arm/gic-v2.c | 7 +++++++ > xen/include/asm-arm/gic.h | 8 +++++--- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 031be920cc..c5ec0d4d35 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -470,6 +470,9 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg) > lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK; > lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK; > lr_reg->grp = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK; > + lr_reg->eoi = !!(lrv & GICH_V2_LR_MAINTENANCE_IRQ); If you turn eoi to bool eoi:1, you can drop !!. > + if ( lr_reg->virq < NR_GIC_SGI ) > + lr_reg->source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK; > } > > static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) > @@ -485,6 +488,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) > ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK) > << GICH_V2_LR_HW_SHIFT) | > ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) ); > + if ( lr_reg->eoi ) > + lrv |= GICH_V2_LR_MAINTENANCE_IRQ; > + if ( lr_reg->virq < NR_GIC_SGI ) > + lrv |= (uint32_t)lr_reg->source << GICH_V2_LR_CPUID_SHIFT; > > writel_gich(lrv, GICH_LR + lr * 4); > } > 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. That would also help to understand the purpose of each field without looking at the write_lr callback details. Also, please mention that source is GICv2 only. Cheers,
Hi, On 06/03/18 16:06, Julien Grall wrote: > Hi Andre, > > On 05/03/18 16:03, Andre Przywara wrote: >> 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 as >> well, packing it on the way to avoid it to grow. > > Not sure if it matter that much as you will always allocate gic_lr on > the stack. Well, someone was complaining the other day about memory usage, so I figured I shouldn't grow this structure needlessly. Using the bitfields was a very low hanging fruit. > >> Then extract and assemble those bits from/to an LR. >> >> This allows the new VGIC to use this information. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> Changelog RFC ... v1: >> - new patch >> >> xen/arch/arm/gic-v2.c | 7 +++++++ >> xen/include/asm-arm/gic.h | 8 +++++--- >> 2 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c >> index 031be920cc..c5ec0d4d35 100644 >> --- a/xen/arch/arm/gic-v2.c >> +++ b/xen/arch/arm/gic-v2.c >> @@ -470,6 +470,9 @@ static void gicv2_read_lr(int lr, struct gic_lr >> *lr_reg) >> lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & >> GICH_V2_LR_STATE_MASK; >> lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & >> GICH_V2_LR_HW_MASK; >> lr_reg->grp = (lrv >> GICH_V2_LR_GRP_SHIFT) & >> GICH_V2_LR_GRP_MASK; >> + lr_reg->eoi = !!(lrv & GICH_V2_LR_MAINTENANCE_IRQ); > > If you turn eoi to bool eoi:1, you can drop !!. > >> + if ( lr_reg->virq < NR_GIC_SGI ) >> + lr_reg->source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & >> GICH_V2_LR_CPUID_MASK; >> } >> static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) >> @@ -485,6 +488,10 @@ static void gicv2_write_lr(int lr, const struct >> gic_lr *lr_reg) >> ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK) >> << GICH_V2_LR_HW_SHIFT) | >> ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << >> GICH_V2_LR_GRP_SHIFT) ); >> + if ( lr_reg->eoi ) >> + lrv |= GICH_V2_LR_MAINTENANCE_IRQ; >> + if ( lr_reg->virq < NR_GIC_SGI ) >> + lrv |= (uint32_t)lr_reg->source << GICH_V2_LR_CPUID_SHIFT; >> writel_gich(lrv, GICH_LR + lr * 4); >> } >> 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 ;-) 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. > That would also help to understand the purpose of each field without > looking at the write_lr callback details. > Also, please mention that source is GICv2 only. I am not sure the purpose of the Xen code is to educate people about how the GIC works. Since you seem like to spec references, I can add a comment to that regard, if you are OK with that. Cheers, Andre.
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. > >> That would also help to understand the purpose of each field without >> looking at the write_lr callback details. > >> Also, please mention that source is GICv2 only. > > I am not sure the purpose of the Xen code is to educate people about how > the GIC works. Since you seem like to spec references, I can add a > comment to that regard, if you are OK with that. Well, it does not mean you need to have dry code... If you write GICv2 only the user will have some tips where to look in the spec. I don't even ask a specific section. Cheers,
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@arm.com> Cheers,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 031be920cc..c5ec0d4d35 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -470,6 +470,9 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg) lr_reg->state = (lrv >> GICH_V2_LR_STATE_SHIFT) & GICH_V2_LR_STATE_MASK; lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK; lr_reg->grp = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK; + lr_reg->eoi = !!(lrv & GICH_V2_LR_MAINTENANCE_IRQ); + if ( lr_reg->virq < NR_GIC_SGI ) + lr_reg->source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & GICH_V2_LR_CPUID_MASK; } static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) @@ -485,6 +488,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg) ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK) << GICH_V2_LR_HW_SHIFT) | ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << GICH_V2_LR_GRP_SHIFT) ); + if ( lr_reg->eoi ) + lrv |= GICH_V2_LR_MAINTENANCE_IRQ; + if ( lr_reg->virq < NR_GIC_SGI ) + lrv |= (uint32_t)lr_reg->source << GICH_V2_LR_CPUID_SHIFT; writel_gich(lrv, GICH_LR + lr * 4); } 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; }; enum gic_version {
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 as well, packing it on the way to avoid it to grow. Then extract and assemble those bits from/to an LR. This allows the new VGIC to use this information. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - new patch xen/arch/arm/gic-v2.c | 7 +++++++ xen/include/asm-arm/gic.h | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-)