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

[Anthony Liguori <anthony@codemonkey.ws>]

On 01/12/2012 02:06 PM, Peter Maydell wrote:
> On 12 January 2012 17:54, Anthony Liguori<aliguori@us.ibm.com>  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<aliguori@us.ibm.com>
>> ---
>>   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
>