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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Jul 30, 2015 at 10:34 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
> On 27/07/15 12:11, vijay.kilari@xxxxxxxxx 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

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

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

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

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

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

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.


Regards
Vijay

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