Messages in this thread |  | | Subject | Re: [PATCH] KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ | | From | Zenghui Yu <> | | Date | Tue, 24 Dec 2019 10:52:26 +0800 |
| |
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
|  |