| Message ID | 20200531175425.10329-5-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | exec/memory: Rework some address and access size limits | expand |
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
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 >
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); void qemu_ram_remap(ram_addr_t addr, ram_addr_t length); /* This should not be used by devices. */ diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c index c29fbef6fc..4063cbccf8 100644 --- a/hw/usb/hcd-musb.c +++ b/hw/usb/hcd-musb.c @@ -1243,7 +1243,7 @@ static void musb_ep_writeh(void *opaque, int ep, int addr, uint16_t value) } /* Generic control */ -static uint32_t musb_readb(void *opaque, hwaddr addr) +static uint64_t musb_readb(void *opaque, hwaddr addr) { MUSBState *s = (MUSBState *) opaque; int ep, i; @@ -1305,7 +1305,7 @@ static uint32_t musb_readb(void *opaque, hwaddr addr) }; } -static void musb_writeb(void *opaque, hwaddr addr, uint32_t value) +static void musb_writeb(void *opaque, hwaddr addr, uint64_t value) { MUSBState *s = (MUSBState *) opaque; int ep; @@ -1392,7 +1392,7 @@ static void musb_writeb(void *opaque, hwaddr addr, uint32_t value) }; } -static uint32_t musb_readh(void *opaque, hwaddr addr) +static uint64_t musb_readh(void *opaque, hwaddr addr) { MUSBState *s = (MUSBState *) opaque; int ep, i; @@ -1446,7 +1446,7 @@ static uint32_t musb_readh(void *opaque, hwaddr addr) }; } -static void musb_writeh(void *opaque, hwaddr addr, uint32_t value) +static void musb_writeh(void *opaque, hwaddr addr, uint64_t value) { MUSBState *s = (MUSBState *) opaque; int ep; @@ -1502,7 +1502,7 @@ static void musb_writeh(void *opaque, hwaddr addr, uint32_t value) }; } -static uint32_t musb_readw(void *opaque, hwaddr addr) +static uint64_t musb_readw(void *opaque, hwaddr addr) { MUSBState *s = (MUSBState *) opaque; int ep; @@ -1520,7 +1520,7 @@ static uint32_t musb_readw(void *opaque, hwaddr addr) }; } -static void musb_writew(void *opaque, hwaddr addr, uint32_t value) +static void musb_writew(void *opaque, hwaddr addr, uint64_t value) { MUSBState *s = (MUSBState *) opaque; int ep;
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(-)