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@xxxxxxxxxx>
---
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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
