[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v8 12/27] ARM: vGIC: advertise LPI support
On 12/04/17 13:48, Andre Przywara wrote:
On 12/04/17 13:38, Julien Grall wrote:
On 12/04/17 01:44, Andre Przywara wrote:
+static void vgic_vcpu_enable_lpis(struct vcpu *v)
+{
+ uint64_t reg = v->domain->arch.vgic.rdist_propbase;
+ unsigned int nr_lpis = BIT((reg & 0x1f) + 1);
+
+ if ( nr_lpis < LPI_OFFSET )
+ nr_lpis = 0;
+ else
+ nr_lpis -= LPI_OFFSET;
+
+ if ( !v->domain->arch.vgic.rdists_enabled )
+ {
+ v->domain->arch.vgic.nr_lpis = nr_lpis;
+ v->domain->arch.vgic.rdists_enabled = true;
+ }
+
+ v->arch.vgic.flags |= VGIC_V3_LPIS_ENABLED;
+}
+
static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t
*info,
uint32_t gicr_reg,
register_t r)
@@ -436,8 +470,26 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct
vcpu *v, mmio_info_t *info,
switch ( gicr_reg )
{
case VREG32(GICR_CTLR):
- /* LPI's not implemented */
- goto write_ignore_32;
+ {
+ unsigned long flags;
+
+ if ( !v->domain->arch.vgic.has_its )
+ goto write_ignore_32;
+ if ( dabt.size != DABT_WORD ) goto bad_width;
+
+ vgic_lock(v); /* protects rdists_enabled */
Again, can't you move this lock in vgic_vcpu_enable_lpis?
But the domain VGIC lock must be taken before the VGIC VCPU lock, right?
And we need the VGIC VCPU lock before calling the function to check for
LPIS_ENABLED, isn't it?
Then add an ASSERT in vgic_vcpu_enable_lpis and explain the locking.
+ spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+ /* LPIs can only be enabled once, but never disabled again. */
+ if ( (r & GICR_CTLR_ENABLE_LPIS) &&
+ !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
+ vgic_vcpu_enable_lpis(v);
+
+ spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+ vgic_unlock(v);
+
+ return 1;
+ }
case VREG32(GICR_IIDR):
/* RO */
@@ -1058,6 +1110,11 @@ static int vgic_v3_distr_mmio_read(struct vcpu
*v, mmio_info_t *info,
typer = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT |
DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32));
+ if ( v->domain->arch.vgic.has_its )
+ {
+ typer |= GICD_TYPE_LPIS;
+ irq_bits = v->domain->arch.vgic.intid_bits;
See my remark in patch #1. I would have expected the field intid_bits to
be used even if ITS is not present.
Well, this is just mimicking current code, which just puts the value
covering the number of SPIs in here.
As we only support LPIs with an ITS, I don't see why we should present
any higher value in there than we do today already for a GICv3.
You missed half of my point from patch #1. When I read "intid_bits", I
directly associate to the field GICD_TYPER.ID_bits and would expect the
both to be strictly identical.
So the field "intid_bits" should be correctly initialized in
vgic_v3_domain_init rather than having to wonder whether ITS is used or not.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
