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

[Philippe Mathieu-Daudé]

On 5/31/20 9:41 PM, Peter Maydell wrote:
> 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.

Indeed you are correct, I have been short-sighted.

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

Agreed.

> 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

Left as TODO.

Thanks for reviewing this patch.

> 
> thanks
> -- PMM
>