From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WO5ne-0001jM-Vq for qemu-devel@nongnu.org; Thu, 13 Mar 2014 09:36:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WO5nY-0001bH-RM for qemu-devel@nongnu.org; Thu, 13 Mar 2014 09:35:54 -0400 Received: from mail-qc0-x232.google.com ([2607:f8b0:400d:c01::232]:34503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WO5nY-0001bA-Ly for qemu-devel@nongnu.org; Thu, 13 Mar 2014 09:35:48 -0400 Received: by mail-qc0-f178.google.com with SMTP id i8so1073858qcq.23 for ; Thu, 13 Mar 2014 06:35:48 -0700 (PDT) Sender: Richard Henderson Message-ID: <5321B2D9.1000704@twiddle.net> Date: Thu, 13 Mar 2014 06:30:01 -0700 From: Richard Henderson MIME-Version: 1.0 References: <1394480575-3698-2-git-send-email-mst@redhat.com> <1394663146-24552-1-git-send-email-rth@twiddle.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/i386: Use unaligned store functions building acpi tables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 wrote: >> Hosts that don't support native unaligned stores will SIGBUS >> without additional help. >> >> Signed-off-by: Richard Henderson >> --- >> 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 >