[PATCH] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[PATCH] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[Zenghui Yu]

When LPI support is enabled at redistributor level, VGIC will potentially
load the correspond LPI penging table and sync it into the pending_latch.
To avoid keeping the 'consumed' pending bits lying around in guest memory
(though they're not used), let's clear them after synchronization.

The similar work had been done in vgic_v3_lpi_sync_pending_status().

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d53d34a33e35..905760bfa404 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
 
 	for (i = 0; i < nr_irqs; i++) {
 		int byte_offset, bit_nr;
+		bool status;
 
 		byte_offset = intids[i] / BITS_PER_BYTE;
 		bit_nr = intids[i] % BITS_PER_BYTE;
@@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
 			ret = kvm_read_guest_lock(vcpu->kvm,
 						  pendbase + byte_offset,
 						  &pendmask, 1);
-			if (ret) {
-				kfree(intids);
-				return ret;
-			}
+			if (ret)
+				goto out;
 			last_byte_offset = byte_offset;
 		}
 
+		status = pendmask & (1 << bit_nr);
+
 		irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
 		raw_spin_lock_irqsave(&irq->irq_lock, flags);
-		irq->pending_latch = pendmask & (1U << bit_nr);
+		irq->pending_latch = status;
 		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
 		vgic_put_irq(vcpu->kvm, irq);
+
+		if (status) {
+			/* clear consumed data */
+			pendmask &= ~(1 << bit_nr);
+			ret = kvm_write_guest_lock(vcpu->kvm,
+						   pendbase + byte_offset,
+						   &pendmask, 1);
+			if (ret)
+				goto out;
+		}
 	}
 
+out:
 	kfree(intids);
-
 	return ret;
 }
 
-- 
2.19.1


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

Re: [PATCH] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[Marc Zyngier]

Hi Zenghui,

On Tue, 31 Mar 2020 11:12:45 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

> When LPI support is enabled at redistributor level, VGIC will potentially
> load the correspond LPI penging table and sync it into the pending_latch.
> To avoid keeping the 'consumed' pending bits lying around in guest memory
> (though they're not used), let's clear them after synchronization.
> 
> The similar work had been done in vgic_v3_lpi_sync_pending_status().
> 
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index d53d34a33e35..905760bfa404 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>  
>  	for (i = 0; i < nr_irqs; i++) {
>  		int byte_offset, bit_nr;
> +		bool status;
>  
>  		byte_offset = intids[i] / BITS_PER_BYTE;
>  		bit_nr = intids[i] % BITS_PER_BYTE;
> @@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>  			ret = kvm_read_guest_lock(vcpu->kvm,
>  						  pendbase + byte_offset,
>  						  &pendmask, 1);
> -			if (ret) {
> -				kfree(intids);
> -				return ret;
> -			}
> +			if (ret)
> +				goto out;
>  			last_byte_offset = byte_offset;
>  		}
>  
> +		status = pendmask & (1 << bit_nr);
> +
>  		irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
>  		raw_spin_lock_irqsave(&irq->irq_lock, flags);
> -		irq->pending_latch = pendmask & (1U << bit_nr);
> +		irq->pending_latch = status;
>  		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>  		vgic_put_irq(vcpu->kvm, irq);
> +
> +		if (status) {
> +			/* clear consumed data */
> +			pendmask &= ~(1 << bit_nr);
> +			ret = kvm_write_guest_lock(vcpu->kvm,
> +						   pendbase + byte_offset,
> +						   &pendmask, 1);
> +			if (ret)
> +				goto out;
> +		}
>  	}
>  
> +out:
>  	kfree(intids);
> -
>  	return ret;
>  }
>  

I've been thinking about this, and I wonder why we don't simply clear
the whole pending table instead of carefully wiping it one bit at a
time. My reasoning is that if a LPI isn't mapped, then it cannot be made
pending the first place.

And I think there is a similar issue in vgic_v3_lpi_sync_pending_status().
Why sync something back from the pending table when the LPI wasn't
mapped yet? This seems pretty bizarre, as the GITS_TRANSLATER spec says
that the write to this register is ignored when:

"- The EventID is mapped to an Interrupt Translation Table and the
EventID is within the range specified by MAPD on page 5-107, but the
EventID is unmapped."

(with the added bonus in the form of a type: the first instance of
"EventID" here should obviously be "DeviceID").

What do you think?

	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] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[Zenghui Yu]

Hi Marc,

On 2020/3/31 16:07, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On Tue, 31 Mar 2020 11:12:45 +0800
> Zenghui Yu <yuzenghui@huawei.com> wrote:
> 
>> When LPI support is enabled at redistributor level, VGIC will potentially
>> load the correspond LPI penging table and sync it into the pending_latch.
>> To avoid keeping the 'consumed' pending bits lying around in guest memory
>> (though they're not used), let's clear them after synchronization.
>>
>> The similar work had been done in vgic_v3_lpi_sync_pending_status().
>>
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> ---
>>   virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index d53d34a33e35..905760bfa404 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>>   
>>   	for (i = 0; i < nr_irqs; i++) {
>>   		int byte_offset, bit_nr;
>> +		bool status;
>>   
>>   		byte_offset = intids[i] / BITS_PER_BYTE;
>>   		bit_nr = intids[i] % BITS_PER_BYTE;
>> @@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>>   			ret = kvm_read_guest_lock(vcpu->kvm,
>>   						  pendbase + byte_offset,
>>   						  &pendmask, 1);
>> -			if (ret) {
>> -				kfree(intids);
>> -				return ret;
>> -			}
>> +			if (ret)
>> +				goto out;
>>   			last_byte_offset = byte_offset;
>>   		}
>>   
>> +		status = pendmask & (1 << bit_nr);
>> +
>>   		irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]);
>>   		raw_spin_lock_irqsave(&irq->irq_lock, flags);
>> -		irq->pending_latch = pendmask & (1U << bit_nr);
>> +		irq->pending_latch = status;
>>   		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
>>   		vgic_put_irq(vcpu->kvm, irq);
>> +
>> +		if (status) {
>> +			/* clear consumed data */
>> +			pendmask &= ~(1 << bit_nr);
>> +			ret = kvm_write_guest_lock(vcpu->kvm,
>> +						   pendbase + byte_offset,
>> +						   &pendmask, 1);
>> +			if (ret)
>> +				goto out;
>> +		}
>>   	}
>>   
>> +out:
>>   	kfree(intids);
>> -
>>   	return ret;
>>   }
>>   
> 
> I've been thinking about this, and I wonder why we don't simply clear
> the whole pending table instead of carefully wiping it one bit at a
> time. My reasoning is that if a LPI isn't mapped, then it cannot be made
> pending the first place.

A writing to GICR_CTLR.EnableLPIs can happen in parallel with MAPTI/INT
command sequence, where the new LPI is mapped to *this* vcpu and made
pending, wrong? I think commit 7d8b44c54e0c had described it in detail.

But thinking that we cache the pending bit in pending_latch (instead of
writing the corresponding bit in guest memory) when a LPI is made
pending, it seems to be safe to clear the whole pending table here.

> 
> And I think there is a similar issue in vgic_v3_lpi_sync_pending_status().
> Why sync something back from the pending table when the LPI wasn't
> mapped yet?

vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path:
vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status.
We should rely on it to sync the pending bit from guest memory (which
was saved on the source side).

> This seems pretty bizarre, as the GITS_TRANSLATER spec says
> that the write to this register is ignored when:
> 
> "- The EventID is mapped to an Interrupt Translation Table and the
> EventID is within the range specified by MAPD on page 5-107, but the
> EventID is unmapped."
> 
> (with the added bonus in the form of a type: the first instance of
> "EventID" here should obviously be "DeviceID").

;-)


Thanks,
Zenghui

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

Re: [PATCH] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[Marc Zyngier]

On Tue, 31 Mar 2020 17:11:37 +0800
Zenghui Yu <yuzenghui@huawei.com> wrote:

Hi Zenghui,

> Hi Marc,
> 
> On 2020/3/31 16:07, Marc Zyngier wrote:
> > Hi Zenghui,

[...]

> >>   > > I've been thinking about this, and I wonder why we don't simply clear  
> > the whole pending table instead of carefully wiping it one bit at a
> > time. My reasoning is that if a LPI isn't mapped, then it cannot be made
> > pending the first place.  
> 
> A writing to GICR_CTLR.EnableLPIs can happen in parallel with MAPTI/INT
> command sequence, where the new LPI is mapped to *this* vcpu and made
> pending, wrong? I think commit 7d8b44c54e0c had described it in detail.

I'm not sure this commit is relevant here. It describes how the
configuration is picked up by MAPTI, not how the pending bit got there
the first place.

> But thinking that we cache the pending bit in pending_latch (instead of
> writing the corresponding bit in guest memory) when a LPI is made
> pending, it seems to be safe to clear the whole pending table here.

Yes, and this is my worry. The spec is pretty vague about what the
behaviour of the redistributor is when something is set in the pending
table. At the moment, we treat these bits as if they had been generated
by a translation, but we do so inconsistently: we only pick these bits
up and generate a LPI if there is a mapping at the ITS level. If these
bits are relevant, we should forward a LPI to the CPU.

It feels we're in UNPREDICTIBLE land...

> 
> > 
> > And I think there is a similar issue in vgic_v3_lpi_sync_pending_status().
> > Why sync something back from the pending table when the LPI wasn't
> > mapped yet?  
> 
> vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path:
> vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status.
> We should rely on it to sync the pending bit from guest memory (which
> was saved on the source side).

The fact that we have *two* paths to restore pending bits is pretty
annoying. There is certainly some scope for simplification here.

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] KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization

[Zenghui Yu]

On 2020/4/1 18:27, Marc Zyngier wrote:

>>> And I think there is a similar issue in vgic_v3_lpi_sync_pending_status().
>>> Why sync something back from the pending table when the LPI wasn't
>>> mapped yet?
>>
>> vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path:
>> vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status.
>> We should rely on it to sync the pending bit from guest memory (which
>> was saved on the source side).
> 
> The fact that we have *two* paths to restore pending bits is pretty
> annoying. There is certainly some scope for simplification here.

One thing need to be clarified first (if we're going to do some 
simplification here) is that if we follow the "ITS Restore Sequence"
rule (in Documentation/virt/kvm/devices/arm-vgic-its.rst, which says
that all redistributors are restored *before* ITS table data), then
the pending bits will *only* be restored on the ITE restore path.

When we're restoring the GICR_CTLR, we invoke vgic_enable_lpis()->
its_sync_lpi_pending_table(). But since no LPI has been restored yet,
we will get an empty lpi_list snapshot from vgic_copy_lpi_list().
No pending table synchronization will happen on this path.

I think this is what we have in the current code.


Thanks,
Zenghui

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