| Message ID | 1491957874-31600-13-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 12/04/17 01:44, Andre Przywara wrote: > To let a guest know about the availability of virtual LPIs, set the > respective bits in the virtual GIC registers and let a guest control > the LPI enable bit. > Only report the LPI capability if the host has initialized at least > one ITS. > This removes a "TBD" comment, as we now populate the processor number > in the GICR_TYPE register. > Advertise 24 bits worth of LPIs to the guest. This sentence is not valid anymore... > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 62 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 849a6f3..33cd5f7 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -170,8 +170,19 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > switch ( gicr_reg ) > { > case VREG32(GICR_CTLR): > - /* We have not implemented LPI's, read zero */ > - goto read_as_zero_32; > + { > + unsigned long flags; > + > + if ( !v->domain->arch.vgic.has_its ) > + goto read_as_zero_32; > + if ( dabt.size != DABT_WORD ) goto bad_width; > + > + spin_lock_irqsave(&v->arch.vgic.lock, flags); > + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), > + info); > + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > + return 1; > + } > > case VREG32(GICR_IIDR): > if ( dabt.size != DABT_WORD ) goto bad_width; > @@ -183,16 +194,20 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > uint64_t typer, aff; > > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > - /* TBD: Update processor id in [23:8] when ITS support is added */ > aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); > typer = aff; > + /* We use the VCPU ID as the redistributor ID in bits[23:8] */ > + typer |= (v->vcpu_id & 0xffff) << 8; > > if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST ) > typer |= GICR_TYPER_LAST; > > + if ( v->domain->arch.vgic.has_its ) > + typer |= GICR_TYPER_PLPIS; > + > *r = vgic_reg64_extract(typer, info); > > return 1; > @@ -426,6 +441,25 @@ static uint64_t sanitize_pendbaser(uint64_t reg) > return reg; > } > > +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? > + 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. > + } > typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; > > *r = vgic_reg32_extract(typer, info); > Cheers,
Hi, On 12/04/17 13:38, Julien Grall wrote: > Hi Andre, > > On 12/04/17 01:44, Andre Przywara wrote: >> To let a guest know about the availability of virtual LPIs, set the >> respective bits in the virtual GIC registers and let a guest control >> the LPI enable bit. >> Only report the LPI capability if the host has initialized at least >> one ITS. >> This removes a "TBD" comment, as we now populate the processor number >> in the GICR_TYPE register. >> Advertise 24 bits worth of LPIs to the guest. > > This sentence is not valid anymore... Indeed .... >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-v3.c | 67 >> ++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 62 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 849a6f3..33cd5f7 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -170,8 +170,19 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct >> vcpu *v, mmio_info_t *info, >> switch ( gicr_reg ) >> { >> case VREG32(GICR_CTLR): >> - /* We have not implemented LPI's, read zero */ >> - goto read_as_zero_32; >> + { >> + unsigned long flags; >> + >> + if ( !v->domain->arch.vgic.has_its ) >> + goto read_as_zero_32; >> + if ( dabt.size != DABT_WORD ) goto bad_width; >> + >> + spin_lock_irqsave(&v->arch.vgic.lock, flags); >> + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & >> VGIC_V3_LPIS_ENABLED), >> + info); >> + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); >> + return 1; >> + } >> >> case VREG32(GICR_IIDR): >> if ( dabt.size != DABT_WORD ) goto bad_width; >> @@ -183,16 +194,20 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct >> vcpu *v, mmio_info_t *info, >> uint64_t typer, aff; >> >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> - /* TBD: Update processor id in [23:8] when ITS support is >> added */ >> aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | >> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | >> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | >> MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); >> typer = aff; >> + /* We use the VCPU ID as the redistributor ID in bits[23:8] */ >> + typer |= (v->vcpu_id & 0xffff) << 8; >> >> if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST ) >> typer |= GICR_TYPER_LAST; >> >> + if ( v->domain->arch.vgic.has_its ) >> + typer |= GICR_TYPER_PLPIS; >> + >> *r = vgic_reg64_extract(typer, info); >> >> return 1; >> @@ -426,6 +441,25 @@ static uint64_t sanitize_pendbaser(uint64_t reg) >> return reg; >> } >> >> +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? >> + 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. Cheers, Andre. >> + } >> typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; >> >> *r = vgic_reg32_extract(typer, info); >> > > Cheers, >
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,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 849a6f3..33cd5f7 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -170,8 +170,19 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, switch ( gicr_reg ) { case VREG32(GICR_CTLR): - /* We have not implemented LPI's, read zero */ - goto read_as_zero_32; + { + unsigned long flags; + + if ( !v->domain->arch.vgic.has_its ) + goto read_as_zero_32; + if ( dabt.size != DABT_WORD ) goto bad_width; + + spin_lock_irqsave(&v->arch.vgic.lock, flags); + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), + info); + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + return 1; + } case VREG32(GICR_IIDR): if ( dabt.size != DABT_WORD ) goto bad_width; @@ -183,16 +194,20 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, uint64_t typer, aff; if ( !vgic_reg64_check_access(dabt) ) goto bad_width; - /* TBD: Update processor id in [23:8] when ITS support is added */ aff = (MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 3) << 56 | MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); typer = aff; + /* We use the VCPU ID as the redistributor ID in bits[23:8] */ + typer |= (v->vcpu_id & 0xffff) << 8; if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST ) typer |= GICR_TYPER_LAST; + if ( v->domain->arch.vgic.has_its ) + typer |= GICR_TYPER_PLPIS; + *r = vgic_reg64_extract(typer, info); return 1; @@ -426,6 +441,25 @@ static uint64_t sanitize_pendbaser(uint64_t reg) return reg; } +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 */ + 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; + } typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; *r = vgic_reg32_extract(typer, info);
To let a guest know about the availability of virtual LPIs, set the respective bits in the virtual GIC registers and let a guest control the LPI enable bit. Only report the LPI capability if the host has initialized at least one ITS. This removes a "TBD" comment, as we now populate the processor number in the GICR_TYPE register. Advertise 24 bits worth of LPIs to the guest. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic-v3.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 62 insertions(+), 5 deletions(-)