| Message ID | 20191220111833.1422-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 5f675c56ed262103b825cbab0e96c34fe681318d |
| Headers | show |
| Series | KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ | expand |
Hi Zenghui, On 12/20/19 12:18 PM, Zenghui Yu wrote: > Although guest will hardly read and use the PTZ (Pending Table Zero) > bit in GICR_PENDBASER, let us emulate the architecture strictly. > As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > > Noticed when checking all fields of GICR_PENDBASER register. > But _not_ sure whether it's worth a fix, as Linux never sets > the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). > > And I wonder under which scenarios can this bit be written as 1. > It seems difficult for software to determine whether the pending > table contains all zeros when writing this bit. > > virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 7dfd15dbb308..ebc218840fc2 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -414,8 +414,11 @@ static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, > gpa_t addr, unsigned int len) > { > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u64 value = vgic_cpu->pendbaser; > > - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); > + value &= ~GICR_PENDBASER_PTZ; > + > + return extract_bytes(value, addr & 7, len); > } > > static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, > Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
Hi, On 12/20/19 12:18 PM, Zenghui Yu wrote: > Although guest will hardly read and use the PTZ (Pending Table Zero) > bit in GICR_PENDBASER, let us emulate the architecture strictly. > As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> nit s/filed/field in the commit title Eric > --- > > Noticed when checking all fields of GICR_PENDBASER register. > But _not_ sure whether it's worth a fix, as Linux never sets > the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). > > And I wonder under which scenarios can this bit be written as 1. > It seems difficult for software to determine whether the pending > table contains all zeros when writing this bit. > > virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 7dfd15dbb308..ebc218840fc2 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -414,8 +414,11 @@ static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, > gpa_t addr, unsigned int len) > { > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u64 value = vgic_cpu->pendbaser; > > - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); > + value &= ~GICR_PENDBASER_PTZ; > + > + return extract_bytes(value, addr & 7, len); > } > > static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >
On 2019-12-20 11:18, Zenghui Yu wrote: > Although guest will hardly read and use the PTZ (Pending Table Zero) > bit in GICR_PENDBASER, let us emulate the architecture strictly. > As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > > Noticed when checking all fields of GICR_PENDBASER register. > But _not_ sure whether it's worth a fix, as Linux never sets > the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). > > And I wonder under which scenarios can this bit be written as 1. > It seems difficult for software to determine whether the pending > table contains all zeros when writing this bit. This is a useless HW optimization, where it can avoid reading the pending table the very first time you write to this register if it is told that it is all zero. A decent ITS implementation already has a mechanism to find out about the pending bits by looking into the IMPDEF area (the first 1kB) of the pending table. PTZ is just yet another way to do the same thing. This can only happen once in the lifetime of the system (when allocating the table), and Linux doesn't really care. As usual, the GIC is setting the level of useless complexity pretty high... > > virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c > b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 7dfd15dbb308..ebc218840fc2 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -414,8 +414,11 @@ static unsigned long > vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, > gpa_t addr, unsigned int len) > { > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u64 value = vgic_cpu->pendbaser; > > - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); > + value &= ~GICR_PENDBASER_PTZ; > + > + return extract_bytes(value, addr & 7, len); > } > > static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, Otherwise looks good. I'll queue it with Eric's correction to the subject line. Thanks, M.
Hi Marc, Eric, On 2019/12/20 21:07, Marc Zyngier wrote: > On 2019-12-20 11:18, Zenghui Yu wrote: >> Although guest will hardly read and use the PTZ (Pending Table Zero) >> bit in GICR_PENDBASER, let us emulate the architecture strictly. >> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >> --- >> >> Noticed when checking all fields of GICR_PENDBASER register. >> But _not_ sure whether it's worth a fix, as Linux never sets >> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >> >> And I wonder under which scenarios can this bit be written as 1. >> It seems difficult for software to determine whether the pending >> table contains all zeros when writing this bit. > > This is a useless HW optimization, where it can avoid reading the > pending table the very first time you write to this register if > it is told that it is all zero. A decent ITS implementation > already has a mechanism to find out about the pending bits by > looking into the IMPDEF area (the first 1kB) of the pending table. Yeah, AFAICT this is what Hisilicon has already implemented today. > PTZ is just yet another way to do the same thing. > > This can only happen once in the lifetime of the system (when allocating > the table), and Linux doesn't really care. I now get it, thanks for teaching me that! > As usual, the GIC is setting > the level of useless complexity pretty high... > >> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> index 7dfd15dbb308..ebc218840fc2 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> @@ -414,8 +414,11 @@ static unsigned long >> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >> gpa_t addr, unsigned int len) >> { >> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + u64 value = vgic_cpu->pendbaser; >> >> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >> + value &= ~GICR_PENDBASER_PTZ; >> + >> + return extract_bytes(value, addr & 7, len); >> } >> >> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, > > Otherwise looks good. I'll queue it with Eric's correction > to the subject line. Thanks both and Merry Christmas! Zenghui
On 2019/12/20 19:18, Zenghui Yu wrote: > Although guest will hardly read and use the PTZ (Pending Table Zero) > bit in GICR_PENDBASER, let us emulate the architecture strictly. > As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > > Noticed when checking all fields of GICR_PENDBASER register. > But _not_ sure whether it's worth a fix, as Linux never sets > the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). > > And I wonder under which scenarios can this bit be written as 1. > It seems difficult for software to determine whether the pending > table contains all zeros when writing this bit. > > virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c > index 7dfd15dbb308..ebc218840fc2 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c > @@ -414,8 +414,11 @@ static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, > gpa_t addr, unsigned int len) > { > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + u64 value = vgic_cpu->pendbaser; > > - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); > + value &= ~GICR_PENDBASER_PTZ; > + > + return extract_bytes(value, addr & 7, len); > } > > static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, > I noticed there is no userspace access callbacks for GICR_PENDBASER, so this patch will make the PTZ field also 'Read As Zero' by userspace. Should we consider adding a uaccess_read callback for GICR_PENDBASER which just returns the unchanged vgic_cpu->pendbaser to userspace? (Though this is really not a big deal. We now always emulate the PTZ field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' only indicates whether KVM will optimize the LPI enabling process, where Read As Zero indicates never optimize..) Thanks, Zenghui
Hi Zenghui, On 2019-12-23 13:43, Zenghui Yu wrote: > On 2019/12/20 19:18, Zenghui Yu wrote: >> Although guest will hardly read and use the PTZ (Pending Table Zero) >> bit in GICR_PENDBASER, let us emulate the architecture strictly. >> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >> --- >> Noticed when checking all fields of GICR_PENDBASER register. >> But _not_ sure whether it's worth a fix, as Linux never sets >> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >> And I wonder under which scenarios can this bit be written as 1. >> It seems difficult for software to determine whether the pending >> table contains all zeros when writing this bit. >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> index 7dfd15dbb308..ebc218840fc2 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> @@ -414,8 +414,11 @@ static unsigned long >> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >> gpa_t addr, unsigned int len) >> { >> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + u64 value = vgic_cpu->pendbaser; >> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >> + value &= ~GICR_PENDBASER_PTZ; >> + >> + return extract_bytes(value, addr & 7, len); >> } >> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >> > > I noticed there is no userspace access callbacks for GICR_PENDBASER, > so this patch will make the PTZ field also 'Read As Zero' by > userspace. > Should we consider adding a uaccess_read callback for GICR_PENDBASER > which just returns the unchanged vgic_cpu->pendbaser to userspace? > (Though this is really not a big deal. We now always emulate the PTZ > field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' > only indicates whether KVM will optimize the LPI enabling process, > where Read As Zero indicates never optimize..) I don't think adding a userspace accessor would help much. All this bit tells userspace is that the guest has programmed a zero filled table. On restore, we'd avoid a rescan of the table if there was no LPI mapped. And thinking of it, this fixes a bug for non-Linux guests: If you write PTZ=1, we never clear it. Which means that if userspace saves and restores PENDBASER with PTZ set, we'll never restore the pending bits, which is pretty bad (see vgic_enable_lpis()). This patch on its own fixes more than one bug! Thanks, M.
Hi Zenghui, On 12/23/19 2:43 PM, Zenghui Yu wrote: > On 2019/12/20 19:18, Zenghui Yu wrote: >> Although guest will hardly read and use the PTZ (Pending Table Zero) >> bit in GICR_PENDBASER, let us emulate the architecture strictly. >> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >> --- >> >> Noticed when checking all fields of GICR_PENDBASER register. >> But _not_ sure whether it's worth a fix, as Linux never sets >> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >> >> And I wonder under which scenarios can this bit be written as 1. >> It seems difficult for software to determine whether the pending >> table contains all zeros when writing this bit. >> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> index 7dfd15dbb308..ebc218840fc2 100644 >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >> @@ -414,8 +414,11 @@ static unsigned long >> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >> gpa_t addr, unsigned int len) >> { >> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + u64 value = vgic_cpu->pendbaser; >> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >> + value &= ~GICR_PENDBASER_PTZ; >> + >> + return extract_bytes(value, addr & 7, len); >> } >> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >> > > I noticed there is no userspace access callbacks for GICR_PENDBASER, > so this patch will make the PTZ field also 'Read As Zero' by userspace. > Should we consider adding a uaccess_read callback for GICR_PENDBASER > which just returns the unchanged vgic_cpu->pendbaser to userspace? > (Though this is really not a big deal. We now always emulate the PTZ > field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' > only indicates whether KVM will optimize the LPI enabling process, > where Read As Zero indicates never optimize..) You're right. If we start a migration when the PTZ has just been set by the SW, then we will miss it on the destination side. So for instance in the last KVM unit test of my series (https://lore.kernel.org/kvmarm/20191216140235.10751-17-eric.auger@redhat.com/), in test_its_pending_migration(), if you kick the migration before enabling LPI's at redist level, you shouldn't see any LPI hitting on the target which is theoretically wrong. So implementing a uaccess_read() would be better I think. Thanks Eric + ptr = gicv3_data.redist_base[nr_cpus - 1] + GICR_PENDBASER; + pendbaser = readq(ptr); + writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr); + + ptr = gicv3_data.redist_base[nr_cpus - 2] + GICR_PENDBASER; + pendbaser = readq(ptr); + writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr); + puts("Now migrate the VM, then press a key to continue...\n"); + (void)getchar(); + report(true, "Migration complete"); + + gicv3_rdist_ctrl_lpi(nr_cpus - 1, true); + gicv3_rdist_ctrl_lpi(nr_cpus - 2, true); + > > > Thanks, > Zenghui > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Zenghui, Marc, On 12/23/19 3:19 PM, Auger Eric wrote: > Hi Zenghui, > > On 12/23/19 2:43 PM, Zenghui Yu wrote: >> On 2019/12/20 19:18, Zenghui Yu wrote: >>> Although guest will hardly read and use the PTZ (Pending Table Zero) >>> bit in GICR_PENDBASER, let us emulate the architecture strictly. >>> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >>> >>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>> --- >>> >>> Noticed when checking all fields of GICR_PENDBASER register. >>> But _not_ sure whether it's worth a fix, as Linux never sets >>> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >>> >>> And I wonder under which scenarios can this bit be written as 1. >>> It seems difficult for software to determine whether the pending >>> table contains all zeros when writing this bit. >>> >>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> index 7dfd15dbb308..ebc218840fc2 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> @@ -414,8 +414,11 @@ static unsigned long >>> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >>> gpa_t addr, unsigned int len) >>> { >>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>> + u64 value = vgic_cpu->pendbaser; >>> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >>> + value &= ~GICR_PENDBASER_PTZ; >>> + >>> + return extract_bytes(value, addr & 7, len); >>> } >>> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >>> >> >> I noticed there is no userspace access callbacks for GICR_PENDBASER, >> so this patch will make the PTZ field also 'Read As Zero' by userspace. >> Should we consider adding a uaccess_read callback for GICR_PENDBASER >> which just returns the unchanged vgic_cpu->pendbaser to userspace? >> (Though this is really not a big deal. We now always emulate the PTZ >> field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' >> only indicates whether KVM will optimize the LPI enabling process, >> where Read As Zero indicates never optimize..) > You're right. If we start a migration when the PTZ has just been set by > the SW, then we will miss it on the destination side. > > So for instance in the last KVM unit test of my series > (https://lore.kernel.org/kvmarm/20191216140235.10751-17-eric.auger@redhat.com/), > in test_its_pending_migration(), if you kick the migration before > enabling LPI's at redist level, you shouldn't see any LPI hitting on the > target which is theoretically wrong. So implementing a uaccess_read() > would be better I think. > > Thanks > > Eric > > + ptr = gicv3_data.redist_base[nr_cpus - 1] + GICR_PENDBASER; > + pendbaser = readq(ptr); > + writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr); > + > + ptr = gicv3_data.redist_base[nr_cpus - 2] + GICR_PENDBASER; > + pendbaser = readq(ptr); > + writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr); That's a clear actually. So Marc is right, forget what I have just said. This will work on destination size as we will write 0. Sorry for the noise Hopefully Christmas break is coming ;-) Best Regards Eric > > + puts("Now migrate the VM, then press a key to continue...\n"); > + (void)getchar(); > + report(true, "Migration complete"); > + > + gicv3_rdist_ctrl_lpi(nr_cpus - 1, true); > + gicv3_rdist_ctrl_lpi(nr_cpus - 2, true); > + >> >> >> Thanks, >> Zenghui >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>
Hi Marc, Eric, On 2019/12/23 22:07, Marc Zyngier wrote: > Hi Zenghui, > > On 2019-12-23 13:43, Zenghui Yu wrote: >> On 2019/12/20 19:18, Zenghui Yu wrote: >>> Although guest will hardly read and use the PTZ (Pending Table Zero) >>> bit in GICR_PENDBASER, let us emulate the architecture strictly. >>> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>> --- >>> Noticed when checking all fields of GICR_PENDBASER register. >>> But _not_ sure whether it's worth a fix, as Linux never sets >>> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >>> And I wonder under which scenarios can this bit be written as 1. >>> It seems difficult for software to determine whether the pending >>> table contains all zeros when writing this bit. >>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> index 7dfd15dbb308..ebc218840fc2 100644 >>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>> @@ -414,8 +414,11 @@ static unsigned long >>> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >>> gpa_t addr, unsigned int len) >>> { >>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>> + u64 value = vgic_cpu->pendbaser; >>> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >>> + value &= ~GICR_PENDBASER_PTZ; >>> + >>> + return extract_bytes(value, addr & 7, len); >>> } >>> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >>> >> >> I noticed there is no userspace access callbacks for GICR_PENDBASER, >> so this patch will make the PTZ field also 'Read As Zero' by userspace. >> Should we consider adding a uaccess_read callback for GICR_PENDBASER >> which just returns the unchanged vgic_cpu->pendbaser to userspace? >> (Though this is really not a big deal. We now always emulate the PTZ >> field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' >> only indicates whether KVM will optimize the LPI enabling process, >> where Read As Zero indicates never optimize..) > > I don't think adding a userspace accessor would help much. All this > bit tells userspace is that the guest has programmed a zero filled > table. On restore, we'd avoid a rescan of the table if there was > no LPI mapped. Yes, I agree. > And thinking of it, this fixes a bug for non-Linux guests: If you write > PTZ=1, we never clear it. Which means that if userspace saves and restores > PENDBASER with PTZ set, we'll never restore the pending bits, which is > pretty bad (see vgic_enable_lpis()). But I'm afraid I can't follow this point. After reading the code (with Qemu) a bit further, the Redistributors are restored before the ITS. So there should be _no_ LPI has been mapped when we're restoring GICR_CTLR and enabling LPI, which says we will not scan the whole pending table and restore pending by vgic_enable_lpis()/its_sync_lpi_pending_table(), regardless of what the PTZ is. Instead, vgic_its_restore_ite()/vgic_v3_lpi_sync_pending_status() is where we actually read the guest RAM and restore the LPI pending state. Which means we will still do the right thing even for non-Linux guests. Not sure if I've got things correctly here. In the end, let's keep the patch as it is. > > This patch on its own fixes more than one bug! > If so, just by luck ;-) Thanks, Zenghui
Hi Zenghui, On 12/24/19 3:52 AM, Zenghui Yu wrote: > Hi Marc, Eric, > > On 2019/12/23 22:07, Marc Zyngier wrote: >> Hi Zenghui, >> >> On 2019-12-23 13:43, Zenghui Yu wrote: >>> On 2019/12/20 19:18, Zenghui Yu wrote: >>>> Although guest will hardly read and use the PTZ (Pending Table Zero) >>>> bit in GICR_PENDBASER, let us emulate the architecture strictly. >>>> As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. >>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >>>> --- >>>> Noticed when checking all fields of GICR_PENDBASER register. >>>> But _not_ sure whether it's worth a fix, as Linux never sets >>>> the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). >>>> And I wonder under which scenarios can this bit be written as 1. >>>> It seems difficult for software to determine whether the pending >>>> table contains all zeros when writing this bit. >>>> virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>>> index 7dfd15dbb308..ebc218840fc2 100644 >>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c >>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c >>>> @@ -414,8 +414,11 @@ static unsigned long >>>> vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, >>>> gpa_t addr, unsigned int len) >>>> { >>>> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>> + u64 value = vgic_cpu->pendbaser; >>>> - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); >>>> + value &= ~GICR_PENDBASER_PTZ; >>>> + >>>> + return extract_bytes(value, addr & 7, len); >>>> } >>>> static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu, >>>> >>> >>> I noticed there is no userspace access callbacks for GICR_PENDBASER, >>> so this patch will make the PTZ field also 'Read As Zero' by userspace. >>> Should we consider adding a uaccess_read callback for GICR_PENDBASER >>> which just returns the unchanged vgic_cpu->pendbaser to userspace? >>> (Though this is really not a big deal. We now always emulate the PTZ >>> field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' >>> only indicates whether KVM will optimize the LPI enabling process, >>> where Read As Zero indicates never optimize..) >> >> I don't think adding a userspace accessor would help much. All this >> bit tells userspace is that the guest has programmed a zero filled >> table. On restore, we'd avoid a rescan of the table if there was >> no LPI mapped. > > Yes, I agree. > >> And thinking of it, this fixes a bug for non-Linux guests: If you write >> PTZ=1, we never clear it. Which means that if userspace saves and >> restores >> PENDBASER with PTZ set, we'll never restore the pending bits, which is >> pretty bad (see vgic_enable_lpis()). > > But I'm afraid I can't follow this point. After reading the code (with > Qemu) a bit further, the Redistributors are restored before the ITS. This is also part of the kernel documentation: Documentation/virt/kvm/devices/arm-vgic-its.txt (ITS restore sequence) So > there should be _no_ LPI has been mapped when we're restoring GICR_CTLR > and enabling LPI, which says we will not scan the whole pending table > and restore pending by vgic_enable_lpis()/its_sync_lpi_pending_table(), > regardless of what the PTZ is. > > Instead, vgic_its_restore_ite()/vgic_v3_lpi_sync_pending_status() is > where we actually read the guest RAM and restore the LPI pending state. yes the pending state is restored from vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status and this path ignores the PTZ. Thanks Eric > Which means we will still do the right thing even for non-Linux guests. > Not sure if I've got things correctly here. > > In the end, let's keep the patch as it is. > >> >> This patch on its own fixes more than one bug! >> > > If so, just by luck ;-) > > > Thanks, > Zenghui > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2019/12/24 12:45, Auger Eric wrote: > Hi Zenghui, > > On 12/24/19 3:52 AM, Zenghui Yu wrote: >> Hi Marc, Eric, >> >> On 2019/12/23 22:07, Marc Zyngier wrote: >>> Hi Zenghui, >>> >>> On 2019-12-23 13:43, Zenghui Yu wrote: >>>> I noticed there is no userspace access callbacks for GICR_PENDBASER, >>>> so this patch will make the PTZ field also 'Read As Zero' by userspace. >>>> Should we consider adding a uaccess_read callback for GICR_PENDBASER >>>> which just returns the unchanged vgic_cpu->pendbaser to userspace? >>>> (Though this is really not a big deal. We now always emulate the PTZ >>>> field to guest as RAZ. And 'vgic_cpu->pendbaser & GICR_PENDBASER_PTZ' >>>> only indicates whether KVM will optimize the LPI enabling process, >>>> where Read As Zero indicates never optimize..) >>> >>> I don't think adding a userspace accessor would help much. All this >>> bit tells userspace is that the guest has programmed a zero filled >>> table. On restore, we'd avoid a rescan of the table if there was >>> no LPI mapped. >> >> Yes, I agree. >> >>> And thinking of it, this fixes a bug for non-Linux guests: If you write >>> PTZ=1, we never clear it. Which means that if userspace saves and >>> restores >>> PENDBASER with PTZ set, we'll never restore the pending bits, which is >>> pretty bad (see vgic_enable_lpis()). >> >> But I'm afraid I can't follow this point. After reading the code (with >> Qemu) a bit further, the Redistributors are restored before the ITS. > > This is also part of the kernel documentation: > Documentation/virt/kvm/devices/arm-vgic-its.txt (ITS restore sequence) Yeah, I see. Thanks for the pointer, Eric! Zenghui > So >> there should be _no_ LPI has been mapped when we're restoring GICR_CTLR >> and enabling LPI, which says we will not scan the whole pending table >> and restore pending by vgic_enable_lpis()/its_sync_lpi_pending_table(), >> regardless of what the PTZ is. >> >> Instead, vgic_its_restore_ite()/vgic_v3_lpi_sync_pending_status() is >> where we actually read the guest RAM and restore the LPI pending state. > yes the pending state is restored from > vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status and > this path ignores the PTZ. > > Thanks > > Eric >> Which means we will still do the right thing even for non-Linux guests. >> Not sure if I've got things correctly here. >> >> In the end, let's keep the patch as it is. >> >>> >>> This patch on its own fixes more than one bug! >>> >> >> If so, just by luck ;-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c index 7dfd15dbb308..ebc218840fc2 100644 --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c @@ -414,8 +414,11 @@ static unsigned long vgic_mmio_read_pendbase(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + u64 value = vgic_cpu->pendbaser; - return extract_bytes(vgic_cpu->pendbaser, addr & 7, len); + value &= ~GICR_PENDBASER_PTZ; + + return extract_bytes(value, addr & 7, len); } static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
Although guest will hardly read and use the PTZ (Pending Table Zero) bit in GICR_PENDBASER, let us emulate the architecture strictly. As per IHI 0069E 9.11.30, PTZ field is WO, and reads as 0. Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- Noticed when checking all fields of GICR_PENDBASER register. But _not_ sure whether it's worth a fix, as Linux never sets the PTZ bit before enabling LPI (set GICR_CTLR_ENABLE_LPIS). And I wonder under which scenarios can this bit be written as 1. It seems difficult for software to determine whether the pending table contains all zeros when writing this bit. virt/kvm/arm/vgic/vgic-mmio-v3.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)