| Message ID | 1464599682-14592-2-git-send-email-leonid.bloch@ravellosystems.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 30, 2016 at 12:14:26PM +0300, Leonid Bloch wrote: > From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > > Replace legacy cpu_to_le64w()/le64_to_cpup() > calls with stq_le_p()/ldq_le_p(). > > Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> Could you please add a code comment to clarify what's going on a bit more? Something to the point that capabilities are guaranteed to be dword-aligned only. Also, this isn't a dependency of this patchset I think - as far as I could say the only user of this is pcie: Introduce function for DSN capability creation but that merely accesses a capability, and all callers pass in an aligned offset. > --- > include/hw/pci/pci.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index ef6ba51..ee238ad 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config) > static inline void > pci_set_quad(uint8_t *config, uint64_t val) > { > - cpu_to_le64w((uint64_t *)config, val); > + stq_le_p(config, val); > } > > static inline uint64_t > pci_get_quad(const uint8_t *config) > { > - return le64_to_cpup((const uint64_t *)config); > + return ldq_le_p(config); > } > > static inline void > -- > 2.5.5
> On 30 May 2016, at 17:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, May 30, 2016 at 12:14:26PM +0300, Leonid Bloch wrote: >> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >> >> Replace legacy cpu_to_le64w()/le64_to_cpup() >> calls with stq_le_p()/ldq_le_p(). >> >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> > Hi Michael, > Could you please add a code comment to clarify what's going on a bit more? > Something to the point that capabilities are guaranteed to > be dword-aligned only. > Just to clarify, do you want to add these comments to pci_set/get_quad functions or to the new DSN-generation function? > Also, this isn't a dependency of this patchset I think - > as far as I could say the only user of this is > pcie: Introduce function for DSN capability creation > but that merely accesses a capability, and all callers pass in > an aligned offset. Right, this issue appeared after introduction of DSN generation function. All other callers pass aligned offsets so far. Thanks, Dmitry > >> --- >> include/hw/pci/pci.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index ef6ba51..ee238ad 100644 >> --- a/include/hw/pci/pci.h >> +++ b/include/hw/pci/pci.h >> @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config) >> static inline void >> pci_set_quad(uint8_t *config, uint64_t val) >> { >> - cpu_to_le64w((uint64_t *)config, val); >> + stq_le_p(config, val); >> } >> >> static inline uint64_t >> pci_get_quad(const uint8_t *config) >> { >> - return le64_to_cpup((const uint64_t *)config); >> + return ldq_le_p(config); >> } >> >> static inline void >> -- >> 2.5.5
On Mon, May 30, 2016 at 06:05:57PM +0300, Dmitry Fleytman wrote: > > > On 30 May 2016, at 17:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, May 30, 2016 at 12:14:26PM +0300, Leonid Bloch wrote: > >> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > >> > >> Replace legacy cpu_to_le64w()/le64_to_cpup() > >> calls with stq_le_p()/ldq_le_p(). > >> > >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> > > > > Hi Michael, > > > Could you please add a code comment to clarify what's going on a bit more? > > Something to the point that capabilities are guaranteed to > > be dword-aligned only. > > > > Just to clarify, do you want to add these comments to > pci_set/get_quad functions or to the new DSN-generation function? pci_set/get_quad > > Also, this isn't a dependency of this patchset I think - > > as far as I could say the only user of this is > > pcie: Introduce function for DSN capability creation > > but that merely accesses a capability, and all callers pass in > > an aligned offset. > > Right, this issue appeared after introduction of DSN generation function. Does DSN generation function pass unaligned offsets? It does not look like it does... > All other callers pass aligned offsets so far. > > Thanks, > Dmitry > > > > >> --- > >> include/hw/pci/pci.h | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > >> index ef6ba51..ee238ad 100644 > >> --- a/include/hw/pci/pci.h > >> +++ b/include/hw/pci/pci.h > >> @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config) > >> static inline void > >> pci_set_quad(uint8_t *config, uint64_t val) > >> { > >> - cpu_to_le64w((uint64_t *)config, val); > >> + stq_le_p(config, val); > >> } > >> > >> static inline uint64_t > >> pci_get_quad(const uint8_t *config) > >> { > >> - return le64_to_cpup((const uint64_t *)config); > >> + return ldq_le_p(config); > >> } > >> > >> static inline void > >> -- > >> 2.5.5
> On 30 May 2016, at 18:11 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, May 30, 2016 at 06:05:57PM +0300, Dmitry Fleytman wrote: >> >>> On 30 May 2016, at 17:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >>> >>> On Mon, May 30, 2016 at 12:14:26PM +0300, Leonid Bloch wrote: >>>> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >>>> >>>> Replace legacy cpu_to_le64w()/le64_to_cpup() >>>> calls with stq_le_p()/ldq_le_p(). >>>> >>>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >>>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> >>> >> >> Hi Michael, >> >>> Could you please add a code comment to clarify what's going on a bit more? >>> Something to the point that capabilities are guaranteed to >>> be dword-aligned only. >>> >> >> Just to clarify, do you want to add these comments to >> pci_set/get_quad functions or to the new DSN-generation function? > > pci_set/get_quad > >>> Also, this isn't a dependency of this patchset I think - >>> as far as I could say the only user of this is >>> pcie: Introduce function for DSN capability creation >>> but that merely accesses a capability, and all callers pass in >>> an aligned offset. >> >> Right, this issue appeared after introduction of DSN generation function. > > Does DSN generation function pass unaligned offsets? > It does not look like it does… It does according to clang sanitiser. > >> All other callers pass aligned offsets so far. >> >> Thanks, >> Dmitry >> >>> >>>> --- >>>> include/hw/pci/pci.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >>>> index ef6ba51..ee238ad 100644 >>>> --- a/include/hw/pci/pci.h >>>> +++ b/include/hw/pci/pci.h >>>> @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config) >>>> static inline void >>>> pci_set_quad(uint8_t *config, uint64_t val) >>>> { >>>> - cpu_to_le64w((uint64_t *)config, val); >>>> + stq_le_p(config, val); >>>> } >>>> >>>> static inline uint64_t >>>> pci_get_quad(const uint8_t *config) >>>> { >>>> - return le64_to_cpup((const uint64_t *)config); >>>> + return ldq_le_p(config); >>>> } >>>> >>>> static inline void >>>> -- >>>> 2.5.5
On Mon, May 30, 2016 at 06:14:56PM +0300, Dmitry Fleytman wrote: > Does DSN generation function pass unaligned offsets? > It does not look like it does… > > > It does according to clang sanitiser. Oh so it's a clang false positive?
> On 30 May 2016, at 18:19 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, May 30, 2016 at 06:14:56PM +0300, Dmitry Fleytman wrote: >> Does DSN generation function pass unaligned offsets? >> It does not look like it does… >> >> >> It does according to clang sanitiser. > > > Oh so it's a clang false positive? I think not. The capability itself is 8-bytes aligned but 64-bit serial number inside of it is not because of 32 bit header in front of it. > > -- > MST
On Mon, May 30, 2016 at 06:22:35PM +0300, Dmitry Fleytman wrote: > > > On 30 May 2016, at 18:19 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, May 30, 2016 at 06:14:56PM +0300, Dmitry Fleytman wrote: > >> Does DSN generation function pass unaligned offsets? > >> It does not look like it does… > >> > >> > >> It does according to clang sanitiser. > > > > > > Oh so it's a clang false positive? > > I think not. > The capability itself is 8-bytes aligned but 64-bit serial number inside of it is not because of 32 bit header in front of it. Oh right. Things like this should really go into commit log in the future. For now a code comment in pci set/get that explains that alignment in capabilities is generally at dword not qword boundary would be enough. > > > > -- > > MST
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index ef6ba51..ee238ad 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -468,13 +468,13 @@ pci_get_long(const uint8_t *config) static inline void pci_set_quad(uint8_t *config, uint64_t val) { - cpu_to_le64w((uint64_t *)config, val); + stq_le_p(config, val); } static inline uint64_t pci_get_quad(const uint8_t *config) { - return le64_to_cpup((const uint64_t *)config); + return ldq_le_p(config); } static inline void