| Message ID | 540CF146-518B-40BA-86A3-5A7B1D89E6B4@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29 May 2016 at 16:22, Dmitry Fleytman <dmitry@daynix.com> wrote: > It turned out that the issue is not in the new code but in > pci.h helper functions used by the new code to fill DSN capability. Thanks for tracking this down. > Following patch fixes the problem: > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index ef6ba51..ee238ad > --- 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); > } > > I see from git blame that some time ago you did similar change in all other > pci_set_* pci_get_* functions except these two. > > Is there any specific reason you did not change these two functions then? The patches where I changed the other functions were cleanups to convert away from some legacy *_to_cpupu() functions, which were specifically intended to work with unaligned pointers (which is what the "u" indicated). I was doing the cleanups per-conversion-function, not per-callsite, and since these two functions aren't using a _to_cpupu() function but just _to_cpup() they wouldn't have shown up in my searches. In any case this is the correct fix, and we should probably audit the other uses of *_to_cpup and cpu_to_* -- I suspect many of them are working with possibly-unaligned pointers in to guest memory and we should replace them with ld*_*_p functions and get rid of the _to_cpup functions entirely. thanks -- PMM
On 2016年05月30日 00:45, Peter Maydell wrote: > On 29 May 2016 at 16:22, Dmitry Fleytman <dmitry@daynix.com> wrote: >> It turned out that the issue is not in the new code but in >> pci.h helper functions used by the new code to fill DSN capability. > Thanks for tracking this down. > >> Following patch fixes the problem: >> >> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h >> index ef6ba51..ee238ad >> --- 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); >> } >> >> I see from git blame that some time ago you did similar change in all other >> pci_set_* pci_get_* functions except these two. >> >> Is there any specific reason you did not change these two functions then? > The patches where I changed the other functions were cleanups > to convert away from some legacy *_to_cpupu() functions, which > were specifically intended to work with unaligned pointers > (which is what the "u" indicated). I was doing the cleanups > per-conversion-function, not per-callsite, and since these > two functions aren't using a _to_cpupu() function but just > _to_cpup() they wouldn't have shown up in my searches. > > In any case this is the correct fix, and we should probably > audit the other uses of *_to_cpup and cpu_to_* -- I suspect > many of them are working with possibly-unaligned > pointers in to guest memory and we should replace them with > ld*_*_p functions and get rid of the _to_cpup functions entirely. > > thanks > -- PMM git grep shows lots of places. Is it ok to send a new version of pull request with Dmitry's fix first? Thanks
On 30 May 2016 at 02:51, Jason Wang <jasowang@redhat.com> wrote: > git grep shows lots of places. Is it ok to send a new version of pull > request with Dmitry's fix first? Sure; I was talking about the audit as a later cleanup thing we should do at some point, not something to do immediately. -- PMM
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index ef6ba51..ee238ad --- 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); } I see from git blame that some time ago you did similar change in all other