| Message ID | 1471344418-19568-4-git-send-email-vladimir.murzin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vladimir, I think commit title is too vague, can you be more specific? 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' > 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? > value |= ((target_vcpu_id & 0xffff) << 8); > if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1) > value |= GICR_TYPER_LAST; > @@ -603,7 +603,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg) > bool broadcast; > > sgi = (reg & ICC_SGI1R_SGI_ID_MASK) >> ICC_SGI1R_SGI_ID_SHIFT; > - broadcast = reg & BIT(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); > + broadcast = reg & BIT_ULL(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); > target_cpus = (reg & ICC_SGI1R_TARGET_LIST_MASK) >> ICC_SGI1R_TARGET_LIST_SHIFT; > mpidr = SGI_AFFINITY_LEVEL(reg, 3); > mpidr |= SGI_AFFINITY_LEVEL(reg, 2); > diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h > index 0b3ecf9..80f92ce 100644 > --- a/virt/kvm/arm/vgic/vgic-mmio.h > +++ b/virt/kvm/arm/vgic/vgic-mmio.h > @@ -96,7 +96,7 @@ unsigned long vgic_data_mmio_bus_to_host(const void *val, unsigned int len); > void vgic_data_host_to_mmio_bus(void *buf, unsigned int len, > unsigned long data); > > -unsigned long extract_bytes(unsigned long data, unsigned int offset, > +unsigned long extract_bytes(u64 data, unsigned int offset, > unsigned int num); > > u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len, > -- > 1.7.9.5 >
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
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@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? > 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? Thanks, -Christoffer
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@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? >> > 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. Thanks Vladimir > > Thanks, > -Christoffer > >
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@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? > >> > > 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. -Christoffer
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@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? >>>> >>> 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 > >
On Wed, Sep 07, 2016 at 10:06:33AM +0100, Vladimir Murzin wrote: > 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@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? > >>>> > >>> 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; > > Ah, right, this is all the 32-bit include files I should be looking at, so this makes sense. Thanks, -Christoffer
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; value |= ((target_vcpu_id & 0xffff) << 8); if (target_vcpu_id == atomic_read(&vcpu->kvm->online_vcpus) - 1) value |= GICR_TYPER_LAST; @@ -603,7 +603,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg) bool broadcast; sgi = (reg & ICC_SGI1R_SGI_ID_MASK) >> ICC_SGI1R_SGI_ID_SHIFT; - broadcast = reg & BIT(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); + broadcast = reg & BIT_ULL(ICC_SGI1R_IRQ_ROUTING_MODE_BIT); target_cpus = (reg & ICC_SGI1R_TARGET_LIST_MASK) >> ICC_SGI1R_TARGET_LIST_SHIFT; mpidr = SGI_AFFINITY_LEVEL(reg, 3); mpidr |= SGI_AFFINITY_LEVEL(reg, 2); diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h index 0b3ecf9..80f92ce 100644 --- a/virt/kvm/arm/vgic/vgic-mmio.h +++ b/virt/kvm/arm/vgic/vgic-mmio.h @@ -96,7 +96,7 @@ unsigned long vgic_data_mmio_bus_to_host(const void *val, unsigned int len); void vgic_data_host_to_mmio_bus(void *buf, unsigned int len, unsigned long data); -unsigned long extract_bytes(unsigned long data, unsigned int offset, +unsigned long extract_bytes(u64 data, unsigned int offset, unsigned int num); u64 update_64bit_reg(u64 reg, unsigned int offset, unsigned int len,
We have couple of 64-bit register defined in GICv3 architecture, so "unsigned long" kind of accessors wouldn't work for 32-bit. However, these registers can't be access as 64-bit in a one go if we run 32-bit host simply because KVM doesn't support multiple load/store on MMIO space. It means that 32-bit guest access these registers in 32-bit chunks, so 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(-)