On 06/09/16 17:31, Christoffer Dall wrote:
> On Tue, Sep 06, 2016 at 02:54:01PM +0100, Vladimir Murzin wrote:
>> On 06/09/16 14:22, Christoffer Dall wrote:
>>> On Tue, Sep 06, 2016 at 01:41:37PM +0100, Vladimir Murzin wrote:
>>>> 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?
>>>
>>> I would suggest:
>>>
>>> KVM: arm: vgic: Support 64-bit data manipulation on 32-bit host systems
>>>
>>>>
>>>>> 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.
>>>>
>>>
>>> Thanks!
>>>
>>>>>> 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@xxxxxxx>
>>>>>> ---
>>>>>> 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?
>>>>
>>> My point was that the code doesn't really make sense when compiled on a
>>> 32-bit platform without also modifing the type for the mpidr variable.
>>> Am I missing something?
>>
>> I've not seen any difference in generated code, but for consistency I'll
>> update mpidr variable to u64.
>>
>
> That could be because you need to update kvm_vcpu_get_mpidr_aff() to
> return a u64 as well.
I think we don't need to update the type of mpidr. mpidr fits in
"unsigned long" nicely and kvm_vcpu_get_mpidr_aff() applies
MPIDR_HWID_BITMASK mask anyway.
In my patch I just abused GENMASK_ULL() and the proper fix for warning
produced by gcc should be
+ value = (u64)(mpidr & GENMASK(23, 0)) << 32;
Cheers
Vladimir
>
> -Christoffer
>
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
