[PATCH 0/3] KVM: arm/arm64: vgic: Some cleanups and fixes

[PATCH 0/3] KVM: arm/arm64: vgic: Some cleanups and fixes

[Zenghui Yu]

Hi KVM/ARM maintainers,

This series contains three cleanups (fixes) I've collected when looking
through the vgic code. Please consider taking them if you're happy with
them.

Thanks!

Zenghui Yu (3):
  KVM: arm/arm64: vgic: Remove the declaration of
    kvm_send_userspace_msi()
  KVM: arm/arm64: vgic: Fix some comments typo
  KVM: arm/arm64: vgic: Don't rely on the wrong pending table

 include/kvm/arm_vgic.h      | 4 +---
 virt/kvm/arm/vgic/vgic-v3.c | 8 ++++----
 virt/kvm/arm/vgic/vgic-v4.c | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/3] KVM: arm/arm64: vgic: Remove the declaration of kvm_send_userspace_msi()

[Zenghui Yu]

The callsite of kvm_send_userspace_msi() is currently arch agnostic.
There seems no reason to keep an extra declaration of it in arm_vgic.h
(we already have one in include/linux/kvm_host.h).

Remove it.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 include/kvm/arm_vgic.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index af4f09c02bf1..0fb240ec0a2a 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -378,8 +378,6 @@ static inline int kvm_vgic_get_max_vcpus(void)
 	return kvm_vgic_global_state.max_gic_vcpus;
 }
 
-int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi);
-
 /**
  * kvm_vgic_setup_default_irq_routing:
  * Setup a default flat gsi routing table mapping all SPIs
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/3] KVM: arm/arm64: vgic: Remove the declaration of kvm_send_userspace_msi()

[Auger Eric]

Hi Zenghui,

On 10/29/19 8:19 AM, Zenghui Yu wrote:
> The callsite of kvm_send_userspace_msi() is currently arch agnostic.
> There seems no reason to keep an extra declaration of it in arm_vgic.h
> (we already have one in include/linux/kvm_host.h).
> 
> Remove it.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  include/kvm/arm_vgic.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index af4f09c02bf1..0fb240ec0a2a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -378,8 +378,6 @@ static inline int kvm_vgic_get_max_vcpus(void)
>  	return kvm_vgic_global_state.max_gic_vcpus;
>  }
>  
> -int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi);
> -
>  /**
>   * kvm_vgic_setup_default_irq_routing:
>   * Setup a default flat gsi routing table mapping all SPIs
> 

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/3] KVM: arm/arm64: vgic: Fix some comments typo

[Zenghui Yu]

s/vgic_its_save_pending_tables/vgic_v3_save_pending_tables/
s/then/the/

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 include/kvm/arm_vgic.h      | 2 +-
 virt/kvm/arm/vgic/vgic-v3.c | 2 +-
 virt/kvm/arm/vgic/vgic-v4.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 0fb240ec0a2a..01f8b3739a09 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -240,7 +240,7 @@ struct vgic_dist {
 	 * Contains the attributes and gpa of the LPI configuration table.
 	 * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
 	 * one address across all redistributors.
-	 * GICv3 spec: 6.1.2 "LPI Configuration tables"
+	 * GICv3 spec "LPI Configuration tables"
 	 */
 	u64			propbaser;
 
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 8d69f007dd0c..5ef93e5041e1 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -357,7 +357,7 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
 }
 
 /**
- * vgic_its_save_pending_tables - Save the pending tables into guest RAM
+ * vgic_v3_save_pending_tables - Save the pending tables into guest RAM
  * kvm lock and all vcpu lock must be held
  */
 int vgic_v3_save_pending_tables(struct kvm *kvm)
diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index 477af6aebb97..d864cf8dd212 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -266,7 +266,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int virq,
 
 	mutex_lock(&its->its_lock);
 
-	/* Perform then actual DevID/EventID -> LPI translation. */
+	/* Perform the actual DevID/EventID -> LPI translation. */
 	ret = vgic_its_resolve_lpi(kvm, its, irq_entry->msi.devid,
 				   irq_entry->msi.data, &irq);
 	if (ret)
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm/arm64: vgic: Fix some comments typo

[Marc Zyngier]

Hi Zenghui,

On Tue, 29 Oct 2019 07:19:18 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> s/vgic_its_save_pending_tables/vgic_v3_save_pending_tables/
> s/then/the/
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  include/kvm/arm_vgic.h      | 2 +-
>  virt/kvm/arm/vgic/vgic-v3.c | 2 +-
>  virt/kvm/arm/vgic/vgic-v4.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 0fb240ec0a2a..01f8b3739a09 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -240,7 +240,7 @@ struct vgic_dist {
>  	 * Contains the attributes and gpa of the LPI configuration table.
>  	 * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
>  	 * one address across all redistributors.
> -	 * GICv3 spec: 6.1.2 "LPI Configuration tables"
> +	 * GICv3 spec "LPI Configuration tables"

Why the change here? Pointing to the chapter in the spec is pretty
helpful, given that it is 800 pages long (although it should mention
what revision of the spec this refers to). For example, it should say
something like "IHI 0069E 6.1.1 ...".

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm/arm64: vgic: Fix some comments typo

[Zenghui Yu]

On 2019/10/29 17:04, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On Tue, 29 Oct 2019 07:19:18 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> s/vgic_its_save_pending_tables/vgic_v3_save_pending_tables/
>> s/then/the/
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> ---
>>   include/kvm/arm_vgic.h      | 2 +-
>>   virt/kvm/arm/vgic/vgic-v3.c | 2 +-
>>   virt/kvm/arm/vgic/vgic-v4.c | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index 0fb240ec0a2a..01f8b3739a09 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -240,7 +240,7 @@ struct vgic_dist {
>>   	 * Contains the attributes and gpa of the LPI configuration table.
>>   	 * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
>>   	 * one address across all redistributors.
>> -	 * GICv3 spec: 6.1.2 "LPI Configuration tables"
>> +	 * GICv3 spec "LPI Configuration tables"

Ah, this part shouldn't have been in this patch, as the description in
the commit message.
(And I remember the reason is just that, it it "6.1.1" in IHI 0069E but
"6.1.2" in some older versions.)

> 
> Why the change here? Pointing to the chapter in the spec is pretty
> helpful, given that it is 800 pages long (although it should mention
> what revision of the spec this refers to). For example, it should say
> something like "IHI 0069E 6.1.1 ...".

Yes, I agreed with you.  Marc, please feel free to drop this part,
or I can resend it with your suggestion.


Thanks,
Zenghui

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/3] KVM: arm/arm64: vgic: Fix some comments typo

[Marc Zyngier]

On Tue, 29 Oct 2019 12:45:15 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> On 2019/10/29 17:04, Marc Zyngier wrote:
> > Hi Zenghui,
> > 
> > On Tue, 29 Oct 2019 07:19:18 +0000,
> > Zenghui Yu <yuzenghui@huawei.com> wrote:
> >> 
> >> s/vgic_its_save_pending_tables/vgic_v3_save_pending_tables/
> >> s/then/the/
> >> 
> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> >> ---
> >>   include/kvm/arm_vgic.h      | 2 +-
> >>   virt/kvm/arm/vgic/vgic-v3.c | 2 +-
> >>   virt/kvm/arm/vgic/vgic-v4.c | 2 +-
> >>   3 files changed, 3 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> index 0fb240ec0a2a..01f8b3739a09 100644
> >> --- a/include/kvm/arm_vgic.h
> >> +++ b/include/kvm/arm_vgic.h
> >> @@ -240,7 +240,7 @@ struct vgic_dist {
> >>   	 * Contains the attributes and gpa of the LPI configuration table.
> >>   	 * Since we report GICR_TYPER.CommonLPIAff as 0b00, we can share
> >>   	 * one address across all redistributors.
> >> -	 * GICv3 spec: 6.1.2 "LPI Configuration tables"
> >> +	 * GICv3 spec "LPI Configuration tables"
> 
> Ah, this part shouldn't have been in this patch, as the description in
> the commit message.
> (And I remember the reason is just that, it it "6.1.1" in IHI 0069E but
> "6.1.2" in some older versions.)
> 
> > 
> > Why the change here? Pointing to the chapter in the spec is pretty
> > helpful, given that it is 800 pages long (although it should mention
> > what revision of the spec this refers to). For example, it should say
> > something like "IHI 0069E 6.1.1 ...".
> 
> Yes, I agreed with you.  Marc, please feel free to drop this part,
> or I can resend it with your suggestion.

No need, I'll fix it up locally.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Zenghui Yu]

It's possible that two LPIs locate in the same "byte_offset" but target
two different vcpus, where their pending status are indicated by two
different pending tables.  In such a scenario, using last_byte_offset
optimization will lead KVM relying on the wrong pending table entry.
Let us use last_ptr instead, which can be treated as a byte index into
a pending table and also, can be vcpu specific.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---

If this patch has done the right thing, we can even add the:

Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")

But to be honest, I'm not clear about what has this patch actually fixed.
Pending tables should contain all zeros before we flush vgic_irq's pending
status into guest's RAM (thinking that guest should never write anything
into it). So the pending table entry we've read from the guest memory
seems always be zero. And we will always do the right thing even if we
rely on the wrong pending table entry.

I think I must have some misunderstanding here... Please fix me.

 virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 5ef93e5041e1..7cd2e2f81513 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
 int vgic_v3_save_pending_tables(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
-	int last_byte_offset = -1;
 	struct vgic_irq *irq;
+	gpa_t last_ptr = -1;
 	int ret;
 	u8 val;
 
@@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 		bit_nr = irq->intid % BITS_PER_BYTE;
 		ptr = pendbase + byte_offset;
 
-		if (byte_offset != last_byte_offset) {
+		if (ptr != last_ptr) {
 			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
 			if (ret)
 				return ret;
-			last_byte_offset = byte_offset;
+			last_ptr = ptr;
 		}
 
 		stored = val & (1U << bit_nr);
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Marc Zyngier]

On Tue, 29 Oct 2019 07:19:19 +0000,
Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
> It's possible that two LPIs locate in the same "byte_offset" but target
> two different vcpus, where their pending status are indicated by two
> different pending tables.  In such a scenario, using last_byte_offset
> optimization will lead KVM relying on the wrong pending table entry.
> Let us use last_ptr instead, which can be treated as a byte index into
> a pending table and also, can be vcpu specific.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> 
> If this patch has done the right thing, we can even add the:
> 
> Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
> 
> But to be honest, I'm not clear about what has this patch actually fixed.
> Pending tables should contain all zeros before we flush vgic_irq's pending
> status into guest's RAM (thinking that guest should never write anything
> into it). So the pending table entry we've read from the guest memory
> seems always be zero. And we will always do the right thing even if we
> rely on the wrong pending table entry.
> 
> I think I must have some misunderstanding here... Please fix me.

I think you're spot on, and it is the code needs fixing, not you! The
problem is that we only read a byte once, irrespective of the vcpu the
interrupts is routed to. If we switch to another vcpu for the same
byte offset, we must reload it.

This can be done by either checking the vcpu, or by tracking the guest
address that we read from (just like you do here).

A small comment below:

>  virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 5ef93e5041e1..7cd2e2f81513 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>  int vgic_v3_save_pending_tables(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> -	int last_byte_offset = -1;
>  	struct vgic_irq *irq;
> +	gpa_t last_ptr = -1;

This should be written as

     gpa_t last_ptr = ~(gpa_t)0;

>  	int ret;
>  	u8 val;
>  
> @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  		bit_nr = irq->intid % BITS_PER_BYTE;
>  		ptr = pendbase + byte_offset;
>  
> -		if (byte_offset != last_byte_offset) {
> +		if (ptr != last_ptr) {
>  			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>  			if (ret)
>  				return ret;
> -			last_byte_offset = byte_offset;
> +			last_ptr = ptr;
>  		}
>  
>  		stored = val & (1U << bit_nr);

Otherwise, this looks good to me (no need to respin for the above
nit).

Eric, can I get an Ack from you, since you write this code?

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Zenghui Yu]

Hi Marc,

On 2019/10/29 17:23, Marc Zyngier wrote:
> On Tue, 29 Oct 2019 07:19:19 +0000,
> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>
>> It's possible that two LPIs locate in the same "byte_offset" but target
>> two different vcpus, where their pending status are indicated by two
>> different pending tables.  In such a scenario, using last_byte_offset
>> optimization will lead KVM relying on the wrong pending table entry.
>> Let us use last_ptr instead, which can be treated as a byte index into
>> a pending table and also, can be vcpu specific.
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> ---
>>
>> If this patch has done the right thing, we can even add the:
>>
>> Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
>>
>> But to be honest, I'm not clear about what has this patch actually fixed.
>> Pending tables should contain all zeros before we flush vgic_irq's pending
>> status into guest's RAM (thinking that guest should never write anything
>> into it). So the pending table entry we've read from the guest memory
>> seems always be zero. And we will always do the right thing even if we
>> rely on the wrong pending table entry.
>>
>> I think I must have some misunderstanding here... Please fix me.
> 
> I think you're spot on, and it is the code needs fixing, not you! The
> problem is that we only read a byte once, irrespective of the vcpu the
> interrupts is routed to. If we switch to another vcpu for the same
> byte offset, we must reload it.
> 
> This can be done by either checking the vcpu, or by tracking the guest
> address that we read from (just like you do here).

okay, the remaining question is that in vgic_v3_save_pending_tables():

	stored = val & (1U << bit_nr);
	if (stored == irq->pending_latch)
		continue;

	if (irq->pending_latch)
		val |= 1 << bit_nr;
	else
		val &= ~(1 << bit_nr);

Do we really have a scenario where irq->pending_latch==false and
stored==true (corresponds to the above "else") and then we clear
pending status of this LPI in guest memory?
I can not think out one now.

> 
> A small comment below:
> 
>>   virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 5ef93e5041e1..7cd2e2f81513 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>>   int vgic_v3_save_pending_tables(struct kvm *kvm)
>>   {
>>   	struct vgic_dist *dist = &kvm->arch.vgic;
>> -	int last_byte_offset = -1;
>>   	struct vgic_irq *irq;
>> +	gpa_t last_ptr = -1;
> 
> This should be written as
> 
>       gpa_t last_ptr = ~(gpa_t)0;

Thanks for pointing it out.

> 
>>   	int ret;
>>   	u8 val;
>>   
>> @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>   		bit_nr = irq->intid % BITS_PER_BYTE;
>>   		ptr = pendbase + byte_offset;
>>   
>> -		if (byte_offset != last_byte_offset) {
>> +		if (ptr != last_ptr) {
>>   			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>>   			if (ret)
>>   				return ret;
>> -			last_byte_offset = byte_offset;
>> +			last_ptr = ptr;
>>   		}
>>   
>>   		stored = val & (1U << bit_nr);
> 
> Otherwise, this looks good to me (no need to respin for the above
> nit).

Thanks,

Zenghui

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Auger Eric]

Hi Zenghui,

On 10/29/19 1:27 PM, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2019/10/29 17:23, Marc Zyngier wrote:
>> On Tue, 29 Oct 2019 07:19:19 +0000,
>> Zenghui Yu <yuzenghui@huawei.com> wrote:
>>>
>>> It's possible that two LPIs locate in the same "byte_offset" but target
>>> two different vcpus, where their pending status are indicated by two
>>> different pending tables.  In such a scenario, using last_byte_offset
>>> optimization will lead KVM relying on the wrong pending table entry.
>>> Let us use last_ptr instead, which can be treated as a byte index into
>>> a pending table and also, can be vcpu specific.
>>>
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>> ---
>>>
>>> If this patch has done the right thing, we can even add the:
>>>
>>> Fixes: 280771252c1b ("KVM: arm64: vgic-v3:
>>> KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
>>>
>>> But to be honest, I'm not clear about what has this patch actually
>>> fixed.
>>> Pending tables should contain all zeros before we flush vgic_irq's
>>> pending
>>> status into guest's RAM (thinking that guest should never write anything
>>> into it). So the pending table entry we've read from the guest memory
>>> seems always be zero. And we will always do the right thing even if we
>>> rely on the wrong pending table entry.
>>>
>>> I think I must have some misunderstanding here... Please fix me.
>>
>> I think you're spot on, and it is the code needs fixing, not you! The
>> problem is that we only read a byte once, irrespective of the vcpu the
>> interrupts is routed to. If we switch to another vcpu for the same
>> byte offset, we must reload it.
>>
>> This can be done by either checking the vcpu, or by tracking the guest
>> address that we read from (just like you do here).
> 
> okay, the remaining question is that in vgic_v3_save_pending_tables():
> 
>     stored = val & (1U << bit_nr);
>     if (stored == irq->pending_latch)
>         continue;
> 
>     if (irq->pending_latch)
>         val |= 1 << bit_nr;
>     else
>         val &= ~(1 << bit_nr);
> 
> Do we really have a scenario where irq->pending_latch==false and
> stored==true (corresponds to the above "else") and then we clear
> pending status of this LPI in guest memory?
> I can not think out one now.

if you save, restore and save again. On the 1st save the LPI may be
pending, it gets stored. On the second save the LPI may be not pending
anymore?

Thanks

Eric
> 
>>
>> A small comment below:
>>
>>>   virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>>> index 5ef93e5041e1..7cd2e2f81513 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm
>>> *kvm, struct vgic_irq *irq)
>>>   int vgic_v3_save_pending_tables(struct kvm *kvm)
>>>   {
>>>       struct vgic_dist *dist = &kvm->arch.vgic;
>>> -    int last_byte_offset = -1;
>>>       struct vgic_irq *irq;
>>> +    gpa_t last_ptr = -1;
>>
>> This should be written as
>>
>>       gpa_t last_ptr = ~(gpa_t)0;
> 
> Thanks for pointing it out.
> 
>>
>>>       int ret;
>>>       u8 val;
>>>   @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>>           bit_nr = irq->intid % BITS_PER_BYTE;
>>>           ptr = pendbase + byte_offset;
>>>   -        if (byte_offset != last_byte_offset) {
>>> +        if (ptr != last_ptr) {
>>>               ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>>>               if (ret)
>>>                   return ret;
>>> -            last_byte_offset = byte_offset;
>>> +            last_ptr = ptr;
>>>           }
>>>             stored = val & (1U << bit_nr);
>>
>> Otherwise, this looks good to me (no need to respin for the above
>> nit).
> 
> Thanks,
> 
> Zenghui
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Zenghui Yu]

Hi Eric,

On 2019/10/29 20:49, Auger Eric wrote:
> On 10/29/19 1:27 PM, Zenghui Yu wrote:
>> okay, the remaining question is that in vgic_v3_save_pending_tables():
>>
>>      stored = val & (1U << bit_nr);
>>      if (stored == irq->pending_latch)
>>          continue;
>>
>>      if (irq->pending_latch)
>>          val |= 1 << bit_nr;
>>      else
>>          val &= ~(1 << bit_nr);
>>
>> Do we really have a scenario where irq->pending_latch==false and
>> stored==true (corresponds to the above "else") and then we clear
>> pending status of this LPI in guest memory?
>> I can not think out one now.
> 
> if you save, restore and save again. On the 1st save the LPI may be
> pending, it gets stored. On the second save the LPI may be not pending
> anymore?

I assume you mean the "restore" by vgic_its_restore_ite().

While restoring a LPI, we will sync the pending status from guest
pending table (into the software pending_latch), and clear the
corresponding bit in guest memory.
See vgic_v3_lpi_sync_pending_status().

So on the second save, the LPI can be not pending, the guest pending
table will also indicate not pending.


Thanks,
Zenghui

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Auger Eric]

Hi Zenghui,

On 10/29/19 2:31 PM, Zenghui Yu wrote:
> Hi Eric,
> 
> On 2019/10/29 20:49, Auger Eric wrote:
>> On 10/29/19 1:27 PM, Zenghui Yu wrote:
>>> okay, the remaining question is that in vgic_v3_save_pending_tables():
>>>
>>>      stored = val & (1U << bit_nr);
>>>      if (stored == irq->pending_latch)
>>>          continue;
>>>
>>>      if (irq->pending_latch)
>>>          val |= 1 << bit_nr;
>>>      else
>>>          val &= ~(1 << bit_nr);
>>>
>>> Do we really have a scenario where irq->pending_latch==false and
>>> stored==true (corresponds to the above "else") and then we clear
>>> pending status of this LPI in guest memory?
>>> I can not think out one now.
>>
>> if you save, restore and save again. On the 1st save the LPI may be
>> pending, it gets stored. On the second save the LPI may be not pending
>> anymore?
> 
> I assume you mean the "restore" by vgic_its_restore_ite().

yes that's what I meant

> 
> While restoring a LPI, we will sync the pending status from guest
> pending table (into the software pending_latch), and clear the
> corresponding bit in guest memory.
> See vgic_v3_lpi_sync_pending_status().
> 
> So on the second save, the LPI can be not pending, the guest pending
> table will also indicate not pending.

You're right; I did not remember vgic_v3_lpi_sync_pending_status (called
from vgic_its_restore_ite/vgic_add_lpi) "cleared the consumed data"
(44de9d683847  KVM: arm64: vgic-v3: vgic_v3_lpi_sync_pending_status).

So effectively after restore the pending table is zeroed and the above
code should be rewrittable in a more simple manner, ie. just update the
byte in case the pending_latch is set.

Nethertheless your patch indeed fixes an actual bug independently on
this cleanup, ie. the written byte may be incorrect if LPIs belonging to
this byte target different RDIST.

Thanks

Eric
> 
> 
> Thanks,
> Zenghui
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Auger Eric]

Hi Zenghui, Marc,

On 10/29/19 8:19 AM, Zenghui Yu wrote:
> It's possible that two LPIs locate in the same "byte_offset" but target
> two different vcpus, where their pending status are indicated by two
> different pending tables.  In such a scenario, using last_byte_offset
> optimization will lead KVM relying on the wrong pending table entry.
> Let us use last_ptr instead, which can be treated as a byte index into
> a pending table and also, can be vcpu specific.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> 
> If this patch has done the right thing, we can even add the:
> 
> Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
> 
> But to be honest, I'm not clear about what has this patch actually fixed.
> Pending tables should contain all zeros before we flush vgic_irq's pending
> status into guest's RAM (thinking that guest should never write anything
> into it). So the pending table entry we've read from the guest memory
> seems always be zero. And we will always do the right thing even if we
> rely on the wrong pending table entry.
> 
> I think I must have some misunderstanding here... Please fix me.
> 
>  virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 5ef93e5041e1..7cd2e2f81513 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>  int vgic_v3_save_pending_tables(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> -	int last_byte_offset = -1;
>  	struct vgic_irq *irq;
> +	gpa_t last_ptr = -1;
>  	int ret;
>  	u8 val;
>  
> @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  		bit_nr = irq->intid % BITS_PER_BYTE;
>  		ptr = pendbase + byte_offset;
>  
> -		if (byte_offset != last_byte_offset) {
> +		if (ptr != last_ptr) {
>  			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>  			if (ret)
>  				return ret;
> -			last_byte_offset = byte_offset;
> +			last_ptr = ptr;
>  		}
>  
>  		stored = val & (1U << bit_nr);
> 
Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks for fixing this.

Eric


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

[Zenghui Yu]

On 2019/10/29 20:17, Auger Eric wrote:
> Hi Zenghui, Marc,
> 
> On 10/29/19 8:19 AM, Zenghui Yu wrote:
>> It's possible that two LPIs locate in the same "byte_offset" but target
>> two different vcpus, where their pending status are indicated by two
>> different pending tables.  In such a scenario, using last_byte_offset
>> optimization will lead KVM relying on the wrong pending table entry.
>> Let us use last_ptr instead, which can be treated as a byte index into
>> a pending table and also, can be vcpu specific.
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> ---
>>
>> If this patch has done the right thing, we can even add the:
>>
>> Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
>>
>> But to be honest, I'm not clear about what has this patch actually fixed.
>> Pending tables should contain all zeros before we flush vgic_irq's pending
>> status into guest's RAM (thinking that guest should never write anything
>> into it). So the pending table entry we've read from the guest memory
>> seems always be zero. And we will always do the right thing even if we
>> rely on the wrong pending table entry.
>>
>> I think I must have some misunderstanding here... Please fix me.
>>
>>   virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 5ef93e5041e1..7cd2e2f81513 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>>   int vgic_v3_save_pending_tables(struct kvm *kvm)
>>   {
>>   	struct vgic_dist *dist = &kvm->arch.vgic;
>> -	int last_byte_offset = -1;
>>   	struct vgic_irq *irq;
>> +	gpa_t last_ptr = -1;
>>   	int ret;
>>   	u8 val;
>>   
>> @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>>   		bit_nr = irq->intid % BITS_PER_BYTE;
>>   		ptr = pendbase + byte_offset;
>>   
>> -		if (byte_offset != last_byte_offset) {
>> +		if (ptr != last_ptr) {
>>   			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>>   			if (ret)
>>   				return ret;
>> -			last_byte_offset = byte_offset;
>> +			last_ptr = ptr;
>>   		}
>>   
>>   		stored = val & (1U << bit_nr);
>>
> Acked-by: Eric Auger <eric.auger@redhat.com>

Thanks Eric,


Zenghui

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm