bswap.h: export bswap_le
PATCH 1/2 bswap.h: export bswap_le
it's a handy API for cases where we want to
get size in bits as a parameter.
--- include/qemu/bswap.h | 5 ----- 1 file changed, 5 deletions(-)
include/qemu/bswap.h +0 -5
@@ -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 */
Signature · 1 line
MST
PATCH 2/2 acpi-build: don't access unaligned addresses
casting an unaligned address to e.g.uint32_t can trigger undefined behaviour in C. Replace cast + assignment with memcpy.
--- hw/i386/acpi-build.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
hw/i386/acpi-build.c +16 -15
@@ -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 ); } }
Signature · 1 line
MST
Re: [Qemu-devel] [PATCH 2/2] acpi-build: don't access unaligned addresses
Quoted 34 lines · Michael S. Tsirkin
casting an unaligned address to e.g.
uint32_tcan trigger undefined behaviour in C. Replace cast + assignment with memcpy.Reported-by Peter Maydell <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>--- hw/i386/acpi-build.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)hw/i386/acpi-build.c +9
-3@@ -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)
hw/i386/acpi-build.c static void build_pci_bus_end(PCIBus *bus, void *bus_state) · line 983
Quoted 6 lines
hw/i386/acpi-build.c +1
-2@@ -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~
PATCH 1/1 hw/i386: Use unaligned store functions building acpi tables
In reply to Michael S. TsirkinHosts that don't support native unaligned stores will SIGBUS without additional help.
--- hw/i386/acpi-build.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)
hw/i386/acpi-build.c +15 -14
@@ -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 ();
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables
Quoted 17 lines · Richard Henderson
Hosts that don't support native unaligned stores will SIGBUS without additional help.
Signed-off-by Richard Henderson <[email protected]>--- hw/i386/acpi-build.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)hw/i386/acpi-build.c +0
-1@@ -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) =
hw/i386/acpi-build.c static void build_pci_bus_end(PCIBus *bus, void *bus_state) · line 895
Quoted 10 lines
hw/i386/acpi-build.c +6
-3-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...
hw/i386/acpi-build.c build_ssdt(GArray *table_data, GArray *linker, · line 935
Quoted 3 lines
hw/i386/acpi-build.c +1
-2- *(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
Re: [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables
Quoted 22 lines · Peter Maydell
Quoted 21 lines · Richard Henderson
Hosts that don't support native unaligned stores will SIGBUS without additional help.
Signed-off-by Richard Henderson <[email protected]>--- hw/i386/acpi-build.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)hw/i386/acpi-build.c +2
-2@@ -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. */
Quoted 11 lines
hw/i386/acpi-build.c static void build_pci_bus_end(PCIBus *bus, void *bus_state) · line 895
Quoted 6 lines
hw/i386/acpi-build.c +4
-2- *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_PTRfunction.
I missed the original thread somewhere.
Quoted 2 lines
(That thread also has my personal preferred option in the comments, which uses
stl_le_pand 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~
Quoted 13 lines
Also you'll find this doesn't apply because a fix has already been committed on master...
hw/i386/acpi-build.c build_ssdt(GArray *table_data, GArray *linker, · line 935
Quoted 3 lines
hw/i386/acpi-build.c +1
-2- *(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
Quoted 13 lines · Michael S. Tsirkin
it's a handy API for cases where we want to
get size in bits as a parameter.Signed-off-by Michael S. Tsirkin <[email protected]>--- include/qemu/bswap.h | 5 ----- 1 file changed, 5 deletions(-)include/qemu/bswap.h +0
-0
include/qemu/bswap.h static inline unsigned long leul_to_cpu(unsigned long v) · line 429
Quoted 7 lines
include/qemu/bswap.h +0
-4@@ -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~