Re: [PATCH] KVM: arm/arm64: vgic: Handle GICR_PENDBASER.PTZ filed as RAZ

[Auger Eric]

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@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..)
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@xxxxxxxxxx/),
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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>