Re: [PATCH 06/28] ARM: GICv3 ITS: introduce ITS command handling

From: Julien Grall <julien.grall@arm.com>
To: Andre Przywara <andre.przywara@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org, Vijay Kilari <vijay.kilari@gmail.com>
Subject: Re: [PATCH 06/28] ARM: GICv3 ITS: introduce ITS command handling
Date: Wed, 8 Mar 2017 15:28:43 +0000	[thread overview]
Message-ID: <63676939-0417-5e89-e59c-624a36d1d62c@arm.com> (raw)
In-Reply-To: <f0cfb7aa-8b8e-ca33-5270-81be98a730c7@arm.com>

On 07/03/17 18:08, Andre Przywara wrote:
> Hi Julien,

Hi Andre,

>
> On 06/02/17 19:16, Julien Grall wrote:
>> Hi Andre,
>>
>> On 30/01/17 18:31, Andre Przywara wrote:
>>> To be able to easily send commands to the ITS, create the respective
>>> wrapper functions, which take care of the ring buffer.
>>> The first two commands we implement provide methods to map a collection
>>> to a redistributor (aka host core) and to flush the command queue (SYNC).
>>> Start using these commands for mapping one collection to each host CPU.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  xen/arch/arm/gic-v3-its.c         | 142
>>> +++++++++++++++++++++++++++++++++++++-
>>>  xen/arch/arm/gic-v3-lpi.c         |  20 ++++++
>>>  xen/arch/arm/gic-v3.c             |  18 ++++-
>>>  xen/include/asm-arm/gic_v3_defs.h |   2 +
>>>  xen/include/asm-arm/gic_v3_its.h  |  36 ++++++++++
>>>  5 files changed, 215 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
>>> index ad7cd2a..6578e8a 100644
>>> --- a/xen/arch/arm/gic-v3-its.c
>>> +++ b/xen/arch/arm/gic-v3-its.c
>>> @@ -19,6 +19,7 @@
>>>  #include <xen/config.h>
>>>  #include <xen/lib.h>
>>>  #include <xen/device_tree.h>
>>> +#include <xen/delay.h>
>>>  #include <xen/libfdt/libfdt.h>
>>>  #include <xen/mm.h>
>>>  #include <xen/sizes.h>
>>> @@ -29,6 +30,98 @@
>>>
>>>  #define ITS_CMD_QUEUE_SZ                SZ_64K
>>>
>>> +#define BUFPTR_MASK                     GENMASK(19, 5)
>>> +static int its_send_command(struct host_its *hw_its, const void
>>> *its_cmd)
>>> +{
>>> +    uint64_t readp, writep;
>>> +
>>> +    spin_lock(&hw_its->cmd_lock);
>>
>> Do you never expect a command to be sent in an interrupt path? I could
>> see at least one, we may decide to throttle the number of LPIs received
>> by a guest so this would involve disabling the interrupt.
>
> I take it you are asking for spin_lock_irq[save]()?

Yes.

> I don't think queuing ITS commands in interrupt context is a good idea,
> especially since I just introduced a grace period to wait for a draining
> command queue.
> I am happy to revisit this when needed.

As mentioned on the previous mail, we might need to send a command 
whilst in the interrupt context if we need to disable an interrupt that 
fire too often.

I would be fine to have an ASSERT(!in_irq()) and a comment explaining 
why for the time being.

>
>>> +
>>> +    readp = readq_relaxed(hw_its->its_base + GITS_CREADR) & BUFPTR_MASK;
>>> +    writep = readq_relaxed(hw_its->its_base + GITS_CWRITER) &
>>> BUFPTR_MASK;
>>> +
>>> +    if ( ((writep + ITS_CMD_SIZE) % ITS_CMD_QUEUE_SZ) == readp )
>>> +    {
>>
>> I look at all the series applied and there is no error message at all
>> when the queue is full. This will make difficult to see what's going on.
>>
>> Furthermore, this limit could be easily reached. Furthermore, this could
>> happen easily if you decide to map a device with thousands of
>> interrupts. For instance the function gicv3_map_its_map_host_events will
>> issue 2 commands per event (MAPTI and INV).
>>
>> So how do you plan to address this?
>
> So I changed this now to wait for 1 ms (or whatever value you prefer) in
> hope the command queue drains. In the end the ITS is hardware, so
> processing commands it's the only thing it does and I don't expect it to
> be seriously stalled, usually. So waiting a tiny bit to cover this odd
> case of command queue contention seems useful to me, especially since we
> only send commands from non-critical Dom0 code.

I don't have any idea of a good value. My worry with such value is you 
are only hoping it will never happen. If you fail here, what will you 
do? You will likely have to revert changes which mean more command and 
then? If it fails once, why would it not fail again? You will end up in 
a spiral loop.

Regarding the value, is it something we could confirm with the hardware 
guys?

> The command queue is now 1 MB in size, so we have 32,768 commands in
> there. Should be enough for everybody ;-)
>
>>> +        spin_unlock(&hw_its->cmd_lock);
>>> +        return -EBUSY;
>>> +    }
>>> +
>>> +    memcpy(hw_its->cmd_buf + writep, its_cmd, ITS_CMD_SIZE);
>>> +    if ( hw_its->flags & HOST_ITS_FLUSH_CMD_QUEUE )
>>> +        __flush_dcache_area(hw_its->cmd_buf + writep, ITS_CMD_SIZE);
>>
>> Please use dcache_.... helpers.
>>
>>> +    else
>>> +        dsb(ishst);
>>> +
>>> +    writep = (writep + ITS_CMD_SIZE) % ITS_CMD_QUEUE_SZ;
>>> +    writeq_relaxed(writep & BUFPTR_MASK, hw_its->its_base +
>>> GITS_CWRITER);
>>> +
>>> +    spin_unlock(&hw_its->cmd_lock);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static uint64_t encode_rdbase(struct host_its *hw_its, int cpu,
>>> uint64_t reg)
>>
>> s/int cpu/unsigned int cpu/
>
> So it's easy to do so, but why is that actually?

Because a CPU and a vCPU ID cannot be signed. So what's the point to 
make them signed except saving 9 characters?

> I see that both "processor" and "vcpu_id" are "int" in struct vcpu, so I
> was using int as the type for CPUs here as well.

[...]

>>> +{
>>> +    struct host_its *its;
>>> +    int ret;
>>> +
>>> +    list_for_each_entry(its, &host_its_list, entry)
>>> +    {
>>> +        /*
>>> +         * This function is called on CPU0 before any ITSes have been
>>> +         * properly initialized. Skip the collection setup in this case,
>>> +         * it will be done explicitly for CPU0 upon initializing the
>>> ITS.
>>> +         */
>>
>> Looking at the code, I don't understand why you need to do that. AFAIU
>> there are no restriction to initialize the ITS (e.g call gicv3_its_init)
>> before gicv3_cpu_init.
>
> Well, it's a bit more subtle: For initialising the ITS (the collection
> table entry, more specifically), we need to know the "rdbase", so either
> the physical address or the logical ID. Those we determine only
> somewhere deep in gicv3_cpu_init().
> So just moving gicv3_its_init() before gicv3_cpu_init() does not work. I
> will try and see if it's worth to split gicv3_its_init() into a generic
> and a per-CPU part, though I doubt that this is helpful.

Looking at the gicv3_its_init function, the only code requiring "rdbase" 
is mapping the collection for the CPU0. The rest is CPU agnostic and 
should only populate the data structure.

So this does not answer why you need to wait until CPU0 is initialized 
to populate those table. The following path would be fine
	gicv3_its_init();
		-> Populate BASER
	gicv3_cpu_init();
		-> gicv3_its_setup_collection()
			-> Initialize collection for CPU0

Cheers,

-- 
Julien Grall

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