| Message ID | 1491434362-30310-31-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 6 Apr 2017, 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. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3.c | 46 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 41 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 3b01247..ba0e79f 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -168,8 +168,12 @@ 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; > + if ( dabt.size != DABT_WORD ) goto bad_width; > + spin_lock(&v->arch.vgic.lock); > + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), > + info); > + spin_unlock(&v->arch.vgic.lock); > + return 1; > > case VREG32(GICR_IIDR): > if ( dabt.size != DABT_WORD ) goto bad_width; > @@ -181,16 +185,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; > @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; > + > + if ( !v->domain->arch.vgic.nr_lpis ) > + v->domain->arch.vgic.nr_lpis = nr_lpis; What if nr_lpis was already set and the nr_lpis has changed? > + 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) > @@ -421,8 +440,20 @@ 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; > + if ( dabt.size != DABT_WORD ) goto bad_width; > + if ( !v->domain->arch.vgic.has_its ) > + return 1; > + > + spin_lock(&v->arch.vgic.lock); > + > + /* 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(&v->arch.vgic.lock); > + > + return 1; > > case VREG32(GICR_IIDR): > /* RO */ > @@ -1032,6 +1063,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 = 24; > + } > typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; > > *r = vgic_reg32_extract(typer, info); > -- > 2.8.2 >
Hi, On 06/04/17 02:04, Stefano Stabellini wrote: > On Thu, 6 Apr 2017, 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. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic-v3.c | 46 +++++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 41 insertions(+), 5 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 3b01247..ba0e79f 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -168,8 +168,12 @@ 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; >> + if ( dabt.size != DABT_WORD ) goto bad_width; >> + spin_lock(&v->arch.vgic.lock); >> + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), >> + info); >> + spin_unlock(&v->arch.vgic.lock); >> + return 1; >> >> case VREG32(GICR_IIDR): >> if ( dabt.size != DABT_WORD ) goto bad_width; >> @@ -181,16 +185,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; >> @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; >> + >> + if ( !v->domain->arch.vgic.nr_lpis ) >> + v->domain->arch.vgic.nr_lpis = nr_lpis; > > What if nr_lpis was already set and the nr_lpis has changed? I think this can never happen: 1) vgic.rdist_propbase is only writeable when the redistributor has not been enabled yet: /* Writing PROPBASER with LPIs enabled is UNPREDICTABLE. */ if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) { reg = v->domain->arch.vgic.rdist_propbase; vgic_reg64_update(®, r, info); reg = sanitize_propbaser(reg); v->domain->arch.vgic.rdist_propbase = reg; } .... 2) This function above can only be called once: .... spin_lock(&v->arch.vgic.lock); /* 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); .... Does that sound safe? Sorry if the architecture is a bit awkward in this regard ;-) Cheers, Andre. >> + 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) >> @@ -421,8 +440,20 @@ 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; >> + if ( dabt.size != DABT_WORD ) goto bad_width; >> + if ( !v->domain->arch.vgic.has_its ) >> + return 1; >> + >> + spin_lock(&v->arch.vgic.lock); >> + >> + /* 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(&v->arch.vgic.lock); >> + >> + return 1; >> >> case VREG32(GICR_IIDR): >> /* RO */ >> @@ -1032,6 +1063,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 = 24; >> + } >> typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; >> >> *r = vgic_reg32_extract(typer, info); >> -- >> 2.8.2 >>
Hi, On 06/04/17 11:21, Andre Przywara wrote: > Hi, > > On 06/04/17 02:04, Stefano Stabellini wrote: >> On Thu, 6 Apr 2017, 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. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/arch/arm/vgic-v3.c | 46 +++++++++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 41 insertions(+), 5 deletions(-) >>> >>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >>> index 3b01247..ba0e79f 100644 >>> --- a/xen/arch/arm/vgic-v3.c >>> +++ b/xen/arch/arm/vgic-v3.c >>> @@ -168,8 +168,12 @@ 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; >>> + if ( dabt.size != DABT_WORD ) goto bad_width; >>> + spin_lock(&v->arch.vgic.lock); >>> + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), >>> + info); >>> + spin_unlock(&v->arch.vgic.lock); >>> + return 1; >>> >>> case VREG32(GICR_IIDR): >>> if ( dabt.size != DABT_WORD ) goto bad_width; >>> @@ -181,16 +185,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; >>> @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; >>> + >>> + if ( !v->domain->arch.vgic.nr_lpis ) >>> + v->domain->arch.vgic.nr_lpis = nr_lpis; >> >> What if nr_lpis was already set and the nr_lpis has changed? > > I think this can never happen: > 1) vgic.rdist_propbase is only writeable when the redistributor has not > been enabled yet: > /* Writing PROPBASER with LPIs enabled is UNPREDICTABLE. */ > if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) > { > reg = v->domain->arch.vgic.rdist_propbase; > vgic_reg64_update(®, r, info); > reg = sanitize_propbaser(reg); > v->domain->arch.vgic.rdist_propbase = reg; This will be called until VGIC_V3_LPIS_ENABLED will be set for the vCPU. However rdist_propbase is part of struct domain. So ... > } > .... > 2) This function above can only be called once: ... as this function will be called multiple per domain (once per vCPU). You could end up having nr_lpis not in sync with propbase. > .... > spin_lock(&v->arch.vgic.lock); > > /* 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); > .... > > Does that sound safe? Sorry if the architecture is a bit awkward in this > regard ;-) I am afraid that this is not safe. Tĥis is fine to have propbase stored in the domain because the spec (8.11.9 in ARM IHI 0069C) says: "Setting different values in different copies of GICR_PROPBASER on Redistributors that are required to use a common LPI Configuration table". But you need to keep Xen internal state consistent. Cheers,
On 06/04/17 12:42, Julien Grall wrote: > Hi, > > On 06/04/17 11:21, Andre Przywara wrote: >> Hi, >> >> On 06/04/17 02:04, Stefano Stabellini wrote: >>> On Thu, 6 Apr 2017, 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. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/vgic-v3.c | 46 >>>> +++++++++++++++++++++++++++++++++++++++++----- >>>> 1 file changed, 41 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >>>> index 3b01247..ba0e79f 100644 >>>> --- a/xen/arch/arm/vgic-v3.c >>>> +++ b/xen/arch/arm/vgic-v3.c >>>> @@ -168,8 +168,12 @@ 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; >>>> + if ( dabt.size != DABT_WORD ) goto bad_width; >>>> + spin_lock(&v->arch.vgic.lock); >>>> + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & >>>> VGIC_V3_LPIS_ENABLED), >>>> + info); >>>> + spin_unlock(&v->arch.vgic.lock); >>>> + return 1; >>>> >>>> case VREG32(GICR_IIDR): >>>> if ( dabt.size != DABT_WORD ) goto bad_width; >>>> @@ -181,16 +185,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; >>>> @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; >>>> + >>>> + if ( !v->domain->arch.vgic.nr_lpis ) >>>> + v->domain->arch.vgic.nr_lpis = nr_lpis; >>> >>> What if nr_lpis was already set and the nr_lpis has changed? >> >> I think this can never happen: >> 1) vgic.rdist_propbase is only writeable when the redistributor has not >> been enabled yet: >> /* Writing PROPBASER with LPIs enabled is UNPREDICTABLE. */ >> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) >> { >> reg = v->domain->arch.vgic.rdist_propbase; >> vgic_reg64_update(®, r, info); >> reg = sanitize_propbaser(reg); >> v->domain->arch.vgic.rdist_propbase = reg; > > This will be called until VGIC_V3_LPIS_ENABLED will be set for the vCPU. > However rdist_propbase is part of struct domain. So ... > >> } >> .... >> 2) This function above can only be called once: > > ... as this function will be called multiple per domain (once per vCPU). > You could end up having nr_lpis not in sync with propbase. > >> .... >> spin_lock(&v->arch.vgic.lock); >> >> /* 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); >> .... >> >> Does that sound safe? Sorry if the architecture is a bit awkward in this >> regard ;-) > > I am afraid that this is not safe. Tĥis is fine to have propbase stored > in the domain because the spec (8.11.9 in ARM IHI 0069C) says: > > "Setting different values in different copies of GICR_PROPBASER on > Redistributors that are required to use a common LPI Configuration table". > > But you need to keep Xen internal state consistent. Ah, the lovely GIC architecture. So technically GICR_PROPBASER is per redistributor (so per VCPU in our case), that's why I was mechanically using the VCPU lock. But as you rightly mention, the spec goes into great details why it actually isn't ;-) So indeed I need to use a lock per domain, also prevent changing propbaser once at least *one* VCPU has enabled its redistributor. Looks like I need a counter. Thanks for pointing this out! Cheers, Andre
Hi Andre, On 06/04/17 00:19, 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. Why 24 bits? This should be the number of LPIs supported by the host. And likely this number should be set from vgic_v3_domain_init(....) and not hardcoded in the emulation. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic-v3.c | 46 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 41 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 3b01247..ba0e79f 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -168,8 +168,12 @@ 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; > + if ( dabt.size != DABT_WORD ) goto bad_width; > + spin_lock(&v->arch.vgic.lock); > + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), > + info); > + spin_unlock(&v->arch.vgic.lock); > + return 1; > > case VREG32(GICR_IIDR): > if ( dabt.size != DABT_WORD ) goto bad_width; > @@ -181,16 +185,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; > @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; > + > + if ( !v->domain->arch.vgic.nr_lpis ) > + v->domain->arch.vgic.nr_lpis = nr_lpis; > + > + 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) > @@ -421,8 +440,20 @@ 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; > + if ( dabt.size != DABT_WORD ) goto bad_width; > + if ( !v->domain->arch.vgic.has_its ) > + return 1; > + > + spin_lock(&v->arch.vgic.lock); > + > + /* 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(&v->arch.vgic.lock); > + > + return 1; > > case VREG32(GICR_IIDR): > /* RO */ > @@ -1032,6 +1063,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 = 24; See my comment above. > + } > typer |= (irq_bits - 1) << GICD_TYPE_ID_BITS_SHIFT; > > *r = vgic_reg32_extract(typer, info); > Cheers,
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 3b01247..ba0e79f 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -168,8 +168,12 @@ 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; + if ( dabt.size != DABT_WORD ) goto bad_width; + spin_lock(&v->arch.vgic.lock); + *r = vgic_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED), + info); + spin_unlock(&v->arch.vgic.lock); + return 1; case VREG32(GICR_IIDR): if ( dabt.size != DABT_WORD ) goto bad_width; @@ -181,16 +185,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; @@ -411,6 +419,17 @@ 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) - LPI_OFFSET; + + if ( !v->domain->arch.vgic.nr_lpis ) + v->domain->arch.vgic.nr_lpis = nr_lpis; + + 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) @@ -421,8 +440,20 @@ 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; + if ( dabt.size != DABT_WORD ) goto bad_width; + if ( !v->domain->arch.vgic.has_its ) + return 1; + + spin_lock(&v->arch.vgic.lock); + + /* 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(&v->arch.vgic.lock); + + return 1; case VREG32(GICR_IIDR): /* RO */ @@ -1032,6 +1063,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 = 24; + } 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 | 46 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-)