[PATCH] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

[PATCH] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

[Andre Przywara]

The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
corresponds to an unimplemented CPU interface is RAZ/WI."
Currently we allow the guest to write any value in there and it can
read that back.
Mask the written value with the proper CPU mask to be spec compliant.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index b44b359..e59d4c7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -129,6 +129,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
 				   unsigned long val)
 {
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
+	u8 cpu_mask = (1 << atomic_read(&vcpu->kvm->online_vcpus)) - 1;
 	int i;
 
 	/* GICD_ITARGETSR[0-7] are read-only */
@@ -141,7 +142,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
 
 		spin_lock(&irq->irq_lock);
 
-		irq->targets = (val >> (i * 8)) & 0xff;
+		irq->targets = ((val >> (i * 8)) & 0xff) & cpu_mask;
 		target = irq->targets ? __ffs(irq->targets) : 0;
 		irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
 
-- 
2.9.0

[PATCH] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

[Marc Zyngier]

Hi Andre,

On 15/11/16 14:27, Andre Przywara wrote:
> The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
> corresponds to an unimplemented CPU interface is RAZ/WI."
> Currently we allow the guest to write any value in there and it can
> read that back.
> Mask the written value with the proper CPU mask to be spec compliant.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..e59d4c7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -129,6 +129,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>  				   unsigned long val)
>  {
>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> +	u8 cpu_mask = (1 << atomic_read(&vcpu->kvm->online_vcpus)) - 1;

For the sake of avoiding open-coding things, how about using GENMASK?

>  	int i;
>  
>  	/* GICD_ITARGETSR[0-7] are read-only */
> @@ -141,7 +142,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>  
>  		spin_lock(&irq->irq_lock);
>  
> -		irq->targets = (val >> (i * 8)) & 0xff;
> +		irq->targets = ((val >> (i * 8)) & 0xff) & cpu_mask;

Can't you just drop the '& 0xff' part, since cpu_mask is guaranteed to
be more restrictive?

>  		target = irq->targets ? __ffs(irq->targets) : 0;
>  		irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

[Andre Przywara]

Hi Marc,

On 15/11/16 14:41, Marc Zyngier wrote:
> Hi Andre,
> 
> On 15/11/16 14:27, Andre Przywara wrote:
>> The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
>> corresponds to an unimplemented CPU interface is RAZ/WI."
>> Currently we allow the guest to write any value in there and it can
>> read that back.
>> Mask the written value with the proper CPU mask to be spec compliant.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index b44b359..e59d4c7 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -129,6 +129,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>>  				   unsigned long val)
>>  {
>>  	u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
>> +	u8 cpu_mask = (1 << atomic_read(&vcpu->kvm->online_vcpus)) - 1;
> 
> For the sake of avoiding open-coding things, how about using GENMASK?

Yes.

> 
>>  	int i;
>>  
>>  	/* GICD_ITARGETSR[0-7] are read-only */
>> @@ -141,7 +142,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>>  
>>  		spin_lock(&irq->irq_lock);
>>  
>> -		irq->targets = (val >> (i * 8)) & 0xff;
>> +		irq->targets = ((val >> (i * 8)) & 0xff) & cpu_mask;
> 
> Can't you just drop the '& 0xff' part, since cpu_mask is guaranteed to
> be more restrictive?

Well, and also irq->targets is an u8 ...

Fixed both.

Thanks!
Andre.

>>  		target = irq->targets ? __ffs(irq->targets) : 0;
>>  		irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>>  
>>
> 
> Thanks,
> 
> 	M.
>