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

[Auger Eric]

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