From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:37734) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlRKL-0005qD-7m for qemu-devel@nongnu.org; Thu, 12 Jan 2012 15:32:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RlRKI-0004hB-Kw for qemu-devel@nongnu.org; Thu, 12 Jan 2012 15:32:49 -0500 Received: from mail-iy0-f173.google.com ([209.85.210.173]:57270) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RlRKI-0004gk-G0 for qemu-devel@nongnu.org; Thu, 12 Jan 2012 15:32:46 -0500 Received: by iaeo4 with SMTP id o4so2004537iae.4 for ; Thu, 12 Jan 2012 12:32:44 -0800 (PST) Message-ID: <4F0F4361.10201@codemonkey.ws> Date: Thu, 12 Jan 2012 14:32:33 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1326390853-1892-1-git-send-email-aliguori@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] build: fix target_phys_addr_t to 64-bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Anthony Liguori , qemu-devel@nongnu.org On 01/12/2012 02:06 PM, Peter Maydell wrote: > On 12 January 2012 17:54, Anthony Liguori wrote: >> This simplifies the build quite a bit and improves the builds performance by >> not rebuilding many objects twice. >> >> There were a surprising number of places that had assumed wrong things about the >> size of target_phys_addr_t including that it was fixed at 32-bit and that it >> was identical to target_ulong. > > Up until now, in a lot of CPU-specific code it has been perfectly reasonable > to assume target_phys_addr_t was 32 bits. No, that's never been a reasonable thing to assume. > > >> >> Signed-off-by: Anthony Liguori >> --- >> Makefile | 2 +- >> Makefile.hw | 25 -------- >> Makefile.objs | 182 +++++++++++++++++++++++++++---------------------------- >> Makefile.target | 4 - >> configure | 13 +---- >> cpu-common.h | 8 --- >> dma.h | 2 - >> hw/a9mpcore.c | 4 +- >> hw/hw.h | 2 +- >> hw/intel-hda.c | 4 - >> hw/omap.h | 6 -- >> hw/pxa2xx_dma.c | 6 +- >> hw/pxa2xx_lcd.c | 6 +- >> hw/rtl8139.c | 4 - >> hw/sh_serial.c | 6 +- >> monitor.c | 21 ------ >> qemu-log.h | 4 +- >> targphys.h | 11 --- >> 18 files changed, 106 insertions(+), 204 deletions(-) >> delete mode 100644 Makefile.hw >> >> diff --git a/Makefile b/Makefile >> index 2bbc547..32a8ec6 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -197,7 +197,7 @@ $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN) $(GENERATED_HEADERS) >> >> qemu-ga$(EXESUF): qemu-ga.o $(qga-obj-y) $(qapi-obj-y) $(tools-obj-y) $(qobject-obj-y) $(version-obj-y) $(QGALIB_OBJ) >> >> -QEMULIBS=libhw32 libhw64 libuser libdis libdis-user >> +QEMULIBS=libuser libdis libdis-user >> >> clean: >> # avoid old build problems by removing potentially incorrect old files >> diff --git a/Makefile.hw b/Makefile.hw >> deleted file mode 100644 >> index 63eb7e4..0000000 >> --- a/Makefile.hw >> +++ /dev/null >> @@ -1,25 +0,0 @@ >> -# Makefile for qemu target independent devices. >> - >> -include ../config-host.mak >> -include ../config-all-devices.mak >> -include config.mak >> -include $(SRC_PATH)/rules.mak >> - >> -.PHONY: all >> - >> -$(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw) >> - >> -QEMU_CFLAGS+=-I.. >> -QEMU_CFLAGS += $(GLIB_CFLAGS) >> - >> -include $(SRC_PATH)/Makefile.objs >> - >> -all: $(hw-obj-y) >> -# Dummy command so that make thinks it has done something >> - @true >> - >> -clean: >> - rm -f *.o */*.o *.d */*.d *.a */*.a *~ */*~ >> - >> -# Include automatically generated dependency files >> --include $(wildcard *.d */*.d) >> diff --git a/Makefile.objs b/Makefile.objs >> index 4f6d26c..13a2281 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -179,118 +179,114 @@ user-obj-y += tcg-runtime.o host-utils.o >> user-obj-y += cutils.o cache-utils.o >> user-obj-y += $(trace-obj-y) >> >> -###################################################################### >> -# libhw >> - >> -hw-obj-y = >> -hw-obj-y += vl.o loader.o >> -hw-obj-$(CONFIG_VIRTIO) += virtio-console.o >> -hw-obj-y += usb-libhw.o >> -hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o >> -hw-obj-y += fw_cfg.o >> -hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o >> -hw-obj-$(CONFIG_PCI) += msix.o msi.o >> -hw-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o >> -hw-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o >> -hw-obj-y += watchdog.o >> -hw-obj-$(CONFIG_ISA_MMIO) += isa_mmio.o >> -hw-obj-$(CONFIG_ECC) += ecc.o >> -hw-obj-$(CONFIG_NAND) += nand.o >> -hw-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o >> -hw-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o >> - >> -hw-obj-$(CONFIG_M48T59) += m48t59.o >> -hw-obj-$(CONFIG_ESCC) += escc.o >> -hw-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o >> - >> -hw-obj-$(CONFIG_SERIAL) += serial.o >> -hw-obj-$(CONFIG_PARALLEL) += parallel.o >> -hw-obj-$(CONFIG_I8254) += i8254.o >> -hw-obj-$(CONFIG_PCSPK) += pcspk.o >> -hw-obj-$(CONFIG_PCKBD) += pckbd.o >> -hw-obj-$(CONFIG_USB_UHCI) += usb-uhci.o >> -hw-obj-$(CONFIG_USB_OHCI) += usb-ohci.o >> -hw-obj-$(CONFIG_USB_EHCI) += usb-ehci.o >> -hw-obj-$(CONFIG_FDC) += fdc.o >> -hw-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o >> -hw-obj-$(CONFIG_APM) += pm_smbus.o apm.o >> -hw-obj-$(CONFIG_DMA) += dma.o >> -hw-obj-$(CONFIG_HPET) += hpet.o >> -hw-obj-$(CONFIG_APPLESMC) += applesmc.o >> -hw-obj-$(CONFIG_SMARTCARD) += usb-ccid.o ccid-card-passthru.o >> -hw-obj-$(CONFIG_SMARTCARD_NSS) += ccid-card-emulated.o >> -hw-obj-$(CONFIG_USB_REDIR) += usb-redir.o >> -hw-obj-$(CONFIG_I8259) += i8259.o >> +common-obj-y += vl.o loader.o >> +common-obj-$(CONFIG_VIRTIO) += virtio-console.o >> +common-obj-y += usb-libhw.o >> +common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o >> +common-obj-y += fw_cfg.o >> +common-obj-$(CONFIG_PCI) += pci.o pci_bridge.o >> +common-obj-$(CONFIG_PCI) += msix.o msi.o >> +common-obj-$(CONFIG_PCI) += pci_host.o pcie_host.o >> +common-obj-$(CONFIG_PCI) += ioh3420.o xio3130_upstream.o xio3130_downstream.o >> +common-obj-y += watchdog.o >> +common-obj-$(CONFIG_ISA_MMIO) += isa_mmio.o >> +common-obj-$(CONFIG_ECC) += ecc.o >> +common-obj-$(CONFIG_NAND) += nand.o >> +common-obj-$(CONFIG_PFLASH_CFI01) += pflash_cfi01.o >> +common-obj-$(CONFIG_PFLASH_CFI02) += pflash_cfi02.o >> + >> +common-obj-$(CONFIG_M48T59) += m48t59.o >> +common-obj-$(CONFIG_ESCC) += escc.o >> +common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o >> + >> +common-obj-$(CONFIG_SERIAL) += serial.o >> +common-obj-$(CONFIG_PARALLEL) += parallel.o >> +common-obj-$(CONFIG_I8254) += i8254.o >> +common-obj-$(CONFIG_PCSPK) += pcspk.o >> +common-obj-$(CONFIG_PCKBD) += pckbd.o >> +common-obj-$(CONFIG_USB_UHCI) += usb-uhci.o >> +common-obj-$(CONFIG_USB_OHCI) += usb-ohci.o >> +common-obj-$(CONFIG_USB_EHCI) += usb-ehci.o >> +common-obj-$(CONFIG_FDC) += fdc.o >> +common-obj-$(CONFIG_ACPI) += acpi.o acpi_piix4.o >> +common-obj-$(CONFIG_APM) += pm_smbus.o apm.o >> +common-obj-$(CONFIG_DMA) += dma.o >> +common-obj-$(CONFIG_HPET) += hpet.o >> +common-obj-$(CONFIG_APPLESMC) += applesmc.o >> +common-obj-$(CONFIG_SMARTCARD) += usb-ccid.o ccid-card-passthru.o >> +common-obj-$(CONFIG_SMARTCARD_NSS) += ccid-card-emulated.o >> +common-obj-$(CONFIG_USB_REDIR) += usb-redir.o >> +common-obj-$(CONFIG_I8259) += i8259.o > > Why is all this makefile frobbery in the same patch as > the things fixing assumptions about the size of the > type and the actual change to the size of the type? There was a good reason but I can't remember it. I could probably remove this and just do common-obj-y += $(hw-obj-y) >> diff --git a/hw/a9mpcore.c b/hw/a9mpcore.c >> index 3ef0e13..fece100 100644 >> --- a/hw/a9mpcore.c >> +++ b/hw/a9mpcore.c >> @@ -87,8 +87,8 @@ static void a9_scu_write(void *opaque, target_phys_addr_t offset, >> mask = 0xffffffff; >> break; >> default: >> - fprintf(stderr, "Invalid size %u in write to a9 scu register %x\n", >> - size, offset); >> + fprintf(stderr, "Invalid size %u in write to a9 scu register " >> + TARGET_FMT_plx "\n", size, offset); >> return; >> } > > I don't like this. When target_phys_addr_t was 32 bits, then > using TARGET_FMT_plx to print offsets into devices isn't > too unreasonable as you only get 8 hex digits. If you expand > to 64 bits then suddenly all these offsets which are actually > really typically small numbers get printed as 16 hex digits, > which I think looks bad. Then cast it to a 32-bit number and print it however you like. > I'm dubious about the utility of being able to hand a 64 bit > offset into an device IO routine anyway. I'm pretty sure it's a hard requirement for PCI devices since the PCI bus is 64-bit and devices would get the full address over the bus of the I/O operation. But target_phys_addr_t is definitely the wrong type for the memory API (because the width should have nothing to do with a target). >> -# if TARGET_PHYS_ADDR_BITS == 32 >> -# define OMAP_FMT_plx "%#08x" >> -# elif TARGET_PHYS_ADDR_BITS == 64 >> # define OMAP_FMT_plx "%#08" PRIx64 >> -# else >> -# error TARGET_PHYS_ADDR_BITS undefined >> -# endif > > Same comments about way too many digits in log messages. > (If this patch or similar goes in at some later date we can > throw in a cleanup patch to just drop the OMAP_FMT_plx macro.) You can switch any printf you care about to PRIx64 and then you can use whatever specifier you want. I don't think changing the semantics of TARGET_FMT_plx is reasonable in this patch. >> uint32_t omap_badwidth_read8(void *opaque, target_phys_addr_t addr); >> void omap_badwidth_write8(void *opaque, target_phys_addr_t addr, >> diff --git a/hw/pxa2xx_dma.c b/hw/pxa2xx_dma.c >> index cb28107..ffb391d 100644 >> --- a/hw/pxa2xx_dma.c >> +++ b/hw/pxa2xx_dma.c >> @@ -512,9 +512,9 @@ static VMStateDescription vmstate_pxa2xx_dma_chan = { >> .minimum_version_id = 1, >> .minimum_version_id_old = 1, >> .fields = (VMStateField[]) { >> - VMSTATE_UINTTL(descr, PXA2xxDMAChannel), >> - VMSTATE_UINTTL(src, PXA2xxDMAChannel), >> - VMSTATE_UINTTL(dest, PXA2xxDMAChannel), >> + VMSTATE_UINT64(descr, PXA2xxDMAChannel), >> + VMSTATE_UINT64(src, PXA2xxDMAChannel), >> + VMSTATE_UINT64(dest, PXA2xxDMAChannel), > > Isn't this a format change demanding a version bump? Yes. >> VMSTATE_UINT32(cmd, PXA2xxDMAChannel), >> VMSTATE_UINT32(state, PXA2xxDMAChannel), >> VMSTATE_INT32(request, PXA2xxDMAChannel), >> diff --git a/hw/pxa2xx_lcd.c b/hw/pxa2xx_lcd.c >> index 5dd4ef0..a84cd77 100644 >> --- a/hw/pxa2xx_lcd.c >> +++ b/hw/pxa2xx_lcd.c >> @@ -921,11 +921,11 @@ static const VMStateDescription vmstate_dma_channel = { >> .minimum_version_id = 0, >> .minimum_version_id_old = 0, >> .fields = (VMStateField[]) { >> - VMSTATE_UINTTL(branch, struct DMAChannel), >> + VMSTATE_UINT64(branch, struct DMAChannel), >> VMSTATE_UINT8(up, struct DMAChannel), >> VMSTATE_BUFFER(pbuffer, struct DMAChannel), >> - VMSTATE_UINTTL(descriptor, struct DMAChannel), >> - VMSTATE_UINTTL(source, struct DMAChannel), >> + VMSTATE_UINT64(descriptor, struct DMAChannel), >> + VMSTATE_UINT64(source, struct DMAChannel), > > Ditto. Ack. >> diff --git a/hw/sh_serial.c b/hw/sh_serial.c >> index 43b0eb1..c612a90 100644 >> --- a/hw/sh_serial.c >> +++ b/hw/sh_serial.c >> @@ -186,7 +186,8 @@ static void sh_serial_write(void *opaque, target_phys_addr_t offs, >> } >> } >> >> - fprintf(stderr, "sh_serial: unsupported write to 0x%02x\n", offs); >> + fprintf(stderr, "sh_serial: unsupported write to " TARGET_FMT_plx "\n", >> + offs); > > And here you can see that you've clearly just messed up the intended > two-hex-digits only printing. This is the only correct solution without doing away with TARGET_FMT_plx entirely. > >> abort(); >> } >> >> @@ -287,7 +288,8 @@ static uint64_t sh_serial_read(void *opaque, target_phys_addr_t offs, >> #endif >> >> if (ret& ~((1<< 16) - 1)) { >> - fprintf(stderr, "sh_serial: unsupported read from 0x%02x\n", offs); >> + fprintf(stderr, "sh_serial: unsupported read from 0x" >> + TARGET_FMT_plx "\n", offs); >> abort(); >> } >> >> diff --git a/monitor.c b/monitor.c >> index 7334401..be3d7f4 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -1274,26 +1274,6 @@ static void do_print(Monitor *mon, const QDict *qdict) >> int format = qdict_get_int(qdict, "format"); >> target_phys_addr_t val = qdict_get_int(qdict, "val"); >> >> -#if TARGET_PHYS_ADDR_BITS == 32 >> - switch(format) { >> - case 'o': >> - monitor_printf(mon, "%#o", val); >> - break; >> - case 'x': >> - monitor_printf(mon, "%#x", val); >> - break; >> - case 'u': >> - monitor_printf(mon, "%u", val); >> - break; >> - default: >> - case 'd': >> - monitor_printf(mon, "%d", val); >> - break; >> - case 'c': >> - monitor_printc(mon, val); >> - break; >> - } >> -#else >> switch(format) { >> case 'o': >> monitor_printf(mon, "%#" PRIo64, val); >> @@ -1312,7 +1292,6 @@ static void do_print(Monitor *mon, const QDict *qdict) >> monitor_printc(mon, val); >> break; >> } >> -#endif >> monitor_printf(mon, "\n"); >> } >> >> diff --git a/qemu-log.h b/qemu-log.h >> index fccfb110..7c9180c 100644 >> --- a/qemu-log.h >> +++ b/qemu-log.h >> @@ -51,6 +51,7 @@ extern int loglevel; >> /* Special cases: */ >> >> /* cpu_dump_state() logging functions: */ >> +#ifdef NEED_CPU_H >> #define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f)); >> #define log_cpu_state_mask(b, env, f) do { \ >> if (loglevel& (b)) log_cpu_state((env), (f)); \ >> @@ -64,8 +65,7 @@ extern int loglevel; >> >> /* page_dump() output to the log file: */ >> #define log_page_dump() page_dump(logfile) >> - >> - >> +#endif > > Why does this #ifdef become necessary? (not saying it isn't, > just wondering) GCC poisoning is very aggressive and actually will complain because cpu_dump_state() is used in a macro (even though it's not instantiated anywhere). Regards, Anthony Liguori > >> >> /* Maintenance: */ >> >> diff --git a/targphys.h b/targphys.h >> index 95648d6..8c4928a 100644 >> --- a/targphys.h >> +++ b/targphys.h >> @@ -3,19 +3,8 @@ >> #ifndef TARGPHYS_H >> #define TARGPHYS_H >> >> -#ifdef TARGET_PHYS_ADDR_BITS >> -/* target_phys_addr_t is the type of a physical address (its size can >> - be different from 'target_ulong'). */ >> - >> -#if TARGET_PHYS_ADDR_BITS == 32 >> -typedef uint32_t target_phys_addr_t; >> -#define TARGET_PHYS_ADDR_MAX UINT32_MAX >> -#define TARGET_FMT_plx "%08x" >> -#elif TARGET_PHYS_ADDR_BITS == 64 >> typedef uint64_t target_phys_addr_t; >> #define TARGET_PHYS_ADDR_MAX UINT64_MAX >> #define TARGET_FMT_plx "%016" PRIx64 >> -#endif >> -#endif >> >> #endif >> -- >> 1.7.4.1 > > > -- PMM >