|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 03/24] ARM: GICv3 ITS: allocate device and collection table
On Thu, 10 Nov 2016, Andre Przywara wrote:
> Hi,
>
> On 26/10/16 23:57, Stefano Stabellini wrote:
> > On Wed, 28 Sep 2016, Andre Przywara wrote:
> >> Each ITS maps a pair of a DeviceID (usually the 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 the ITS.
> >> We limit the number of devices to cover 4 PCI busses for now.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >> ---
> >> xen/arch/arm/gic-its.c | 114
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> xen/arch/arm/gic-v3.c | 5 ++
> >> xen/include/asm-arm/gic-its.h | 49 +++++++++++++++++-
> >> 3 files changed, 167 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
> >> index b52dff3..40238a2 100644
> >> --- a/xen/arch/arm/gic-its.c
> >> +++ b/xen/arch/arm/gic-its.c
> >> @@ -21,6 +21,7 @@
> >> #include <xen/device_tree.h>
> >> #include <xen/libfdt/libfdt.h>
> >> #include <asm/p2m.h>
> >> +#include <asm/io.h>
> >> #include <asm/gic.h>
> >> #include <asm/gic_v3_defs.h>
> >> #include <asm/gic-its.h>
> >> @@ -38,6 +39,119 @@ static DEFINE_PER_CPU(void *, pending_table);
> >> min_t(unsigned int, lpi_data.host_lpi_bits, CONFIG_HOST_LPI_BITS)
> >> #define MAX_HOST_LPIS (BIT(MAX_HOST_LPI_BITS) - 8192)
> >>
> >> +#define BASER_ATTR_MASK \
> >> + ((0x3UL << GITS_BASER_SHAREABILITY_SHIFT) | \
> >> + (0x7UL << GITS_BASER_OUTER_CACHEABILITY_SHIFT) | \
> >> + (0x7UL << GITS_BASER_INNER_CACHEABILITY_SHIFT))
> >> +#define BASER_RO_MASK (GENMASK(52, 48) | GENMASK(58, 56))
> >> +
> >> +static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
> >> +{
> >> + uint64_t ret;
> >> +
> >> + if ( page_bits < 16)
> >> + return (uint64_t)addr & GENMASK(47, page_bits);
> >> +
> >> + ret = addr & GENMASK(47, 16);
> >> + return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
> >> +}
> >> +
> >> +static int gicv3_map_baser(void __iomem *basereg, uint64_t regc, int
> >> nr_items)
> >
> > Shouldn't this be called its_map_baser?
>
> Yes, the BASER registers are an ITS property.
>
> >> +{
> >> + uint64_t attr;
> >> + int entry_size = (regc >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f;
> >
> > The spec says "This field is read-only and specifies the number of
> > bytes per entry, minus one." Do we need to increment it by 1?
>
> Mmh, looks so. I guess it worked because the number gets dwarfed by the
> page size round up below.
>
> >> + int pagesz;
> >> + int order;
> >> + void *buffer = NULL;
> >> +
> >> + 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;
> >> +
> >> + /*
> >> + * Loop over the page sizes (4K, 16K, 64K) to find out what the host
> >> + * supports.
> >> + */
> >
> > Is this really the best way to do it? Can't we assume ITS supports 4K,
> > given that Xen requires 4K pages at the moment?
>
> The ITS pages are totally independent from the core's MMU page size.
> So the spec says: "If the GIC implementation supports only a single,
> fixed page size, this field might be RO."
> I take it that this means that the only implemented page size could be
> 64K, for instance. And in fact the KVM ITS emulation advertises exactly
> this to a guest.
>
> > Is it actually possible
> > to find hardware that supports 4K but with an ITS that only support 64K
> > or 16K pages? It seems insane to me. Otherwise can't we probe the page
> > size somehow?
>
> We can probe by writing and seeing if it sticks - that's what the code
> does. Is it really so horrible? I agree it's nasty, but isn't it
> basically a loop around the code needed anyway?
It looks very strange that there isn't a better way to find that info.
It looks a bit like an hack. It is also bad from a software point of
view being forced to cope with all three possible page granularities.
But oh well, sometimes we just have to deal with whatever the hardware
offers us.
> Yes to the rest of the comments.
>
>
> >> + for (pagesz = 0; pagesz < 3; pagesz++)
> >> + {
> >> + uint64_t reg;
> >> + int nr_bytes;
> >> +
> >> + nr_bytes = ROUNDUP(nr_items * entry_size, BIT(pagesz * 2 + 12));
> >> + order = get_order_from_bytes(nr_bytes);
> >> +
> >> + if ( !buffer )
> >> + buffer = alloc_xenheap_pages(order, 0);
> >> + if ( !buffer )
> >> + return -ENOMEM;
> >> +
> >> + reg = attr;
> >> + reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
> >> + reg |= nr_bytes >> (pagesz * 2 + 12);
> >> + reg |= regc & BASER_RO_MASK;
> >> + reg |= GITS_BASER_VALID;
> >> + reg |= encode_phys_addr(virt_to_maddr(buffer), pagesz * 2 + 12);
> >> +
> >> + writeq_relaxed(reg, basereg);
> >> + regc = readl_relaxed(basereg);
> >> +
> >> + /* The host didn't like our attributes, just use what it
> >> returned. */
> >> + if ( (regc & BASER_ATTR_MASK) != attr )
> >> + attr = regc & BASER_ATTR_MASK;
> >> +
> >> + /* If the host accepted our page size, we are done. */
> >> + if ( (reg & (3UL << GITS_BASER_PAGE_SIZE_SHIFT)) == pagesz )
> >> + return 0;
> >> +
> >> + /* Check whether our buffer is aligned to the next page size
> >> already. */
> >> + if ( !(virt_to_maddr(buffer) & (BIT(pagesz * 2 + 12 + 2) - 1)) )
> >> + {
> >> + free_xenheap_pages(buffer, order);
> >> + buffer = NULL;
> >> + }
> >> + }
> >> +
> >> + if ( buffer )
> >> + free_xenheap_pages(buffer, order);
> >> +
> >> + return -EINVAL;
> >> +}
> >> +
> >> +int gicv3_its_init(struct host_its *hw_its)
> >> +{
> >> + uint64_t reg;
> >> + int i;
> >> +
> >> + hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
> >> + if ( !hw_its->its_base )
> >> + return -ENOMEM;
> >> +
> >> + for (i = 0; i < 8; i++)
> >
> > Code style. Unfortunately we don't have a script to check, but please
> > refer to CODING_STYLE. I'd prefer if every number was #define'ed,
> > including `8' (something like GITS_BASER_MAX).
> >
> >
> >> + {
> >> + void __iomem *basereg = hw_its->its_base + GITS_BASER0 + i * 8;
> >> + int type;
> >> +
> >> + reg = readq_relaxed(basereg);
> >> + type = (reg >> 56) & 0x7;
> >
> > Please #define 56 and 0x7
> >
> >
> >> + switch ( type )
> >> + {
> >> + case GITS_BASER_TYPE_NONE:
> >> + continue;
> >> + case GITS_BASER_TYPE_DEVICE:
> >> + /* TODO: find some better way of limiting the number of
> >> devices */
> >> + gicv3_map_baser(basereg, reg, 1024);
> >
> > An hardcoded max value might be OK, but please #define it.
> >
> >
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |