Re: [PATCH v5 12/22] xen/arm: ITS: Add GICR register emulation

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

On 31/07/15 10:08, Vijay Kilari wrote:
> On Thu, Jul 30, 2015 at 10:34 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> Hi Vijay,
>>
>> On 27/07/15 12:11, vijay.kilari@gmail.com wrote:
>>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>>
>> [..]
>>
>>> +static int gicv3_dist_supports_lpis(void)
>>> +{
>>> +    return readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_LPIS_SUPPORTED;
>>> +}
>>> +
>>>  static int __cpuinit gicv3_cpu_init(void)
>>>  {
>>>      int i;
>>> @@ -1274,6 +1279,11 @@ static int __init gicv3_init(void)
>>>
>>>      spin_lock(&gicv3.lock);
>>>
>>> +    if ( gicv3_dist_supports_lpis() )
>>> +        gicv3_info.lpi_supported = 1;
>>> +    else
>>> +        gicv3_info.lpi_supported = 0;
>>> +
>>
>> You will avoid 3 lines if you do:
>>
>> gicv3_info.lpi_supported = !!gicv3_dist_supports_lpis();
>>
> 
>    This will change in patch #17 where we do check for its_probe() to
> be succesful
> to set gicv3_info.lpi_supported

Then move this addition of lpi_supported in patch #17. This is not
necessary at all here. And this is quite confusing to see GIC specific
changes in vGIC code.

Overall, I think your series would benefits to be split in two parts.
First part would contains all the code for the hardware GIC/ITS driver.
The second part would be for virtual GIC/ITS changes.

>>>      gicv3_dist_init();
>>>      res = gicv3_cpu_init();
>>>      gicv3_hyp_init();
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 1757193..af8a34b 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -67,6 +67,11 @@ unsigned int gic_number_lines(void)
>>>      return gic_hw_ops->info->nr_lines;
>>>  }
>>>
>>> +bool_t gic_lpi_supported(void)
>>> +{
>>> +    return gic_hw_ops->info->lpi_supported;
>>> +}
>>> +
>>>  void gic_save_state(struct vcpu *v)
>>>  {
>>>      ASSERT(!local_irq_is_enabled());
>>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>>> index 1c7d9b6..4afb62b 100644
>>> --- a/xen/arch/arm/vgic-v3-its.c
>>> +++ b/xen/arch/arm/vgic-v3-its.c
>>
>> Can you explain why the emulation of the LPI property table has to be
>> done in the vITS code?
>>
>> Overall, there is nothing vITS specific in this code and all the
>> functions you've introduced within this file are called only by the
>> vgic-v3 code.
> 
> yes, it is called from vgic-v3 code because it is emulating GICR_PROP/PEND
> registers.
> 
> This is emulating LPI property table. Hence all the LPI handling code is kept
> in vits file.
> GICR_PROP/PEND is defined only for LPI case.

You don't explain why the LPI handling code is kept in the vits file....
The LPI property table can exist even without the presence of
the ITS.

There is nothing ITS specific in the emulation, and your design prove it
(you are calling all the emulation code from the vGICv3).

>>
>>> @@ -28,6 +28,7 @@
>>>  #include <asm/io.h>
>>>  #include <asm/gic_v3_defs.h>
>>>  #include <asm/gic.h>
>>> +#include <asm/gic-its.h>
>>>  #include <asm/vgic.h>
>>>  #include <asm/gic-its.h>
>>>  #include <asm/atomic.h>
>>> @@ -76,6 +77,34 @@ static inline uint32_t vits_get_max_collections(struct domain *d)
>>>      return (d->max_vcpus + 1);
>>>  }
>>>
>>> +static void vits_disable_lpi(struct vcpu *v, uint32_t vlpi)
>>> +{
>>> +    struct pending_irq *p;
>>> +
>>> +    p = irq_to_pending(v, vlpi);
>>> +    clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>>> +    gic_remove_from_queues(v, vlpi);
>>> +}
>>> +
>>> +static void vits_enable_lpi(struct vcpu *v, uint32_t vlpi, uint8_t priority)
>>> +{
>>> +    struct pending_irq *p;
>>> +    unsigned long flags;
>>> +
>>> +    p = irq_to_pending(v, vlpi);
>>> +
>>> +    set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>>> +
>>> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>> +
>>> +    /*XXX: raise on right vcpu */
>>
>> As said on the previous versions, I think there will be locking issue
>> given that pending_irq structure is protected by the vCPU where the IRQ
>> is locked.
> 
>  Can you please explain in detail why there is a locking issue.
> I remember this locking mechanism is coming from enable_irqs()
> as we follow same infrastructure to inject/process LPIs

I guess you mean vgic_enable_irqs? And no what you've implemented is
definitely not the same as vgic_enable_irqs.

vgic_enable_irqs is locking the pending_irq structure using the vgic
lock of the targeting VCPU (see v_target = ... ->get_target_cpu(...)).

Here, you are locking with the current vCPU, i.e the vCPU which wrote
into the LPI property table.

All the vGIC code is doing the same, so using the wrong locking won't
protect this structure.

> 
>>
>> If you think it's not the case please explain why but don't leave the
>> question unanswered.
>>
> [...]
>>>
>>> +
>>> +int vits_map_lpi_prop(struct domain *d)
>>> +{
>>> +    struct vgic_its *vits = d->arch.vgic.vits;
>>> +    paddr_t gaddr, addr;
>>> +    unsigned long mfn;
>>> +    uint32_t lpi_size, id_bits;
>>> +    int i;
>>> +
>>> +    gaddr = vits->propbase & MASK_4K;
>>
>> The physical address is only bits [47:12]. The uppers are either RES0 or
>> used for the OuterCache.
> 
> Can you please be more explicit. What is the issue here?

Sorry, I was confused by the MASK_4K which actually is not implemented
as the name suggested but which clearing the bits [63:48].

A correct MASK_4K would haven't clear the top bits. So the name of this
define is not right.

> 
>>
>>> +    id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1);
>>> +
>>> +    if ( id_bits > d->arch.vgic.id_bits )
>>> +        id_bits = d->arch.vgic.id_bits;
>>
>> As said on v4, you are allowing the possibility to have a smaller
>> property table than the effective number of LPIs.
>>
>> An ASSERT in vits_get_priority (patch #16) doesn't ensure the validity
>> of the size of the property table provided by the guest. This will
>> surely crash Xen in debug mode, and who knows what will happen in
>> production mode.
> 
>  lpi_size is calculated based on id_bits. If it is smaller, the lpi_size will be
> smaller where only size of lpi_size is considered.

Where id_bits is based on what the guest provides in GICR_PROPBASER.
Although the guest, malicious or not, can decide to setup this id_bits
smaller than the number of vLPIs effectively supported by the gic-v3.

In this case, if a vLPI higher than this number is injected you will hit
the ASSERT(vlpi < vits->prop_size) in vits_get_priority. A guest
*should* not be able to crash Xen because it decides to send valid input.

> 
>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
>>> index 683e3cc..a466a8f 100644
>>> --- a/xen/arch/arm/vgic-v3.c
>>> +++ b/xen/arch/arm/vgic-v3.c
>>> @@ -29,6 +29,8 @@
>>>  #include <asm/current.h>
>>>  #include <asm/mmio.h>
>>>  #include <asm/gic_v3_defs.h>
>>> +#include <asm/gic.h>
>>> +#include <asm/gic-its.h>
>>>  #include <asm/vgic.h>
>>>
>>>  /* GICD_PIDRn register values for ARM implementations */
>>> @@ -109,29 +111,47 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
>>>      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 aff;
>>> +    uint64_t aff, val;
>>>
>>>      switch ( gicr_reg )
>>>      {
>>>      case GICR_CTLR:
>>> -        /* We have not implemented LPI's, read zero */
>>> -        goto read_as_zero_32;
>>> +        if ( dabt.size != DABT_WORD ) goto bad_width;
>>> +        vgic_lock(v);
>>> +        *r = v->domain->arch.vgic.gicr_ctlr;
>>> +        vgic_unlock(v);
>>> +        return 1;
>>>      case GICR_IIDR:
>>>          if ( dabt.size != DABT_WORD ) goto bad_width;
>>>          *r = GICV3_GICR_IIDR_VAL;
>>>          return 1;
>>>      case GICR_TYPER:
>>> -        if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width;
>>> -        /* TBD: Update processor id in [23:8] when ITS support is added */
>>> +    case GICR_TYPER + 4:
>>
>> Why did you introduce it only in v5? This change is not related to the
>> LPI but a correct implementation of access size on GICv3 register.
>>
>> Overall, I think this should be done in a separate patch for all the
>> registers and not only this one. It would make the code change less
>> complicate to read.
>>
>> Please fix the write version of TYPER which happen to be 64 bits only too.
> 
> I added it because, after updating my Linux kernel driver, I see that it
>  is making 32-bit access to TYPER register

This change should go in a separate patch then. It's not related to this
patch.

> 
> static bool gic_rdists_supports_plpis(void)
> {
> return !!(readl_relaxed(gic_data_rdist_rd_base() + GICR_TYPER) &
> GICR_TYPER_PLPIS);
> }
> 
> But spec doesn't say it supports 32-bit access.

8.1.3 ARM IHI 0069A

"For the GITS_*, GICD_* and GICR_* registers, the upper 32 bits and the
lower 32 bits can be accessed
independently, unless the register requires a 64 bit access."

Regards,

-- 
Julien Grall