Re: [PATCH v2 3/7] KVM: arm: vgic-new: improve compatibility with 32-bit

[Vladimir Murzin <vladimir.murzin@arm.com>]

Hi Christoffer,

On 05/09/16 12:29, Christoffer Dall wrote:
> Hi Vladimir,
> 
> I think commit title is too vague, can you be more specific?
> 

KVM: arm: vgic-new: make extract_bytes to always work on 64-bit data

is it better?

> On Tue, Aug 16, 2016 at 11:46:54AM +0100, Vladimir Murzin wrote:
>> We have couple of 64-bit register defined in GICv3 architecture, so
> 
> 'a couple',  'registers' (plural)
> 
>> "unsigned long" kind of accessors wouldn't work for 32-bit. However,
> 
> 'wouldn't work for 32-bit' is kind of generic as well.  Perhaps you mean
> that unsigned long accesses to these registers will only access a single
> 32-bit work of that register.
> 
>> these registers can't be access as 64-bit in a one go if we run 32-bit
> 
> 'accessed', 's/in one go/with a single instruction/' ?
> 
> 'a 32-bit host'
> 
>> host simply because KVM doesn't support multiple load/store on MMIO
> 
> by 'multiple load/store' you mean the 'load/store multiple' instructions
> specifically, right?  Not a sequence of multiple loads and stores.  I
> think you should be more specific here as well, for example, I think
> ldrd and strd are problematic as well.
> 
>> space.
>>
>> It means that 32-bit guest access these registers in 32-bit chunks, so
> 
> 'a 32-bit guest', 'accesses'
> 

all suggestions you've made above are true. I'll rework commit message
to be more precise.

>> the only thing we need to do is to ensure that extract_bytes() always
>> takes 64-bit data.
>>
>> Since we are here fix couple of other width related issues by using
>> ULL variants over UL.
>>
>> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c |    6 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h    |    2 +-
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index ff668e0..cc20b60 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -23,7 +23,7 @@
>>  #include "vgic-mmio.h"
>>  
>>  /* extract @num bytes at @offset bytes offset in data */
>> -unsigned long extract_bytes(unsigned long data, unsigned int offset,
>> +unsigned long extract_bytes(u64 data, unsigned int offset,
>>  			    unsigned int num)
>>  {
>>  	return (data >> (offset * 8)) & GENMASK_ULL(num * 8 - 1, 0);
>> @@ -179,7 +179,7 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
>>  	int target_vcpu_id = vcpu->vcpu_id;
>>  	u64 value;
>>  
>> -	value = (mpidr & GENMASK(23, 0)) << 32;
>> +	value = (mpidr & GENMASK_ULL(23, 0)) << 32;
> 
> why does this make a difference when mpidr is an unsigned long?

because we access a little bit further than unsigned long can accommodate

  CC      arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.o
arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c: In function
'vgic_mmio_read_v3r_typer':
arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:184:35: warning:
left shift count >= width of type [-Wshift-count-overflow]
  value = (mpidr & GENMASK(23, 0)) << 32;
                                   ^

I can include this warning in commit message or maybe you want a
separate patch?

Vladimir