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

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

[Zenghui Yu]

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,
-- 
2.19.1

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

[Auger Eric]

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

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

[Auger 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,
> 

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

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

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

[Zenghui Yu]

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

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

[Zenghui Yu]

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

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

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

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

[Zenghui Yu]

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

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

[Auger Eric]

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

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

[Zenghui Yu]

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 ;-)

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@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
> 

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@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
>>