qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] build: fix target_phys_addr_t to 64-bit

From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH] build: fix target_phys_addr_t to 64-bit
Date: Thu, 12 Jan 2012 14:32:33 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 
On 01/12/2012 02:06 PM, Peter Maydell wrote:

On 12 January 2012 17:54, Anthony Liguori<address@hidden>  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<address@hidden>
---
  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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]