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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Fri, Jul 31, 2015 at 4:35 PM, Julien Grall <julien.grall@citrix.com> wrote:
> 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,
[...]
>>>> +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.

   With just vlpi, we cannot get target vcpu without devid. Now
question is there a
need to call gic_raise_guest_irq() for inflight LPIs?

>>>
>>>> +    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.
>

>From 8.11.19, if id_bits is < 8192 (as below statement), GIC treats
LPIs as out of range.

"If the value of this field is less than 0b1101, indicating that the
largest interrupt ID is less than 8192
(the smallest LPI interrupt ID), the GIC will behave as if all
physical LPIs are out of range."

Based on this, we should make a check on this GICR_PROPBASER.id_bits
before injecting LPI to domain  when LPI is received.

>>
>>>> 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.

  Why separate patch?.  This change could be part of GICR* reg emulation
done for LPI

Regards
Vija