Re: [Xen-devel] [PATCH v2 04/27] ARM: GICv3 ITS: map ITS command buffer

[André Przywara <andre.przywara@xxxxxxx>]

On 22/03/17 15:23, Julien Grall wrote:
> Hi Andre,
> 
> On 16/03/17 11:20, Andre Przywara wrote:
>> Instead of directly manipulating the tables in memory, an ITS driver
>> sends commands via a ring buffer in normal system memory to the ITS h/w
>> to create or alter the LPI mappings.
>> Allocate memory for that buffer and tell the ITS about it to be able
>> to send ITS commands.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/gic-v3-its.c        | 57
>> ++++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/gic_v3_its.h |  6 +++++
>>  2 files changed, 63 insertions(+)
>>
>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>> index 9982fe9..e5601ed 100644
>> --- a/xen/arch/arm/gic-v3-its.c
>> +++ b/xen/arch/arm/gic-v3-its.c
>> @@ -20,10 +20,13 @@
>>
>>  #include <xen/lib.h>
>>  #include <xen/mm.h>
>> +#include <xen/sizes.h>
>>  #include <asm/gic_v3_defs.h>
>>  #include <asm/gic_v3_its.h>
>>  #include <asm/io.h>
>>
>> +#define ITS_CMD_QUEUE_SZ                SZ_64K
> 
> I thought you were planning to increase the size to 1MB?

Good, you noticed ;-) Indeed forgot to change it ...

>> +
>>  LIST_HEAD(host_its_list);
>>
>>  bool gicv3_its_host_has_its(void)
>> @@ -56,6 +59,55 @@ static uint64_t encode_propbaser_phys_addr(paddr_t
>> addr, unsigned int page_bits)
>>      return ret | ((addr & GENMASK(51, 48)) >> (48 - 12));
>>  }
>>
>> +static void *its_map_cbaser(struct host_its *its)
>> +{
>> +    void __iomem *cbasereg = its->its_base + GITS_CBASER;
>> +    uint64_t reg;
>> +    void *buffer;
>> +    unsigned int order;
>> +
>> +    reg  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>> +    reg |= GIC_BASER_CACHE_SameAsInner <<
>> GITS_BASER_OUTER_CACHEABILITY_SHIFT;
>> +    reg |= GIC_BASER_CACHE_RaWaWb <<
>> GITS_BASER_INNER_CACHEABILITY_SHIFT;
>> +
>> +    /* The ITS command buffer needs to be 64K aligned. */
> 
> Looking at the spec, the command buffer does not need to be 64K aligned.
> On the previous version, you made it 4K aligned. So why this restriction?

As you have already learnt, the GIC is more subtle sometimes ;-) Read
the description at "Physical_Address, bits [51:12]" in the CBASER
paragraph and tell me what you take from it. I decided to read it as
"has to be 64K aligned".
Happy to correct this otherwise.

>> +    order = max(get_order_from_bytes(ITS_CMD_QUEUE_SZ), 16U -
>> PAGE_SHIFT);
>> +    buffer = alloc_xenheap_pages(order, 0);
> 
> I am not sure to understand why you move from _zalloc to
> alloc_xenheap_*. The resulting behavior will be exactly the same but the
> former result to a simpler code.
                                                  ^^^^^^^^^^^^^^^^
Really?
I checked and found that alloc_xenheap_pages gives me physically
contiguous allocation in the given order, where *alloc just guarantees
virtually contiguous pages.

> 
>> +    if ( !buffer )
>> +        return NULL;
>> +
>> +    if ( virt_to_maddr(buffer) & ~GENMASK(51, 12) )
>> +    {
>> +        free_xenheap_pages(buffer, 0);
>> +        return NULL;
>> +    }
>> +    memset(buffer, 0, ITS_CMD_QUEUE_SZ);
>> +
>> +    reg |= GITS_VALID_BIT | virt_to_maddr(buffer);
>> +    reg |= ((ITS_CMD_QUEUE_SZ / SZ_4K) - 1) & GITS_CBASER_SIZE_MASK;
>> +    writeq_relaxed(reg, cbasereg);
>> +    reg = readq_relaxed(cbasereg);
>> +
>> +    /* If the ITS dropped shareability, drop cacheability as well. */
>> +    if ( (reg & GITS_BASER_SHAREABILITY_MASK) == 0 )
>> +    {
>> +        reg &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
>> +        writeq_relaxed(reg, cbasereg);
>> +    }
>> +
>> +    /*
>> +     * If the command queue memory is mapped as uncached, we need to
>> flush
>> +     * it on every access.
>> +     */
>> +    if ( !(reg & GITS_BASER_INNER_CACHEABILITY_MASK) )
>> +    {
>> +        its->flags |= HOST_ITS_FLUSH_CMD_QUEUE;
>> +        dprintk(XENLOG_WARNING, "using non-cacheable ITS command
>> queue\n");
> 
> Please use printk, the message is useful even for non-debug build.

OK.

Cheers,
Andre.

> 
>> +    }
>> +
>> +    return buffer;
>> +}
>> +
> 
> Cheers,
> 


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