[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 21/28] xen/arm: ITS: Add GICR register emulation
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |