Re: [Xen-devel] [PATCH v7 21/28] xen/arm: ITS: Add GICR register emulation

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Julien,

On Wed, Sep 23, 2015 at 3:52 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Vijay,
>
[...]
>> +static int vits_map_lpi_prop(struct vcpu *v)
>> +{
>> +    struct vgic_its *vits = v->domain->arch.vgic.vits;
>> +    paddr_t gaddr, addr;
>> +    unsigned long mfn, flags;
>> +    uint32_t id_bits, vgic_id_bits;
>> +    int i;
>> +
>> +    gaddr = vits->propbase & GICR_PROPBASER_PA_MASK;
>> +    id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1);
>> +
>> +    vgic_id_bits = get_count_order(v->domain->arch.vgic.nr_lpis +
>> +                                   FIRST_GIC_LPI);
>> +    /*
>> +     * Here we limit the size of LPI property table to the number of LPIs
>> +     * that domain supports.
>> +     */
>> +    if ( id_bits > vgic_id_bits )
>> +        id_bits = vgic_id_bits;
>
> As said on v4/v5, you are allowing the possibility to have a smaller
> property table than the effective number of LPIs.
>
> An ASSERT in vgic_v3_get_irq_priority (patch #25) 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.
>
> We had a discussion about what to do on v5 and I was expecting from you
> at least to put a TODO/comment in the code explaining what needs to be
> done in order to fix the problem.
>
> You could also have workaround the problem by always allocating the
> prop_page using vgic_id_bits not matter the size provide by the caller.
>
>> +
>> +    vits->prop_size = 1 << id_bits;
>
> The field prop_size should only be updated if we succeed to allocat the
> prop_page.
>
>> +
>> +    /*
>> +     * Allocate Virtual LPI Property table.
>> +     * TODO: To re-use guest property table
>> +     */
>> +    vits->prop_page = 
>> alloc_xenheap_pages(get_order_from_bytes(vits->prop_size),
>> +                                          0);
>> +    if ( !vits->prop_page )
>> +    {
>> +        printk(XENLOG_G_ERR
>> +               "d%d: vITS: Fail to allocate LPI Prop page\n",
>> +               v->domain->domain_id);
>> +        return 0;
>> +    }
>> +
>> +    addr = gaddr;
>> +
>> +    spin_lock_irqsave(&vits->prop_lock, flags);
>
> On a previous version I clearly explain the consequence of syncing the
> LPI property table. It's now getting worse with this lock because you
> disable the interrupt.

 LPI property table is accessed in interrupt context. So interrupts
are disabled.

In anycase GICR_PROPBASER can be updated only when
GICR_CTLR.Enable_LPIS = 0 ( I have missed this check ).
So we can enable/disable LPIs in GICR_CTLR instead of disabling interrupts.
But again question is (vgic.gicr_ctlr) is per vcpu. We have to disable
LPIs for this
vcpu and domain only. Check has to be made against gicr_ctlr value before
routing LPI for every domain?.

>
> Syncing the LPI property table may mean enabling an unknown amount
> amount of LPIs. This could be very slow and you block the physical CPU
> to receive any interrupt.
>
> While this may be acceptable (?) for DOM0 is this unacceptable for a
> guest. A valid guest could potentially block a CPU for a really long
> time due to the number of LPIs.
>

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