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@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? Vladimir _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm