Re: [Xen-devel] [PATCH 04/28] ARM: GICv3 ITS: allocate device and collection table

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi Shanker,

thanks for having a look.

On 24/02/17 19:29, Shanker Donthineni wrote:
> Hi Andre
>
>
> On 02/16/2017 01:03 PM, Shanker Donthineni wrote:
>> Hi Andre,
>>
>>
>> On 01/30/2017 12:31 PM, 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.
>>> The maximum number of devices is limited to a compile-time constant
>>> exposed in Kconfig.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>>> ---
>>>   xen/arch/arm/Kconfig             |  14 +++++
>>>   xen/arch/arm/gic-v3-its.c        | 129
>>> +++++++++++++++++++++++++++++++++++++++
>>>   xen/arch/arm/gic-v3.c            |   5 ++
>>>   xen/include/asm-arm/gic_v3_its.h |  55 ++++++++++++++++-
>>>   4 files changed, 202 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 71734a1..81bc233 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -64,6 +64,20 @@ config MAX_PHYS_LPI_BITS
>>>             This can be overriden on the command line with the
>>> max_lpi_bits
>>>             parameter.
>>>
>>> +config MAX_PHYS_ITS_DEVICE_BITS
>>> +        depends on HAS_ITS
>>> +        int "Number of device bits the ITS supports"
>>> +        range 1 32
>>> +        default "10"
>>> +        help
>>> +          Specifies the maximum number of devices which want to use the
>>> ITS.
>>> +          Xen needs to allocates memory for the whole range very early.
>>> +          The allocation scheme may be sparse, so a much larger
>>> number must
>>> +          be supported to cover devices with a high bus number or
>>> those on
>>> +          separate bus segments.
>>> +          This can be overriden on the command line with the
>>> max_its_device_bits
>>> +          parameter.
>>> +
>>>   endmenu
>>>
>>>   menu "ARM errata workaround via the alternative framework"
>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>>> index ff0f571..c31fef6 100644
>>> --- a/xen/arch/arm/gic-v3-its.c
>>> +++ b/xen/arch/arm/gic-v3-its.c
>>> @@ -20,9 +20,138 @@
>>>   #include <xen/lib.h>
>>>   #include <xen/device_tree.h>
>>>   #include <xen/libfdt/libfdt.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/sizes.h>
>>>   #include <asm/gic.h>
>>>   #include <asm/gic_v3_defs.h>
>>>   #include <asm/gic_v3_its.h>
>>> +#include <asm/io.h>
>>> +
>>> +#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(58, 56) | GENMASK(52, 48))
>>> +
>>> +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);
>>> +}
>>> +
>>> +#define PAGE_BITS(sz) ((sz) * 2 + PAGE_SHIFT)
>>> +
>>> +static int its_map_baser(void __iomem *basereg, uint64_t regc, int
>>> nr_items)
>>> +{
>>> +    uint64_t attr, reg;
>>> +    int entry_size = ((regc >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f)
>>> + 1;
>>> +    int pagesz = 0, order, table_size;
>
> Please try ITS page sizes in the order 64K, 16K and 4K to cover more ITS
> devices using a flat table. Similar to Linux ITS driver.
>
>>> +    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;
>>> +
>>> +    /*
>>> +     * 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.
>>> +     */
>>> +    while ( 1 )
>>> +    {
>>> +        table_size = ROUNDUP(nr_items * entry_size,
>>> BIT(PAGE_BITS(pagesz)));
>>> +        order = get_order_from_bytes(table_size);
>>> +
>
> Limit to 256 ITS pages, ITS spec doesn't support more than 256 ITS pages.
>
>        /* Maximum of 256 ITS pages are allowed */
>        if ( (table_size >> PAGE_BITS(pagesz)) > GITS_BASER_PAGES_MAX )
>                table_size = BIT(PAGE_BITS(pagesz)) * GITS_BASER_PAGES_MAX;
>
>>> +        if ( !buffer )
>>> +            buffer = alloc_xenheap_pages(order, 0);
>>> +        if ( !buffer )
>>> +            return -ENOMEM;
>>> +
>
> Please zero memory memset(buffer, 0x00, order << PAGE_SHIFT)

All three comments make sense, thanks for pointing these out.
I will incorporate those changes into the next post.

Cheers,
Andre.
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.

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