Re: [PATCH 4/6] exec/cpu-common: Do not restrict CPU to 32-bit memory access maximum

[Peter Maydell]

On Sun, 31 May 2020 at 18:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Most CPUs can do 64-bit operations. Update the CPUReadMemoryFunc
> and CPUWriteMemoryFunc prototypes.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/exec/cpu-common.h |  4 ++--
>  hw/usb/hcd-musb.c         | 12 ++++++------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index b47e5630e7..5ac766e3b6 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -43,8 +43,8 @@ extern ram_addr_t ram_size;
>
>  /* memory API */
>
> -typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint32_t value);
> -typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
> +typedef void CPUWriteMemoryFunc(void *opaque, hwaddr addr, uint64_t value);
> +typedef uint64_t CPUReadMemoryFunc(void *opaque, hwaddr addr);

I don't think the type of these functions has anything to do with the
CPU's capabilities, does it? The typedefs are a legacy remnant from before
the conversion to MemoryRegions:
 * before MemoryRegions, devices provided separate functions for
   byte/word/long reads and writes (64-bit writes were simply
   impossible with the ancient APIs, which required a 3-element
   function pointer array for read and the same for write)
 * the initial MemoryRegion conversion introduced the new-style
   "one read/write fn for all widths" APIs, but also supported
   old-style six-function devices, for ease of conversion, using
   MemoryRegionOps::old_mmio.
 * in commit 62a0db942dec6ebfe we were finally able to drop the
   old_mmio (having changed over the last devices using old-style).
   (I see I forgot to delete the now-unused MemoryRegionMmio typedef.)

The only remaining user of these typedefs is hw/usb/hcd-musb.c,
which is still not converted to QOM/qdev. It uses them to allow
its one user (hw/usb/tusb6010.c) to perform reads/writes on the
underlying musb registers.

There's no point in changing these typedefs to pass or return
a 64-bit data type, because their sole use is in the musb_read[]
and musb_write[] arrays, which only allow for 1, 2 or 4 byte
accesses, depending on which array element you use.

Possible cleanups here:
Easy:
 * delete the unused MmeoryRegionMmio
 * move these typedefs into include/hw/usb.h and rename them
   to MUSBReadFunc and MUSBWriteFunc, since that's all they're
   used for now
Tricky:
 * convert the hw/usb/hcd-musb.c code to QOM/qdev, which would
   include refactoring the current musb_read/musb_write so that
   instead of the tusb6010.c code calling function entries in these
   arrays the hcd-musb.c code exposed a MemoryRegion; the tusb6010
   code would access it via memory_region_dispatch_read/write

thanks
-- PMM