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: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] build: fix target_phys_addr_t to 64-bit
Date: Thu, 12 Jan 2012 20:06:43 +0000

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.


>
> 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?

>
>  # PPC devices
> -hw-obj-$(CONFIG_PREP_PCI) += prep_pci.o
> +common-obj-$(CONFIG_PREP_PCI) += prep_pci.o
>  # Mac shared devices
> -hw-obj-$(CONFIG_MACIO) += macio.o
> -hw-obj-$(CONFIG_CUDA) += cuda.o
> -hw-obj-$(CONFIG_ADB) += adb.o
> -hw-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
> -hw-obj-$(CONFIG_MAC_DBDMA) += mac_dbdma.o
> +common-obj-$(CONFIG_MACIO) += macio.o
> +common-obj-$(CONFIG_CUDA) += cuda.o
> +common-obj-$(CONFIG_ADB) += adb.o
> +common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
> +common-obj-$(CONFIG_MAC_DBDMA) += mac_dbdma.o
>  # OldWorld PowerMac
> -hw-obj-$(CONFIG_HEATHROW_PIC) += heathrow_pic.o
> -hw-obj-$(CONFIG_GRACKLE_PCI) += grackle_pci.o
> +common-obj-$(CONFIG_HEATHROW_PIC) += heathrow_pic.o
> +common-obj-$(CONFIG_GRACKLE_PCI) += grackle_pci.o
>  # NewWorld PowerMac
> -hw-obj-$(CONFIG_UNIN_PCI) += unin_pci.o
> -hw-obj-$(CONFIG_DEC_PCI) += dec_pci.o
> +common-obj-$(CONFIG_UNIN_PCI) += unin_pci.o
> +common-obj-$(CONFIG_DEC_PCI) += dec_pci.o
>  # PowerPC E500 boards
> -hw-obj-$(CONFIG_PPCE500_PCI) += ppce500_pci.o
> +common-obj-$(CONFIG_PPCE500_PCI) += ppce500_pci.o
>
>  # MIPS devices
> -hw-obj-$(CONFIG_PIIX4) += piix4.o
> -hw-obj-$(CONFIG_G364FB) += g364fb.o
> +common-obj-$(CONFIG_PIIX4) += piix4.o
> +common-obj-$(CONFIG_G364FB) += g364fb.o
>
>  # PCI watchdog devices
> -hw-obj-$(CONFIG_PCI) += wdt_i6300esb.o
> +common-obj-$(CONFIG_PCI) += wdt_i6300esb.o
>
> -hw-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
> +common-obj-$(CONFIG_PCI) += pcie.o pcie_aer.o pcie_port.o
>
>  # PCI network cards
> -hw-obj-$(CONFIG_NE2000_PCI) += ne2000.o
> -hw-obj-$(CONFIG_EEPRO100_PCI) += eepro100.o
> -hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
> -hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
> -hw-obj-$(CONFIG_E1000_PCI) += e1000.o
> -hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
> -
> -hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
> -hw-obj-$(CONFIG_LAN9118) += lan9118.o
> -hw-obj-$(CONFIG_NE2000_ISA) += ne2000-isa.o
> -hw-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
> +common-obj-$(CONFIG_NE2000_PCI) += ne2000.o
> +common-obj-$(CONFIG_EEPRO100_PCI) += eepro100.o
> +common-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
> +common-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
> +common-obj-$(CONFIG_E1000_PCI) += e1000.o
> +common-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
> +
> +common-obj-$(CONFIG_SMC91C111) += smc91c111.o
> +common-obj-$(CONFIG_LAN9118) += lan9118.o
> +common-obj-$(CONFIG_NE2000_ISA) += ne2000-isa.o
> +common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>
>  # IDE
> -hw-obj-$(CONFIG_IDE_CORE) += ide/core.o ide/atapi.o
> -hw-obj-$(CONFIG_IDE_QDEV) += ide/qdev.o
> -hw-obj-$(CONFIG_IDE_PCI) += ide/pci.o
> -hw-obj-$(CONFIG_IDE_ISA) += ide/isa.o
> -hw-obj-$(CONFIG_IDE_PIIX) += ide/piix.o
> -hw-obj-$(CONFIG_IDE_CMD646) += ide/cmd646.o
> -hw-obj-$(CONFIG_IDE_MACIO) += ide/macio.o
> -hw-obj-$(CONFIG_IDE_VIA) += ide/via.o
> -hw-obj-$(CONFIG_AHCI) += ide/ahci.o
> -hw-obj-$(CONFIG_AHCI) += ide/ich.o
> +common-obj-$(CONFIG_IDE_CORE) += ide/core.o ide/atapi.o
> +common-obj-$(CONFIG_IDE_QDEV) += ide/qdev.o
> +common-obj-$(CONFIG_IDE_PCI) += ide/pci.o
> +common-obj-$(CONFIG_IDE_ISA) += ide/isa.o
> +common-obj-$(CONFIG_IDE_PIIX) += ide/piix.o
> +common-obj-$(CONFIG_IDE_CMD646) += ide/cmd646.o
> +common-obj-$(CONFIG_IDE_MACIO) += ide/macio.o
> +common-obj-$(CONFIG_IDE_VIA) += ide/via.o
> +common-obj-$(CONFIG_AHCI) += ide/ahci.o
> +common-obj-$(CONFIG_AHCI) += ide/ich.o
>
>  # SCSI layer
> -hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
> -hw-obj-$(CONFIG_ESP) += esp.o
> +common-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
> +common-obj-$(CONFIG_ESP) += esp.o
>
> -hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
> -hw-obj-y += qdev-addr.o container.o
> +common-obj-y += dma-helpers.o sysbus.o isa-bus.o
> +common-obj-y += qdev-addr.o container.o
>
>  # VGA
> -hw-obj-$(CONFIG_VGA_PCI) += vga-pci.o
> -hw-obj-$(CONFIG_VGA_ISA) += vga-isa.o
> -hw-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
> -hw-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
> -hw-obj-$(CONFIG_VMMOUSE) += vmmouse.o
> +common-obj-$(CONFIG_VGA_PCI) += vga-pci.o
> +common-obj-$(CONFIG_VGA_ISA) += vga-isa.o
> +common-obj-$(CONFIG_VGA_ISA_MM) += vga-isa-mm.o
> +common-obj-$(CONFIG_VMWARE_VGA) += vmware_vga.o
> +common-obj-$(CONFIG_VMMOUSE) += vmmouse.o
>
> -hw-obj-$(CONFIG_RC4030) += rc4030.o
> -hw-obj-$(CONFIG_DP8393X) += dp8393x.o
> -hw-obj-$(CONFIG_DS1225Y) += ds1225y.o
> -hw-obj-$(CONFIG_MIPSNET) += mipsnet.o
> +common-obj-$(CONFIG_RC4030) += rc4030.o
> +common-obj-$(CONFIG_DP8393X) += dp8393x.o
> +common-obj-$(CONFIG_DS1225Y) += ds1225y.o
> +common-obj-$(CONFIG_MIPSNET) += mipsnet.o
>
>  # Sound
>  sound-obj-y =
> @@ -303,7 +299,7 @@ sound-obj-$(CONFIG_CS4231A) += cs4231a.o
>  sound-obj-$(CONFIG_HDA) += intel-hda.o hda-audio.o
>
>  adlib.o fmopl.o: QEMU_CFLAGS += -DBUILD_Y8950=0
> -hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
> +common-obj-$(CONFIG_SOUND) += $(sound-obj-y)
>
>  9pfs-nested-$(CONFIG_VIRTFS)  = virtio-9p.o
>  9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-local.o virtio-9p-xattr.o
> @@ -313,7 +309,7 @@ hw-obj-$(CONFIG_SOUND) += $(sound-obj-y)
>  9pfs-nested-$(CONFIG_OPEN_BY_HANDLE) +=  virtio-9p-handle.o
>  9pfs-nested-$(CONFIG_VIRTFS) += virtio-9p-proxy.o
>
> -hw-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
> +common-obj-$(CONFIG_REALLY_VIRTFS) += $(addprefix 9pfs/, $(9pfs-nested-y))
>  $(addprefix 9pfs/, $(9pfs-nested-y)): QEMU_CFLAGS+=$(GLIB_CFLAGS)
>
>
> diff --git a/Makefile.target b/Makefile.target
> index 06d79b8..1a832db 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -9,9 +9,6 @@ include ../config-host.mak
>  include config-devices.mak
>  include config-target.mak
>  include $(SRC_PATH)/rules.mak
> -ifneq ($(HWDIR),)
> -include $(HWDIR)/config.mak
> -endif
>
>  TARGET_PATH=$(SRC_PATH)/target-$(TARGET_BASE_ARCH)
>  $(call set-vpath, $(SRC_PATH):$(TARGET_PATH):$(SRC_PATH)/hw)
> @@ -392,7 +389,6 @@ $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): 
> $(GENERATED_HEADERS)
>  obj-y += $(addprefix ../, $(common-obj-y))
>  obj-y += $(addprefix ../libdis/, $(libdis-y))
>  obj-y += $(libobj-y)
> -obj-y += $(addprefix $(HWDIR)/, $(hw-obj-y))
>  obj-y += $(addprefix ../, $(trace-obj-y))
>
>  endif # CONFIG_SOFTMMU
> diff --git a/configure b/configure
> index eb9a01d..c184465 100755
> --- a/configure
> +++ b/configure
> @@ -3574,8 +3574,6 @@ if test "$target_softmmu" = "yes" ; then
>   echo "TARGET_PHYS_ADDR_BITS=$target_phys_bits" >> $config_target_mak
>   echo "CONFIG_SOFTMMU=y" >> $config_target_mak
>   echo "LIBS+=$libs_softmmu $target_libs_softmmu" >> $config_target_mak
> -  echo "HWDIR=../libhw$target_phys_bits" >> $config_target_mak
> -  echo "subdir-$target: subdir-libhw$target_phys_bits" >> $config_host_mak
>  fi
>  if test "$target_user_only" = "yes" ; then
>   echo "CONFIG_USER_ONLY=y" >> $config_target_mak
> @@ -3780,7 +3778,7 @@ DIRS="$DIRS pc-bios/spapr-rtas"
>  DIRS="$DIRS roms/seabios roms/vgabios"
>  DIRS="$DIRS fsdev ui"
>  DIRS="$DIRS qapi qapi-generated"
> -DIRS="$DIRS qga trace"
> +DIRS="$DIRS qga trace ide 9pfs"
>  FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
>  FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
>  FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
> @@ -3815,15 +3813,6 @@ for rom in seabios vgabios ; do
>     echo "LD=$ld" >> $config_mak
>  done
>
> -for hwlib in 32 64; do
> -  d=libhw$hwlib
> -  mkdir -p $d
> -  mkdir -p $d/ide
> -  symlink $source_path/Makefile.hw $d/Makefile
> -  mkdir -p $d/9pfs
> -  echo "QEMU_CFLAGS+=-DTARGET_PHYS_ADDR_BITS=$hwlib" > $d/config.mak
> -done
> -
>  if [ "$source_path" != `pwd` ]; then
>     # out of tree build
>     mkdir -p libcacard
> diff --git a/cpu-common.h b/cpu-common.h
> index a40c57d..85a3b35 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -3,9 +3,7 @@
>
>  /* CPU interfaces that are target independent.  */
>
> -#ifdef TARGET_PHYS_ADDR_BITS
>  #include "targphys.h"
> -#endif
>
>  #ifndef NEED_CPU_H
>  #include "poison.h"
> @@ -23,15 +21,9 @@ enum device_endian {
>  };
>
>  /* address in the RAM (different from a physical address) */
> -#if defined(CONFIG_XEN_BACKEND) && TARGET_PHYS_ADDR_BITS == 64
>  typedef uint64_t ram_addr_t;
>  #  define RAM_ADDR_MAX UINT64_MAX
>  #  define RAM_ADDR_FMT "%" PRIx64
> -#else
> -typedef unsigned long ram_addr_t;
> -#  define RAM_ADDR_MAX ULONG_MAX
> -#  define RAM_ADDR_FMT "%lx"
> -#endif
>
>  /* memory API */
>
> diff --git a/dma.h b/dma.h
> index a13209d..03c6843 100644
> --- a/dma.h
> +++ b/dma.h
> @@ -17,7 +17,6 @@
>
>  typedef struct ScatterGatherEntry ScatterGatherEntry;
>
> -#if defined(TARGET_PHYS_ADDR_BITS)
>  typedef target_phys_addr_t dma_addr_t;
>
>  #define DMA_ADDR_FMT TARGET_FMT_plx
> @@ -42,7 +41,6 @@ struct QEMUSGList {
>  void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint);
>  void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len);
>  void qemu_sglist_destroy(QEMUSGList *qsg);
> -#endif
>
>  typedef BlockDriverAIOCB *DMAIOFunc(BlockDriverState *bs, int64_t sector_num,
>                                  QEMUIOVector *iov, int nb_sectors,
> 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.

I'm dubious about the utility of being able to hand a 64 bit
offset into an device IO routine anyway.

> diff --git a/hw/hw.h b/hw/hw.h
> index 932014a..8d0c7c9 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -4,7 +4,7 @@
>
>  #include "qemu-common.h"
>
> -#if defined(TARGET_PHYS_ADDR_BITS) && !defined(NEED_CPU_H)
> +#if !defined(NEED_CPU_H)
>  #include "cpu-common.h"
>  #endif
>
> diff --git a/hw/intel-hda.c b/hw/intel-hda.c
> index 10769e0..322440e 100644
> --- a/hw/intel-hda.c
> +++ b/hw/intel-hda.c
> @@ -215,13 +215,9 @@ static target_phys_addr_t intel_hda_addr(uint32_t lbase, 
> uint32_t ubase)
>  {
>     target_phys_addr_t addr;
>
> -#if TARGET_PHYS_ADDR_BITS == 32
> -    addr = lbase;
> -#else
>     addr = ubase;
>     addr <<= 32;
>     addr |= lbase;
> -#endif
>     return addr;
>  }
>
> diff --git a/hw/omap.h b/hw/omap.h
> index 60fa34c..a99fb7e 100644
> --- a/hw/omap.h
> +++ b/hw/omap.h
> @@ -951,13 +951,7 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion 
> *sysmem,
>                 unsigned long sdram_size,
>                 const char *core);
>
> -# 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.)

>
>  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?

>         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.

>         VMSTATE_UINT32(id, struct DMAChannel),
>         VMSTATE_UINT32(command, struct DMAChannel),
>         VMSTATE_END_OF_LIST()
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 0ae9f57..e8eca15 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -799,11 +799,7 @@ static void rtl8139_write_buffer(RTL8139State *s, const 
> void *buf, int size)
>  #define MIN_BUF_SIZE 60
>  static inline dma_addr_t rtl8139_addr64(uint32_t low, uint32_t high)
>  {
> -#if TARGET_PHYS_ADDR_BITS > 32
>     return low | ((target_phys_addr_t)high << 32);
> -#else
> -    return low;
> -#endif
>  }
>
>  static int rtl8139_can_receive(VLANClientState *nc)
> 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.


>     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)


>
>  /* 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]