* [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le @ 2014-03-10 19:43 Michael S. Tsirkin 2014-03-10 19:43 ` [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses Michael S. Tsirkin 2014-03-12 22:21 ` [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Richard Henderson 0 siblings, 2 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2014-03-10 19:43 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell it's a handy API for cases where we want to get size in bits as a parameter. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/qemu/bswap.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h index 0cb7c05..4489ad9 100644 --- a/include/qemu/bswap.h +++ b/include/qemu/bswap.h @@ -423,9 +423,4 @@ static inline unsigned long leul_to_cpu(unsigned long v) #endif } -#undef le_bswap -#undef be_bswap -#undef le_bswaps -#undef be_bswaps - #endif /* BSWAP_H */ -- MST ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses 2014-03-10 19:43 [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Michael S. Tsirkin @ 2014-03-10 19:43 ` Michael S. Tsirkin 2014-03-12 22:24 ` Richard Henderson 2014-03-12 22:25 ` [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables Richard Henderson 2014-03-12 22:21 ` [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Richard Henderson 1 sibling, 2 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2014-03-10 19:43 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, Anthony Liguori casting an unaligned address to e.g. uint32_t can trigger undefined behaviour in C. Replace cast + assignment with memcpy. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/i386/acpi-build.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b667d31..e08f514 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -466,9 +466,15 @@ static void acpi_align_size(GArray *blob, unsigned align) g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align)); } -/* Get pointer within table in a safe manner */ -#define ACPI_BUILD_PTR(table, size, off, type) \ - ((type *)(acpi_data_get_ptr(table, size, off, sizeof(type)))) +/* Set a value within table in a safe manner */ +#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \ + do { \ + uint64_t ACPI_BUILD_SET_LE_val = le_bswap(val, bits); \ + memcpy(acpi_data_get_ptr(table, size, off, \ + (bits) / BITS_PER_BYTE), \ + &ACPI_BUILD_SET_LE_val, \ + (bits) / BITS_PER_BYTE); \ + } while (0) static inline void *acpi_data_get_ptr(uint8_t *table_data, unsigned table_size, unsigned off, unsigned size) @@ -974,22 +980,17 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size) { - *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) = - cpu_to_le32(pci->w32.begin); + ACPI_BUILD_SET_LE(start, size, acpi_pci32_start[0], 32, pci->w32.begin); - *ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t) = - cpu_to_le32(pci->w32.end - 1); + ACPI_BUILD_SET_LE(start, size, acpi_pci32_end[0], 32, pci->w32.end - 1); if (pci->w64.end || pci->w64.begin) { - *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 1; - *ACPI_BUILD_PTR(start, size, acpi_pci64_start[0], uint64_t) = - cpu_to_le64(pci->w64.begin); - *ACPI_BUILD_PTR(start, size, acpi_pci64_end[0], uint64_t) = - cpu_to_le64(pci->w64.end - 1); - *ACPI_BUILD_PTR(start, size, acpi_pci64_length[0], uint64_t) = - cpu_to_le64(pci->w64.end - pci->w64.begin); + ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 1); + ACPI_BUILD_SET_LE(start, size, acpi_pci64_start[0], 64, pci->w64.begin); + ACPI_BUILD_SET_LE(start, size, acpi_pci64_end[0], 64, pci->w64.end - 1); + ACPI_BUILD_SET_LE(start, size, acpi_pci64_length[0], 64, pci->w64.end - pci->w64.begin); } else { - *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 0; + ACPI_BUILD_SET_LE(start, size, acpi_pci64_valid[0], 8, 0); } } -- MST ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses 2014-03-10 19:43 ` [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses Michael S. Tsirkin @ 2014-03-12 22:24 ` Richard Henderson 2014-03-12 22:25 ` [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables Richard Henderson 1 sibling, 0 replies; 7+ messages in thread From: Richard Henderson @ 2014-03-12 22:24 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel; +Cc: Peter Maydell, Anthony Liguori On 03/10/2014 12:43 PM, Michael S. Tsirkin wrote: > casting an unaligned address to e.g. > uint32_t can trigger undefined behaviour in C. > Replace cast + assignment with memcpy. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/i386/acpi-build.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index b667d31..e08f514 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -466,9 +466,15 @@ static void acpi_align_size(GArray *blob, unsigned align) > g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align)); > } > > -/* Get pointer within table in a safe manner */ > -#define ACPI_BUILD_PTR(table, size, off, type) \ > - ((type *)(acpi_data_get_ptr(table, size, off, sizeof(type)))) > +/* Set a value within table in a safe manner */ > +#define ACPI_BUILD_SET_LE(table, size, off, bits, val) \ > + do { \ > + uint64_t ACPI_BUILD_SET_LE_val = le_bswap(val, bits); \ > + memcpy(acpi_data_get_ptr(table, size, off, \ > + (bits) / BITS_PER_BYTE), \ > + &ACPI_BUILD_SET_LE_val, \ > + (bits) / BITS_PER_BYTE); \ > + } while (0) > > static inline void *acpi_data_get_ptr(uint8_t *table_data, unsigned table_size, > unsigned off, unsigned size) > @@ -974,22 +980,17 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) > > static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size) > { > - *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) = > - cpu_to_le32(pci->w32.begin); > + ACPI_BUILD_SET_LE(start, size, acpi_pci32_start[0], 32, pci->w32.begin); You're working too hard at this because you're using the wrong bswap interface. Also, you missed one. I'll follow up with a patch I wrote just recently, but hadn't yet gotten around to posting. r~ ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables 2014-03-10 19:43 ` [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses Michael S. Tsirkin 2014-03-12 22:24 ` Richard Henderson @ 2014-03-12 22:25 ` Richard Henderson 2014-03-12 23:26 ` Peter Maydell 1 sibling, 1 reply; 7+ messages in thread From: Richard Henderson @ 2014-03-12 22:25 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, aliguori, mst Hosts that don't support native unaligned stores will SIGBUS without additional help. Signed-off-by: Richard Henderson <rth@twiddle.net> --- hw/i386/acpi-build.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index b1a7ebb..d636115 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -886,22 +886,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size) { - *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) = - cpu_to_le32(pci->w32.begin); + /* Note that these pointers are unaligned, so we must use routines + that take care for unaligned stores on the host. */ - *ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t) = - cpu_to_le32(pci->w32.end - 1); + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t), + pci->w32.begin); + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t), + pci->w32.end - 1); if (pci->w64.end || pci->w64.begin) { - *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 1; - *ACPI_BUILD_PTR(start, size, acpi_pci64_start[0], uint64_t) = - cpu_to_le64(pci->w64.begin); - *ACPI_BUILD_PTR(start, size, acpi_pci64_end[0], uint64_t) = - cpu_to_le64(pci->w64.end - 1); - *ACPI_BUILD_PTR(start, size, acpi_pci64_length[0], uint64_t) = - cpu_to_le64(pci->w64.end - pci->w64.begin); + stb_p(ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t), 1); + stq_le_p(ACPI_BUILD_PTR(start, size, acpi_pci64_start[0], uint64_t), + pci->w64.begin); + stq_le_p(ACPI_BUILD_PTR(start, size, acpi_pci64_end[0], uint64_t), + pci->w64.end - 1); + stq_le_p(ACPI_BUILD_PTR(start, size, acpi_pci64_length[0], uint64_t), + pci->w64.end - pci->w64.begin); } else { - *ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t) = 0; + stb_p(ACPI_BUILD_PTR(start, size, acpi_pci64_valid[0], uint8_t), 0); } } @@ -930,8 +932,7 @@ build_ssdt(GArray *table_data, GArray *linker, patch_pci_windows(pci, ssdt_ptr, sizeof(ssdp_misc_aml)); - *(uint16_t *)(ssdt_ptr + *ssdt_isa_pest) = - cpu_to_le16(misc->pvpanic_port); + stw_le_p(ssdt_ptr + *ssdt_isa_pest, misc->pvpanic_port); { GArray *sb_scope = build_alloc_array(); -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables 2014-03-12 22:25 ` [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables Richard Henderson @ 2014-03-12 23:26 ` Peter Maydell 2014-03-13 13:30 ` Richard Henderson 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2014-03-12 23:26 UTC (permalink / raw) To: Richard Henderson; +Cc: QEMU Developers, Anthony Liguori, Michael S. Tsirkin On 12 March 2014 22:25, Richard Henderson <rth@twiddle.net> wrote: > Hosts that don't support native unaligned stores will SIGBUS > without additional help. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > hw/i386/acpi-build.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index b1a7ebb..d636115 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -886,22 +886,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) > > static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size) > { > - *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) = > - cpu_to_le32(pci->w32.begin); > + /* Note that these pointers are unaligned, so we must use routines > + that take care for unaligned stores on the host. */ > > - *ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t) = > - cpu_to_le32(pci->w32.end - 1); > + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t), > + pci->w32.begin); > + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t), > + pci->w32.end - 1); See the mail thread on Michael's original patch -- he didn't like this because we end up writing the size of the store twice (once in the "l" suffix in the function name and once by passing a type to the ACP_BUILD_PTR function. (That thread also has my personal preferred option in the comments, which uses stl_le_p and friends but via a wrapping macro.) Also you'll find this doesn't apply because a fix has already been committed on master... > - *(uint16_t *)(ssdt_ptr + *ssdt_isa_pest) = > - cpu_to_le16(misc->pvpanic_port); > + stw_le_p(ssdt_ptr + *ssdt_isa_pest, misc->pvpanic_port); Patch on list to fix this too I think. thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables 2014-03-12 23:26 ` Peter Maydell @ 2014-03-13 13:30 ` Richard Henderson 0 siblings, 0 replies; 7+ messages in thread From: Richard Henderson @ 2014-03-13 13:30 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers, Anthony Liguori, Michael S. Tsirkin On 03/12/2014 04:26 PM, Peter Maydell wrote: > On 12 March 2014 22:25, Richard Henderson <rth@twiddle.net> wrote: >> Hosts that don't support native unaligned stores will SIGBUS >> without additional help. >> >> Signed-off-by: Richard Henderson <rth@twiddle.net> >> --- >> hw/i386/acpi-build.c | 29 +++++++++++++++-------------- >> 1 file changed, 15 insertions(+), 14 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index b1a7ebb..d636115 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -886,22 +886,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state) >> >> static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size) >> { >> - *ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t) = >> - cpu_to_le32(pci->w32.begin); >> + /* Note that these pointers are unaligned, so we must use routines >> + that take care for unaligned stores on the host. */ >> >> - *ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t) = >> - cpu_to_le32(pci->w32.end - 1); >> + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_start[0], uint32_t), >> + pci->w32.begin); >> + stl_le_p(ACPI_BUILD_PTR(start, size, acpi_pci32_end[0], uint32_t), >> + pci->w32.end - 1); > > See the mail thread on Michael's original patch -- he didn't like > this because we end up writing the size of the store twice > (once in the "l" suffix in the function name and once by passing > a type to the ACP_BUILD_PTR function. I missed the original thread somewhere. > (That thread also has my personal preferred option in the comments, > which uses stl_le_p and friends but via a wrapping macro.) I'm in favour of any solution that doesn't duplicate the bswap logic, like the version I responded to did. r~ > > Also you'll find this doesn't apply because a fix has already been > committed on master... > >> - *(uint16_t *)(ssdt_ptr + *ssdt_isa_pest) = >> - cpu_to_le16(misc->pvpanic_port); >> + stw_le_p(ssdt_ptr + *ssdt_isa_pest, misc->pvpanic_port); > > Patch on list to fix this too I think. > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le 2014-03-10 19:43 [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Michael S. Tsirkin 2014-03-10 19:43 ` [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses Michael S. Tsirkin @ 2014-03-12 22:21 ` Richard Henderson 1 sibling, 0 replies; 7+ messages in thread From: Richard Henderson @ 2014-03-12 22:21 UTC (permalink / raw) To: Michael S. Tsirkin, qemu-devel; +Cc: peter.maydell On 03/10/2014 12:43 PM, Michael S. Tsirkin wrote: > it's a handy API for cases where we want to > get size in bits as a parameter. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/qemu/bswap.h | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h > index 0cb7c05..4489ad9 100644 > --- a/include/qemu/bswap.h > +++ b/include/qemu/bswap.h > @@ -423,9 +423,4 @@ static inline unsigned long leul_to_cpu(unsigned long v) > #endif > } > > -#undef le_bswap > -#undef be_bswap > -#undef le_bswaps > -#undef be_bswaps I don't like this because the bits "as parameter" must be constant in order for token pasting to work. Better to just use the existing function calls. r~ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-03-13 13:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-10 19:43 [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Michael S. Tsirkin 2014-03-10 19:43 ` [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses Michael S. Tsirkin 2014-03-12 22:24 ` Richard Henderson 2014-03-12 22:25 ` [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables Richard Henderson 2014-03-12 23:26 ` Peter Maydell 2014-03-13 13:30 ` Richard Henderson 2014-03-12 22:21 ` [Qemu-devel] [PATCH 1/2] bswap.h: export bswap_le Richard Henderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).