Re: [Xen-devel] [RFC PATCH 02/24] ARM: GICv3: allocate LPI pending and property table

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 15/11/16 11:32, Andre Przywara wrote:
> On 01/11/16 17:22, Julien Grall wrote:
> > On 28/09/2016 19:24, Andre Przywara wrote:
> > > The ARM GICv3 ITS provides a new kind of interrupt called LPIs.
> > > The pending bits and the configuration data (priority, enable bits) for
> > > those LPIs are stored in tables in normal memory, which software has to
> > > provide to the hardware.
> > > Allocate the required memory, initialize it and hand it over to each
> > > ITS. We limit the number of LPIs we use with a compile time constant to
> > > avoid wasting memory.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > ---
> > >  xen/arch/arm/Kconfig              |  6 ++++
> > >  xen/arch/arm/efi/efi-boot.h       |  1 -
> > >  xen/arch/arm/gic-its.c            | 76
> > > +++++++++++++++++++++++++++++++++++++++
> > >  xen/arch/arm/gic-v3.c             | 27 ++++++++++++++
> > >  xen/include/asm-arm/cache.h       |  4 +++
> > >  xen/include/asm-arm/gic-its.h     | 22 +++++++++++-
> > >  xen/include/asm-arm/gic_v3_defs.h | 48 ++++++++++++++++++++++++-
> > >  7 files changed, 181 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > index 9fe3b8e..66e2bb8 100644
> > > --- a/xen/arch/arm/Kconfig
> > > +++ b/xen/arch/arm/Kconfig
> > > @@ -50,6 +50,12 @@ config HAS_ITS
> > >          depends on ARM_64
> > >          depends on HAS_GICV3
> > >
> > > +config HOST_LPI_BITS
> > > +        depends on HAS_ITS
> > > +        int "Maximum bits for GICv3 host LPIs (14-32)"
> > > +        range 14 32
> > > +        default "20"
> > > +
> > This would be better to be defined as a parameter command line. So the
> > user does not need to rebuild Xen in order to increase the number of
> > bits supported. It would also be useful to get a rational behind the
> > default number in the commit message.
> Yeah, a command line option sounds useful, though I have to check
> whether this changes compile-time computation into actual runtime one.
>
> The number is made-up, based on some reasoning on the possible memory
> consumption and the number of LPIs provided. 8 MB and 1 million LPIs
> sounded like a sweet spot.
> If I got this correctly, Linux atm doesn't use more than 65536 LPIs.
You know my answer here ;). Linux is not the only guests OS supports on 
Xen, so we should avoid making assumptions on the best behavior based on 
this.
[...]

> > > +
> > > +    /*
> > > +     * The pending table holds one bit per LPI, so we need three bits
> > > less
> > > +     * than the number of LPI_BITs. But the alignment requirement
> > > from the
> > > +     * ITS is 64K, so make order at least 16 (-12).
> > > +     */
> > > +    pendtable = alloc_xenheap_pages(MAX(lpi_data.host_lpi_bits - 3,
> > > 16) - 12, 0);
> > > +    if ( !pendtable )
> > > +        return 0;
> > > +
> > > +    memset(pendtable, 0, BIT(lpi_data.host_lpi_bits - 3));
> > Same remark for BIT here.
> >
> > > +    this_cpu(pending_table) = pendtable;
> > > +
> > > +    reg  = attr | GICR_PENDBASER_PTZ;
> > > +    reg |= virt_to_maddr(pendtable) & GENMASK(51, 16);
> > I don't think the mask is useful and would need to be changed if the
> > physical address bits increased as it was done in ARMv8.2.
> Mmmh, not so sure we can extend the mask to cover the region that it
> RES0 at the moment. We need some mask anyway (to not clobber the upper
> bits), so I figured we just use what the spec says.
The masked PA should always be equal to the PA. Otherwise we would 
program the wrong address into the register and who knows what can 
happen. So this mask is pointless.
[..]

> > >  #endif /* CONFIG_HAS_ITS */
> > >
> > >  #endif /* __ASSEMBLY__ */
> > > diff --git a/xen/include/asm-arm/gic_v3_defs.h
> > > b/xen/include/asm-arm/gic_v3_defs.h
> > > index 6bd25a5..da5fb77 100644
> > > --- a/xen/include/asm-arm/gic_v3_defs.h
> > > +++ b/xen/include/asm-arm/gic_v3_defs.h
> > > @@ -44,7 +44,8 @@
> > >  #define GICC_SRE_EL2_ENEL1           (1UL << 3)
> > >
> > >  /* Additional bits in GICD_TYPER defined by GICv3 */
> > > -#define GICD_TYPE_ID_BITS_SHIFT 19
> > > +#define GICD_TYPE_ID_BITS_SHIFT      19
> > > +#define GICD_TYPE_LPIS               (1U << 17)
> > I was about to say that this should be named GICD_TYPER... but it looks
> > like we already defined and use GIC_TYPE_ID_BITS_SHIFTS. So it is up to
> > you if you rename it to get the correct register name.
> Yeah, I was unsure about this as well. My hunch is we should avoid the
> churn and keep existing names around. Experience shows that those simple
> renames tend to introduce nasty rebase issues, especially with a
> long-standing series like this.
Clean up are always welcomed ;). I am fine if it comes after.

Regards,

--
Julien Grall

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