Re: [PATCH v5 10/22] xen/arm: ITS: Add GITS registers emulation

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

On 31/07/15 08:25, Vijay Kilari wrote:
> On Wed, Jul 29, 2015 at 12:31 AM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Vijay,
>>
>> On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> Emulate GITS* registers
>>>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>> ---
>>> v4: - Removed GICR register emulation
>>> ---
>>>  xen/arch/arm/irq.c            |    3 +
>>>  xen/arch/arm/vgic-v3-its.c    |  365 ++++++++++++++++++++++++++++++++++++++++-
>>>  xen/include/asm-arm/gic-its.h |   15 ++
>>>  xen/include/asm-arm/gic.h     |    1 +
>>>  4 files changed, 381 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>> index 1f38605..85cacb0 100644
>>> --- a/xen/arch/arm/irq.c
>>> +++ b/xen/arch/arm/irq.c
>>> @@ -31,6 +31,9 @@
>>>  static unsigned int local_irqs_type[NR_LOCAL_IRQS];
>>>  static DEFINE_SPINLOCK(local_irqs_type_lock);
>>>
>>> +/* Number of LPI supported in XEN */
>>> +unsigned int num_of_lpis = 8192;
>>> +
>>
>> It makes little sense to introduce the support of LPIs in Xen in a patch
>> called "Add GITS registers emulation".
>>
>> This should go in a specific ITS (not vITS) patch.
>>
>> Furthermore, you need to explain where to the 8192 comes from...
> 
> Two reasons for setting to 8192
> 
> 1) Being LPI starts from 8192 the 8192 is 13 and next if id_bits is 14
> then it can hold
>     upto 16K. So 16K-8K = 8K

This is a valid reason. Please add it in the code.

[..]

>>> +static int vgic_v3_gits_mmio_read(struct vcpu *v, mmio_info_t *info)
>>> +{
>>> +    struct vgic_its *vits = v->domain->arch.vgic.vits;
>>> +    struct hsr_dabt dabt = info->dabt;
>>> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>>> +    register_t *r = select_user_reg(regs, dabt.reg);
>>> +    uint64_t val = 0;
>>> +    uint32_t gits_reg;
>>> +
>>> +    gits_reg = info->gpa - vits->gits_base;
>>> +    DPRINTK("%pv: vITS: GITS_MMIO_READ offset 0x%"PRIx32"\n", v, gits_reg);
>>> +
>>> +    switch ( gits_reg )
>>> +    {
>>> +    case GITS_CTLR:
>>> +        if ( dabt.size != DABT_WORD ) goto bad_width;
>>> +        vits_spin_lock(vits);
>>> +        *r = vits->ctrl | GITS_CTLR_QUIESCENT;
>>
>> Why did you put GITS_CTLR_QUIESCENT?
> 
> The ITS is quiescent, has no translations in progress and has
> completed all operations.
> So I have set quescent by default.

This needs to be explained in the code.

[..]

>>> + * Mask those fields while emulating GITS_BASER reg.
>>> + */
>>
>> As said on v4,
>>
>> Other fields are (or could be RO) in GITS_BASER:
>>     - Indirect: we only support flat table
>         By default it is 0. So support flat table. Do you want it explicitly
>        specify Indirect?

It's not what I said ... Your implementation in the VITS *only* supports
flat-table (i.e Single Level). You are currently allowing the guest to
set this bit to 1 which means that it may use Two Level.

This won't work with your implementation. So this field *should* be RAZ/WI.

> 
>>     - Page_Size: it's fine to only support 4KB granularity. It also
>> means less code.
>         Page_size is set by guest. this is not RO

Please read the spec (8.19.1 in ARM IHI 0069A): "If the GIC
implementation supports only a single, fixed page size, this field might
be RO."

If you look closely to the Linux code, if it can't set the Page size it
will retry with a small granularity.

Implementing it as RO would have been fine and save a bit of checking.
Anyway, as said this was only a suggestion.

[..]

>>>  int vits_domain_init(struct domain *d)
>>>  {
>>>      struct vgic_its *vits;
>>>      int i;
>>>
>>> +    if ( is_hardware_domain(d) )
>>> +        d->arch.vgic.nr_lpis = num_of_lpis;
>>> +    else
>>> +        d->arch.vgic.nr_lpis = NR_LPIS;
>>
>> NR_LPIS is defined in patch #14. And the name seems to be wrong.
>>
>> Anyway, I don't understand why you are trying to initialize vITS on
>> guest. We agree that it should only be used on DOM0 for now until we
>> effectively need it for the guest.
>>
>> Furthermore, it miss at least the toolstack in order to get the part
>> guest ready.
>>
>> So please ensure that the vITS is not initialized for the guest.
> 
> In patch#18, this function is called only for DOM0

So why do try to half support vITS for guest? As said, there is lots of
things missing to get the support done for guest (toolstack, ITS
mapping,...).

Please drop all the references to the guest in vits_domain_init and add
an ASSERT to check that vits_init_domain is not called for guest.

Regards,

-- 
Julien Grall