Re: [Xen-devel] [PATCH v2 03/27] ARM: GICv3 ITS: allocate device and collection table

[Julien Grall <julien.grall@xxxxxxx>]

On 22/03/17 16:08, André Przywara wrote:
> Hi,
Hi Andre,

> On 22/03/17 13:52, Julien Grall wrote:
> > Hi Andre,
> >
> > On 03/16/2017 11:20 AM, Andre Przywara wrote:
> > > Each ITS maps a pair of a DeviceID (for instance derived from a PCI
> > > b/d/f triplet) and an EventID (the MSI payload or interrupt ID) to a
> > > pair of LPI number and collection ID, which points to the target CPU.
> > > This mapping is stored in the device and collection tables, which
> > > software
> > > has to provide for the ITS to use.
> > > Allocate the required memory and hand it to the ITS.
> > > The maximum number of devices is limited to a compile-time constant
> > > exposed in Kconfig.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > ---
> > >  docs/misc/xen-command-line.markdown |   8 ++
> > >  xen/arch/arm/Kconfig                |  14 ++++
> > >  xen/arch/arm/gic-v3-its.c           | 163
> > > ++++++++++++++++++++++++++++++++++++
> > >  xen/arch/arm/gic-v3.c               |   3 +
> > >  xen/include/asm-arm/gic_v3_its.h    |  63 +++++++++++++-
> > >  5 files changed, 250 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/docs/misc/xen-command-line.markdown
> > > b/docs/misc/xen-command-line.markdown
> > > index 619016d..068d116 100644
> > > --- a/docs/misc/xen-command-line.markdown
> > > +++ b/docs/misc/xen-command-line.markdown
> > > @@ -1158,6 +1158,14 @@ based interrupts. Any higher IRQs will be
> > > available for use via PCI MSI.
> > >  ### maxcpus
> > >  > `= <integer>`
> > >
> > > +### max\_its\_device\_bits
> > > +> `= <integer>`
> > > +
> > > +Specifies the maximum number of devices using MSIs on the ARM GICv3 ITS
> > > +controller to allocate table entries for. Each table entry uses a
> > > hardware
> > > +specific size, typically 8 or 16 bytes.
> > > +Defaults to 10 bits (to cover at most 1024 devices).
> > The description is misleading. Even if the platform has less than 1024
> > devices, 10 may not be enough if the Device ID are not contiguous.
> Right.
>
> > However, I don't think this is useful. A user may not know the DevIDs in
> > use of the platform and hence will not be able to choose a sensible value.
> >
> > I still think that we should allocate what the hardware asked us and if
> > it is necessary to limit this should be done per-platform.
> I think you are right, a configurable limit does not make much sense. I
> was wondering if we should have a hard coded *memory* limit instead, to
> prevent ridiculously high allocations, say entry_size=8 and 32 bits of
> device IDs, which would result in 32GB of memory for a flat device table.
>
> Or if we don't want to arbitrarily limit this, we always print the
> actual allocated size, maybe with a extra WARNING if it exceeds sane levels.
But what is a sane level? To be fair, I don't think this is the business 
of the common code to care about high allocations. If the platform ask 
32bits and not able to cope, then it should be a workaround for the 
platform.
[...]

> > > +    void *buffer;
> > > +
> > > +    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
> > > +    attr |= GIC_BASER_CACHE_SameAsInner <<
> > > GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> > > +    attr |= GIC_BASER_CACHE_RaWaWb <<
> > > GITS_BASER_INNER_CACHEABILITY_SHIFT;
> > > +
> > > +    /*
> > > +     * Setup the BASE register with the attributes that we like. Then
> > > read
> > > +     * it back and see what sticks (page size, cacheability and
> > > shareability
> > > +     * attributes), retrying if necessary.
> > > +     */
> > > +retry:
> > > +    table_size = ROUNDUP(nr_items * entry_size,
> > > BIT(BASER_PAGE_BITS(pagesz)));
> > > +    /* The BASE registers support at most 256 pages. */
> > > +    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
> > > +    /* The memory block must be aligned to the requested page size. */
> > > +    order = max(get_order_from_bytes(table_size), pagesz * 2);
> > > +
> > > +    buffer = alloc_xenheap_pages(order, 0);
> > Why don't you use _zalloc(...)? This would avoid to compute the order
> > and drop few lines.
> Because they need to be physically contiguous.
> Please correct me if I am wrong here, but the normal *alloc functions do
> not guarantee this (from checking the implementation).
_x*alloc will work the same way as k*alloc in Linux. The memory will be 
contiguous. Regarding the implementation details, _x*alloc is an overlay 
of alloc_xenheap_pages. For small size (i.e < PAGE_SIZE), it will 
coalesce in the same page. For bigger size, alloc_xenheap_pages will be 
used. Although, the resulting memory usage will differ because _x*alloc 
will free unused pages.
This is because alloc_xenheap_pages is working in term of order. So if 
you request 12K, alloc_xenheap_pages will allocate 16K. The 
implementation of _x*alloc will free the last 4K to avoid wasting space.
So effectively, _x*alloc will result in lower memory usage.

> > >      gicv3_dist_init();
> > > +    res = gicv3_its_init();
> > > +    if ( res )
> > > +        printk(XENLOG_WARNING "GICv3: ITS: initialization failed:
> > > %d\n", res);
> > I would have expect a panic here because the ITS subsystem could be half
> > initialized and it is not safe to continue.
> OK, let me check what actually happens here if there is no ITS ;-)
Technically, this message should not happen when there is no ITS because 
it is not mandatory to have one on the platform.
So this would be an coding error for me.

Cheers,

--
Julien Grall

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