| Message ID | 20221026183004.7293-1-ayankuma@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v4] xen/arm: vGICv3: Emulate properly 32-bit access on GICR_PENDBASER | expand |
On 26/10/2022 19:30, Ayan Kumar Halder wrote: > If a guest is running in 32 bit mode and it tries to access > "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() > will return the value stored "v->arch.vgic.rdist_pendbase + 4". > This will be stored in a 64bit cpu register. > So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored > in the lower 32 bits of the 64bit cpu register. > > This 64bit cpu register is then modified bitwise with a mask (ie > GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the > 64 bit cpu register) is not cleared as expected by the specification. > > The correct thing to do here is to store the value of > "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is > then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to > vreg_reg64_extract() which will extract 32 bits from the given offset. > > Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect > v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is > now being read/written in an atomic manner (using read_atomic()/write_atomic()). > Fixes: fe7fa1332dabd9ce4 ("ARM: vGICv3: handle virtual LPI pending and > property tables") > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > --- > > Changes from:- > > v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate > GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an > appropriate commit message. > > v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read > v->arch.vgic.rdist_pendbase in an atomic context. > 2. Rectified the commit message to state that the cpu register is 64 bit. > (because currently, GICv3 is supported on Arm64 only). Reworded to make it > clear. > > v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase > in __vgic_v3_rdistr_rd_mmio_write(). > > xen/arch/arm/vgic-v3.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 0c23f6df9d..1adbdc0e54 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > > case VREG64(GICR_PENDBASER): > { > - unsigned long flags; > + uint64_t val; > > if ( !v->domain->arch.vgic.has_its ) > goto read_as_zero_64; > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > > - spin_lock_irqsave(&v->arch.vgic.lock, flags); > - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); > - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > + val = read_atomic(&v->arch.vgic.rdist_pendbase); > + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > + *r = vreg_reg64_extract(val, info); > return 1; > } > > @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, > > case VREG64(GICR_PENDBASER): > { > - unsigned long flags; > - > if ( !v->domain->arch.vgic.has_its ) > goto write_ignore_64; > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > > - spin_lock_irqsave(&v->arch.vgic.lock, flags); > - > /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ > if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) > { > - reg = v->arch.vgic.rdist_pendbase; > + reg = read_atomic(&v->arch.vgic.rdist_pendbase); > vreg_reg64_update(®, r, info); > reg = sanitize_pendbaser(reg); > - v->arch.vgic.rdist_pendbase = reg; > + write_atomic(&v->arch.vgic.rdist_pendbase, reg); > } > > - spin_unlock_irqrestore(&v->arch.vgic.lock, false); > - > return 1; > } >
On Wed, 26 Oct 2022 19:30:04 +0100 Ayan Kumar Halder <ayankuma@amd.com> wrote: Hi, > If a guest is running in 32 bit mode and it tries to access > "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() > will return the value stored "v->arch.vgic.rdist_pendbase + 4". > This will be stored in a 64bit cpu register. > So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored > in the lower 32 bits of the 64bit cpu register. > > This 64bit cpu register is then modified bitwise with a mask (ie > GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the > 64 bit cpu register) is not cleared as expected by the specification. > > The correct thing to do here is to store the value of > "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is > then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to > vreg_reg64_extract() which will extract 32 bits from the given offset. > > Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect > v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is > now being read/written in an atomic manner (using read_atomic()/write_atomic()). > > Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > Release-acked-by: Henry Wang <Henry.Wang@arm.com> > --- > > Changes from:- > > v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate > GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an > appropriate commit message. > > v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read > v->arch.vgic.rdist_pendbase in an atomic context. > 2. Rectified the commit message to state that the cpu register is 64 bit. > (because currently, GICv3 is supported on Arm64 only). Reworded to make it > clear. > > v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase > in __vgic_v3_rdistr_rd_mmio_write(). > > xen/arch/arm/vgic-v3.c | 19 ++++++------------- > 1 file changed, 6 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 0c23f6df9d..1adbdc0e54 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > > case VREG64(GICR_PENDBASER): > { > - unsigned long flags; > + uint64_t val; > > if ( !v->domain->arch.vgic.has_its ) > goto read_as_zero_64; > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > > - spin_lock_irqsave(&v->arch.vgic.lock, flags); > - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); > - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > + val = read_atomic(&v->arch.vgic.rdist_pendbase); > + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > + *r = vreg_reg64_extract(val, info); That part looks fine now. > return 1; > } > > @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, > > case VREG64(GICR_PENDBASER): > { > - unsigned long flags; > - > if ( !v->domain->arch.vgic.has_its ) > goto write_ignore_64; > if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > > - spin_lock_irqsave(&v->arch.vgic.lock, flags); > - I don't think you can drop the lock here easily. If it would be just for the LPIs enabled check, that'd be fine, because you can never turn LPIs off again (but that would deserve an explicit comment then). But down below you do a read-modify-write operation of rdist_pendbase, and need to make sure no one else is attempting that at the same time. Cheers, Andre > /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ > if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) > { > - reg = v->arch.vgic.rdist_pendbase; > + reg = read_atomic(&v->arch.vgic.rdist_pendbase); > vreg_reg64_update(®, r, info); > reg = sanitize_pendbaser(reg); > - v->arch.vgic.rdist_pendbase = reg; > + write_atomic(&v->arch.vgic.rdist_pendbase, reg); > } > > - spin_unlock_irqrestore(&v->arch.vgic.lock, false); > - > return 1; > } >
On 27/10/2022 10:44, Andre Przywara wrote: > On Wed, 26 Oct 2022 19:30:04 +0100 > Ayan Kumar Halder <ayankuma@amd.com> wrote: > > Hi, Hi Andre, Thanks for the feedback. Just one clarification. > >> If a guest is running in 32 bit mode and it tries to access >> "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() >> will return the value stored "v->arch.vgic.rdist_pendbase + 4". >> This will be stored in a 64bit cpu register. >> So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored >> in the lower 32 bits of the 64bit cpu register. >> >> This 64bit cpu register is then modified bitwise with a mask (ie >> GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the >> 64 bit cpu register) is not cleared as expected by the specification. >> >> The correct thing to do here is to store the value of >> "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is >> then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to >> vreg_reg64_extract() which will extract 32 bits from the given offset. >> >> Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect >> v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is >> now being read/written in an atomic manner (using read_atomic()/write_atomic()). >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >> --- >> >> Changes from:- >> >> v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate >> GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an >> appropriate commit message. >> >> v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read >> v->arch.vgic.rdist_pendbase in an atomic context. >> 2. Rectified the commit message to state that the cpu register is 64 bit. >> (because currently, GICv3 is supported on Arm64 only). Reworded to make it >> clear. >> >> v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase >> in __vgic_v3_rdistr_rd_mmio_write(). >> >> xen/arch/arm/vgic-v3.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 0c23f6df9d..1adbdc0e54 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, >> >> case VREG64(GICR_PENDBASER): >> { >> - unsigned long flags; >> + uint64_t val; >> >> if ( !v->domain->arch.vgic.has_its ) >> goto read_as_zero_64; >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); >> - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); >> - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ >> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); >> + val = read_atomic(&v->arch.vgic.rdist_pendbase); >> + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ >> + *r = vreg_reg64_extract(val, info); > That part looks fine now. > >> return 1; >> } >> >> @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, >> >> case VREG64(GICR_PENDBASER): >> { >> - unsigned long flags; >> - >> if ( !v->domain->arch.vgic.has_its ) >> goto write_ignore_64; >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); >> - > I don't think you can drop the lock here easily. If it would be just for > the LPIs enabled check, that'd be fine, because you can never turn LPIs off > again (but that would deserve an explicit comment then). /* LPIs once enabled (ie via vgic_vcpu_enable_lpis()) are never disabled by Xen */ Let me know if this comment makes sense. > > But down below you do a read-modify-write operation of rdist_pendbase, and > need to make sure no one else is attempting that at the same time. AFAIU, Xen is non preemptable. So there cannot be a context switch between read-modify-write. But I think you have a valid point. If two callers are trying to read-modify-write on rdist_pendbase at the same time, it is possible that caller2 reads a stale value of rdist_pendbase (just before caller1 writes). So caller2 might program rdist_pendbase in such a way that caller1's update is gone. I will fix this in v5. - Ayan > > Cheers, > Andre > >> /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ >> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) >> { >> - reg = v->arch.vgic.rdist_pendbase; >> + reg = read_atomic(&v->arch.vgic.rdist_pendbase); >> vreg_reg64_update(®, r, info); >> reg = sanitize_pendbaser(reg); >> - v->arch.vgic.rdist_pendbase = reg; >> + write_atomic(&v->arch.vgic.rdist_pendbase, reg); >> } >> >> - spin_unlock_irqrestore(&v->arch.vgic.lock, false); >> - >> return 1; >> } >>
On 27/10/2022 10:44, Andre Przywara wrote: > On Wed, 26 Oct 2022 19:30:04 +0100 > Ayan Kumar Halder <ayankuma@amd.com> wrote: > > Hi, Hi Andre, I need a clarification. > >> If a guest is running in 32 bit mode and it tries to access >> "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() >> will return the value stored "v->arch.vgic.rdist_pendbase + 4". >> This will be stored in a 64bit cpu register. >> So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored >> in the lower 32 bits of the 64bit cpu register. >> >> This 64bit cpu register is then modified bitwise with a mask (ie >> GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the >> 64 bit cpu register) is not cleared as expected by the specification. >> >> The correct thing to do here is to store the value of >> "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is >> then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to >> vreg_reg64_extract() which will extract 32 bits from the given offset. >> >> Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect >> v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is >> now being read/written in an atomic manner (using read_atomic()/write_atomic()). >> >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> >> Release-acked-by: Henry Wang <Henry.Wang@arm.com> >> --- >> >> Changes from:- >> >> v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate >> GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an >> appropriate commit message. >> >> v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read >> v->arch.vgic.rdist_pendbase in an atomic context. >> 2. Rectified the commit message to state that the cpu register is 64 bit. >> (because currently, GICv3 is supported on Arm64 only). Reworded to make it >> clear. >> >> v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase >> in __vgic_v3_rdistr_rd_mmio_write(). >> >> xen/arch/arm/vgic-v3.c | 19 ++++++------------- >> 1 file changed, 6 insertions(+), 13 deletions(-) >> >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 0c23f6df9d..1adbdc0e54 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, >> >> case VREG64(GICR_PENDBASER): >> { >> - unsigned long flags; >> + uint64_t val; >> >> if ( !v->domain->arch.vgic.has_its ) >> goto read_as_zero_64; >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); >> - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); >> - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ >> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); >> + val = read_atomic(&v->arch.vgic.rdist_pendbase); >> + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ >> + *r = vreg_reg64_extract(val, info); > That part looks fine now. > >> return 1; >> } >> >> @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, >> >> case VREG64(GICR_PENDBASER): >> { >> - unsigned long flags; >> - >> if ( !v->domain->arch.vgic.has_its ) >> goto write_ignore_64; >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; >> >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); >> - > I don't think you can drop the lock here easily. If it would be just for > the LPIs enabled check, that'd be fine, because you can never turn LPIs off > again (but that would deserve an explicit comment then). > > But down below you do a read-modify-write operation of rdist_pendbase, and > need to make sure no one else is attempting that at the same time. > > Cheers, > Andre > >> /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ >> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) >> { >> - reg = v->arch.vgic.rdist_pendbase; >> + reg = read_atomic(&v->arch.vgic.rdist_pendbase); >> vreg_reg64_update(®, r, info); >> reg = sanitize_pendbaser(reg); >> - v->arch.vgic.rdist_pendbase = reg; >> + write_atomic(&v->arch.vgic.rdist_pendbase, reg); >> } >> >> - spin_unlock_irqrestore(&v->arch.vgic.lock, false); Shouldn't this be "spin_unlock_irqrestore(&v->arch.vgic.lock, flags)" ? - Ayan >> - >> return 1; >> } >>
On 27/10/2022 16:40, Ayan Kumar Halder wrote: > > On 27/10/2022 10:44, Andre Przywara wrote: >> On Wed, 26 Oct 2022 19:30:04 +0100 >> Ayan Kumar Halder <ayankuma@amd.com> wrote: >> >> Hi, > > Hi Andre, Hi, > I need a clarification. I am not Andre but will answer :). [...] >>> /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ >>> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) >>> { >>> - reg = v->arch.vgic.rdist_pendbase; >>> + reg = read_atomic(&v->arch.vgic.rdist_pendbase); >>> vreg_reg64_update(®, r, info); >>> reg = sanitize_pendbaser(reg); >>> - v->arch.vgic.rdist_pendbase = reg; >>> + write_atomic(&v->arch.vgic.rdist_pendbase, reg); >>> } >>> - spin_unlock_irqrestore(&v->arch.vgic.lock, false); > > Shouldn't this be "spin_unlock_irqrestore(&v->arch.vgic.lock, flags)" ? Good catch. Yes it does. The current code will clear DAIF (even if irqsave touch only I). The I/O emulation is done with interrupts enabled usually, so now they are going to be unhandled until leave_hypervisor_to_guest(). This could be a "very" long time. Thankfully ITS is experimental, otherwise I would have considered this a potential security issue. Can you send a separate patch for that? Cheers,
On Thu, 27 Oct 2022 16:40:01 +0100 Ayan Kumar Halder <ayankuma@amd.com> wrote: Hi Ayan, > On 27/10/2022 10:44, Andre Przywara wrote: > > On Wed, 26 Oct 2022 19:30:04 +0100 > > Ayan Kumar Halder <ayankuma@amd.com> wrote: > > > > Hi, > > Hi Andre, > > I need a clarification. > > > > >> If a guest is running in 32 bit mode and it tries to access > >> "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() > >> will return the value stored "v->arch.vgic.rdist_pendbase + 4". > >> This will be stored in a 64bit cpu register. > >> So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored > >> in the lower 32 bits of the 64bit cpu register. > >> > >> This 64bit cpu register is then modified bitwise with a mask (ie > >> GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the > >> 64 bit cpu register) is not cleared as expected by the specification. > >> > >> The correct thing to do here is to store the value of > >> "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is > >> then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to > >> vreg_reg64_extract() which will extract 32 bits from the given offset. > >> > >> Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect > >> v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is > >> now being read/written in an atomic manner (using read_atomic()/write_atomic()). > >> > >> Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> > >> Release-acked-by: Henry Wang <Henry.Wang@arm.com> > >> --- > >> > >> Changes from:- > >> > >> v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate > >> GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an > >> appropriate commit message. > >> > >> v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read > >> v->arch.vgic.rdist_pendbase in an atomic context. > >> 2. Rectified the commit message to state that the cpu register is 64 bit. > >> (because currently, GICv3 is supported on Arm64 only). Reworded to make it > >> clear. > >> > >> v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase > >> in __vgic_v3_rdistr_rd_mmio_write(). > >> > >> xen/arch/arm/vgic-v3.c | 19 ++++++------------- > >> 1 file changed, 6 insertions(+), 13 deletions(-) > >> > >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > >> index 0c23f6df9d..1adbdc0e54 100644 > >> --- a/xen/arch/arm/vgic-v3.c > >> +++ b/xen/arch/arm/vgic-v3.c > >> @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > >> > >> case VREG64(GICR_PENDBASER): > >> { > >> - unsigned long flags; > >> + uint64_t val; > >> > >> if ( !v->domain->arch.vgic.has_its ) > >> goto read_as_zero_64; > >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > >> > >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); > >> - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); > >> - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > >> - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > >> + val = read_atomic(&v->arch.vgic.rdist_pendbase); > >> + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ > >> + *r = vreg_reg64_extract(val, info); > > That part looks fine now. > > > >> return 1; > >> } > >> > >> @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, > >> > >> case VREG64(GICR_PENDBASER): > >> { > >> - unsigned long flags; > >> - > >> if ( !v->domain->arch.vgic.has_its ) > >> goto write_ignore_64; > >> if ( !vgic_reg64_check_access(dabt) ) goto bad_width; > >> > >> - spin_lock_irqsave(&v->arch.vgic.lock, flags); > >> - > > I don't think you can drop the lock here easily. If it would be just for > > the LPIs enabled check, that'd be fine, because you can never turn LPIs off > > again (but that would deserve an explicit comment then). > > > > But down below you do a read-modify-write operation of rdist_pendbase, and > > need to make sure no one else is attempting that at the same time. > > > > Cheers, > > Andre > > > >> /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ > >> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) > >> { > >> - reg = v->arch.vgic.rdist_pendbase; > >> + reg = read_atomic(&v->arch.vgic.rdist_pendbase); > >> vreg_reg64_update(®, r, info); > >> reg = sanitize_pendbaser(reg); > >> - v->arch.vgic.rdist_pendbase = reg; > >> + write_atomic(&v->arch.vgic.rdist_pendbase, reg); > >> } > >> > >> - spin_unlock_irqrestore(&v->arch.vgic.lock, false); > > Shouldn't this be "spin_unlock_irqrestore(&v->arch.vgic.lock, flags)" ? Ouch, indeed, looks like a typo! There is a some type check in spin_lock_irqsave() and local_irq_restore(), but not in spin_unlock_irqrestore(), so we missed that. Cheers, Andre > > - Ayan > > >> - > >> return 1; > >> } > >>
On 27/10/2022 17:13, Julien Grall wrote: > > > On 27/10/2022 16:40, Ayan Kumar Halder wrote: >> >> On 27/10/2022 10:44, Andre Przywara wrote: >>> On Wed, 26 Oct 2022 19:30:04 +0100 >>> Ayan Kumar Halder <ayankuma@amd.com> wrote: >>> >>> Hi, >> >> Hi Andre, > > Hi, Hi Julien/Andre, > >> I need a clarification. > > I am not Andre but will answer :). > > [...] > >>>> /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ >>>> if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) >>>> { >>>> - reg = v->arch.vgic.rdist_pendbase; >>>> + reg = read_atomic(&v->arch.vgic.rdist_pendbase); >>>> vreg_reg64_update(®, r, info); >>>> reg = sanitize_pendbaser(reg); >>>> - v->arch.vgic.rdist_pendbase = reg; >>>> + write_atomic(&v->arch.vgic.rdist_pendbase, reg); >>>> } >>>> - spin_unlock_irqrestore(&v->arch.vgic.lock, false); >> >> Shouldn't this be "spin_unlock_irqrestore(&v->arch.vgic.lock, flags)" ? > > Good catch. Yes it does. The current code will clear DAIF (even if > irqsave touch only I). The I/O emulation is done with interrupts > enabled usually, so now they are going to be unhandled until > leave_hypervisor_to_guest(). > > This could be a "very" long time. Thankfully ITS is experimental, > otherwise I would have considered this a potential security issue. > > Can you send a separate patch for that? Thanks for confirming. I have now sent a patch to address this. - Ayan > > Cheers, >
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 0c23f6df9d..1adbdc0e54 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -249,16 +249,15 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, case VREG64(GICR_PENDBASER): { - unsigned long flags; + uint64_t val; if ( !v->domain->arch.vgic.has_its ) goto read_as_zero_64; if ( !vgic_reg64_check_access(dabt) ) goto bad_width; - spin_lock_irqsave(&v->arch.vgic.lock, flags); - *r = vreg_reg64_extract(v->arch.vgic.rdist_pendbase, info); - *r &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + val = read_atomic(&v->arch.vgic.rdist_pendbase); + val &= ~GICR_PENDBASER_PTZ; /* WO, reads as 0 */ + *r = vreg_reg64_extract(val, info); return 1; } @@ -566,25 +565,19 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, case VREG64(GICR_PENDBASER): { - unsigned long flags; - if ( !v->domain->arch.vgic.has_its ) goto write_ignore_64; if ( !vgic_reg64_check_access(dabt) ) goto bad_width; - spin_lock_irqsave(&v->arch.vgic.lock, flags); - /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */ if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) ) { - reg = v->arch.vgic.rdist_pendbase; + reg = read_atomic(&v->arch.vgic.rdist_pendbase); vreg_reg64_update(®, r, info); reg = sanitize_pendbaser(reg); - v->arch.vgic.rdist_pendbase = reg; + write_atomic(&v->arch.vgic.rdist_pendbase, reg); } - spin_unlock_irqrestore(&v->arch.vgic.lock, false); - return 1; }
If a guest is running in 32 bit mode and it tries to access "GICR_PENDBASER + 4" mmio reg, it will be trapped to Xen. vreg_reg64_extract() will return the value stored "v->arch.vgic.rdist_pendbase + 4". This will be stored in a 64bit cpu register. So now we have the top 32 bits of GICR_PENDBASER (a 64 bit MMIO register) stored in the lower 32 bits of the 64bit cpu register. This 64bit cpu register is then modified bitwise with a mask (ie GICR_PENDBASER_PTZ, it clears the 62nd bit). But the PTZ (which is bit 30 in the 64 bit cpu register) is not cleared as expected by the specification. The correct thing to do here is to store the value of "v->arch.vgic.rdist_pendbase" in a temporary 64 bit variable. This variable is then modified bitwise with GICR_PENDBASER_PTZ mask. It is then passed to vreg_reg64_extract() which will extract 32 bits from the given offset. Also, we have removed spin_lock_irqsave()/spin_unlock_irqrestore() to protect v->arch.vgic.rdist_pendbase. The reason being v->arch.vgic.rdist_pendbase is now being read/written in an atomic manner (using read_atomic()/write_atomic()). Signed-off-by: Ayan Kumar Halder <ayankuma@amd.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> --- Changes from:- v1 - 1. Extracted this fix from "[RFC PATCH v1 05/12] Arm: GICv3: Emulate GICR_PENDBASER and GICR_PROPBASER on AArch32" into a separate patch with an appropriate commit message. v2 - 1. Removed spin_lock_irqsave(). Used read_atomic() to read v->arch.vgic.rdist_pendbase in an atomic context. 2. Rectified the commit message to state that the cpu register is 64 bit. (because currently, GICv3 is supported on Arm64 only). Reworded to make it clear. v3 - 1. Added read_atomic()/write_atomic() for access to v->arch.vgic.rdist_pendbase in __vgic_v3_rdistr_rd_mmio_write(). xen/arch/arm/vgic-v3.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)