| Message ID | 20200218132023.22936-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid cpu_physical_memory_rw() with a constant is_write argument | expand |
On 2/18/20 5:20 AM, Philippe Mathieu-Daudé wrote: > This commit was produced with the included Coccinelle script > scripts/coccinelle/as-rw-const.patch. > > Inspired-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Am 18.02.20 um 14:20 schrieb Philippe Mathieu-Daudé: > This commit was produced with the included Coccinelle script > scripts/coccinelle/as-rw-const.patch. > > Inspired-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> [...] > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > index a8b6e5aeb8..f5971ccc74 100644 > --- a/target/i386/hax-all.c > +++ b/target/i386/hax-all.c > @@ -376,8 +376,8 @@ static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft) > * hft->direction == 2: gpa ==> gpa2 > */ > uint64_t value; > - cpu_physical_memory_rw(hft->gpa, (uint8_t *) &value, hft->size, 0); > - cpu_physical_memory_rw(hft->gpa2, (uint8_t *) &value, hft->size, 1); > + cpu_physical_memory_read(hft->gpa, (uint8_t *)&value, hft->size); > + cpu_physical_memory_write(hft->gpa2, (uint8_t *)&value, hft->size); Maybe those type casts could be removed, too. They are no longer needed after your modification. Stefan
On Tue, Feb 18, 2020 at 6:57 PM Stefan Weil <sw@weilnetz.de> wrote: > Am 18.02.20 um 14:20 schrieb Philippe Mathieu-Daudé: > > > This commit was produced with the included Coccinelle script > > scripts/coccinelle/as-rw-const.patch. > > > > Inspired-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> > [...] > > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > > index a8b6e5aeb8..f5971ccc74 100644 > > --- a/target/i386/hax-all.c > > +++ b/target/i386/hax-all.c > > @@ -376,8 +376,8 @@ static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft) > > * hft->direction == 2: gpa ==> gpa2 > > */ > > uint64_t value; > > - cpu_physical_memory_rw(hft->gpa, (uint8_t *) &value, hft->size, 0); > > - cpu_physical_memory_rw(hft->gpa2, (uint8_t *) &value, hft->size, 1); > > + cpu_physical_memory_read(hft->gpa, (uint8_t *)&value, hft->size); > > + cpu_physical_memory_write(hft->gpa2, (uint8_t *)&value, hft->size); > > > Maybe those type casts could be removed, too. They are no longer needed > after your modification. Good catch, thanks Stefan! > > Stefan > >
On Tue, 18 Feb 2020 at 17:57, Stefan Weil <sw@weilnetz.de> wrote: > > Am 18.02.20 um 14:20 schrieb Philippe Mathieu-Daudé: > > > This commit was produced with the included Coccinelle script > > scripts/coccinelle/as-rw-const.patch. > > > > Inspired-by: Peter Maydell <peter.maydell@linaro.org> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> > [...] > > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > > index a8b6e5aeb8..f5971ccc74 100644 > > --- a/target/i386/hax-all.c > > +++ b/target/i386/hax-all.c > > @@ -376,8 +376,8 @@ static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft) > > * hft->direction == 2: gpa ==> gpa2 > > */ > > uint64_t value; > > - cpu_physical_memory_rw(hft->gpa, (uint8_t *) &value, hft->size, 0); > > - cpu_physical_memory_rw(hft->gpa2, (uint8_t *) &value, hft->size, 1); > > + cpu_physical_memory_read(hft->gpa, (uint8_t *)&value, hft->size); > > + cpu_physical_memory_write(hft->gpa2, (uint8_t *)&value, hft->size); > > > Maybe those type casts could be removed, too. They are no longer needed > after your modification. I think that we should fix the inconsistency where these functions all take "uint8_t* buf": - address_space_rw() - address_space_read() - address_space_write() - address_space_write_rom() - cpu_physical_memory_rw() - cpu_memory_rw_debug() but these take void*: - cpu_physical_memory_read() - cpu_physical_memory_write() - address_space_write_cached() - address_space_read_cached_slow() - address_space_write_cached_slow() - pci_dma_read() - pci_dma_write() - pci_dma_rw() - dma_memory_read() - dma_memory_write() - dma_memory_rw() - dma_memory_rw_relaxed() Depending on which way we go we would either want to remove these casts, or not. I guess that we have more cases of 'void*', and that would certainly be the easier way to convert (otherwise we probably need to add a bunch of new casts to uint8_t* in various callsites), but I don't have a strong opinion. Paolo ? thanks -- PMM
Am 18.02.20 um 19:49 schrieb Peter Maydell: > I think that we should fix the inconsistency where these functions > all take "uint8_t* buf": > > - address_space_rw() > - address_space_read() > - address_space_write() > - address_space_write_rom() > - cpu_physical_memory_rw() > - cpu_memory_rw_debug() > > but these take void*: > - cpu_physical_memory_read() > - cpu_physical_memory_write() > - address_space_write_cached() > - address_space_read_cached_slow() > - address_space_write_cached_slow() > - pci_dma_read() > - pci_dma_write() > - pci_dma_rw() > - dma_memory_read() > - dma_memory_write() > - dma_memory_rw() > - dma_memory_rw_relaxed() > > Depending on which way we go we would either want to remove these > casts, or not. > > I guess that we have more cases of 'void*', and that would > certainly be the easier way to convert (otherwise we probably > need to add a bunch of new casts to uint8_t* in various callsites), > but I don't have a strong opinion. Paolo ? > > thanks > -- PMM Indeed, fixing such inconsitencies would be good. Personally I like the way how the standard C library handles such pointers for functions like memcpy, fread, fwrite and others. Therefore I suggest to use `const void *` and `void *` and to avoid type casts. Regards Stefan
Am 18.02.20 um 19:59 schrieb Stefan Weil:
> Indeed, fixing such inconsitencies would be good.
s/inconsitencies/inconsistencies/
:-)
On 2/18/20 10:59 AM, Stefan Weil wrote: > Personally I like the way how the standard C library handles such > pointers for functions like memcpy, fread, fwrite and others. > > Therefore I suggest to use `const void *` and `void *` and to avoid type > casts. Seconded. r~
On 2/18/20 7:49 PM, Peter Maydell wrote: > On Tue, 18 Feb 2020 at 17:57, Stefan Weil <sw@weilnetz.de> wrote: >> >> Am 18.02.20 um 14:20 schrieb Philippe Mathieu-Daudé: >> >>> This commit was produced with the included Coccinelle script >>> scripts/coccinelle/as-rw-const.patch. >>> >>> Inspired-by: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> >> [...] >>> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c >>> index a8b6e5aeb8..f5971ccc74 100644 >>> --- a/target/i386/hax-all.c >>> +++ b/target/i386/hax-all.c >>> @@ -376,8 +376,8 @@ static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft) >>> * hft->direction == 2: gpa ==> gpa2 >>> */ >>> uint64_t value; >>> - cpu_physical_memory_rw(hft->gpa, (uint8_t *) &value, hft->size, 0); >>> - cpu_physical_memory_rw(hft->gpa2, (uint8_t *) &value, hft->size, 1); >>> + cpu_physical_memory_read(hft->gpa, (uint8_t *)&value, hft->size); >>> + cpu_physical_memory_write(hft->gpa2, (uint8_t *)&value, hft->size); >> >> >> Maybe those type casts could be removed, too. They are no longer needed >> after your modification. > > I think that we should fix the inconsistency where these functions > all take "uint8_t* buf": > > - address_space_rw() > - address_space_read() > - address_space_write() > - address_space_write_rom() > - cpu_physical_memory_rw() > - cpu_memory_rw_debug() > > but these take void*: > - cpu_physical_memory_read() > - cpu_physical_memory_write() > - address_space_write_cached() > - address_space_read_cached_slow() > - address_space_write_cached_slow() > - pci_dma_read() > - pci_dma_write() > - pci_dma_rw() > - dma_memory_read() > - dma_memory_write() > - dma_memory_rw() > - dma_memory_rw_relaxed() I don't understand well cpu_physical_memory*(). Aren't these obsolete? They confuse me when using multi-core CPUs. > > Depending on which way we go we would either want to remove these > casts, or not. > > I guess that we have more cases of 'void*', and that would > certainly be the easier way to convert (otherwise we probably > need to add a bunch of new casts to uint8_t* in various callsites), > but I don't have a strong opinion. Paolo ? I thought about it too but it is quite some work, and I'v to admit I lost some faith with my previous chardev conversion. There Paolo/Daniel agreed to follow the libc read()/write() prototypes. > > thanks > -- PMM >
On Tue, 18 Feb 2020 at 20:07, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > I don't understand well cpu_physical_memory*(). Aren't these obsolete? > They confuse me when using multi-core CPUs. They sort of are, but there is no simple mechanical replacement for them -- you need to look at the individual use to see what address space it should really be using. For instance the cases in hw/dma/ probably would require the device to be updated to the new pattern where it takes a MemoryRegion defining what it should be doing DMA to, and then it can create an AddressSpace and use that with address_space_*. But that's a bunch of work on older devices which mostly people don't care very much about. In theory we could do a textual replacement of cpu_physical_memory* with address_space_rw(&address_space_memory,...) but that's usually not the right address space, so I'm not sure that churn is worthwhile. thanks -- PMM
On 18/02/20 19:49, Peter Maydell wrote: > Depending on which way we go we would either want to remove these > casts, or not. > > I guess that we have more cases of 'void*', and that would > certainly be the easier way to convert (otherwise we probably > need to add a bunch of new casts to uint8_t* in various callsites), > but I don't have a strong opinion. Paolo ? I agree we should use void* (possibly const). Paolo
diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c index b69732729b..a3bc7e3921 100644 --- a/hw/xen/xen_pt_graphics.c +++ b/hw/xen/xen_pt_graphics.c @@ -222,7 +222,7 @@ void xen_pt_setup_vga(XenPCIPassthroughState *s, XenHostPCIDevice *dev, } /* Currently we fixed this address as a primary for legacy BIOS. */ - cpu_physical_memory_rw(0xc0000, bios, bios_size, 1); + cpu_physical_memory_write(0xc0000, bios, bios_size); } uint32_t igd_read_opregion(XenPCIPassthroughState *s) diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c index a8b6e5aeb8..f5971ccc74 100644 --- a/target/i386/hax-all.c +++ b/target/i386/hax-all.c @@ -376,8 +376,8 @@ static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft) * hft->direction == 2: gpa ==> gpa2 */ uint64_t value; - cpu_physical_memory_rw(hft->gpa, (uint8_t *) &value, hft->size, 0); - cpu_physical_memory_rw(hft->gpa2, (uint8_t *) &value, hft->size, 1); + cpu_physical_memory_read(hft->gpa, (uint8_t *)&value, hft->size); + cpu_physical_memory_write(hft->gpa2, (uint8_t *)&value, hft->size); } return 0; diff --git a/scripts/coccinelle/as_rw_const.cocci b/scripts/coccinelle/as_rw_const.cocci index 30da707701..c9a39f1abe 100644 --- a/scripts/coccinelle/as_rw_const.cocci +++ b/scripts/coccinelle/as_rw_const.cocci @@ -1,6 +1,6 @@ // Avoid uses of address_space_rw() with a constant is_write argument. // Usage: -// spatch --sp-file as-rw-const.spatch --dir . --in-place +// spatch --sp-file scripts/coccinelle/as_rw_const.cocci --dir . --in-place @@ expression E1, E2, E3, E4, E5; @@ -28,3 +28,21 @@ expression E1, E2, E3, E4, E5; - address_space_rw(E1, E2, E3, E4, E5, 1) + address_space_write(E1, E2, E3, E4, E5) + +// Avoid uses of cpu_physical_memory_rw() with a constant is_write argument. +@@ +expression E1, E2, E3; +@@ +( +- cpu_physical_memory_rw(E1, E2, E3, false) ++ cpu_physical_memory_read(E1, E2, E3) +| +- cpu_physical_memory_rw(E1, E2, E3, 0) ++ cpu_physical_memory_read(E1, E2, E3) +| +- cpu_physical_memory_rw(E1, E2, E3, true) ++ cpu_physical_memory_write(E1, E2, E3) +| +- cpu_physical_memory_rw(E1, E2, E3, 1) ++ cpu_physical_memory_write(E1, E2, E3) +)
This commit was produced with the included Coccinelle script scripts/coccinelle/as-rw-const.patch. Inspired-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- Based-on: <20200218112457.22712-1-peter.maydell@linaro.org> Maybe can be squashed in Peter's patch? Cocci script can be written as: @@ expression E1, E2, E3, E4, E5; symbol true, false; @@ ( - address_space_rw(E1, E2, E3, E4, E5, false) + address_space_read(E1, E2, E3, E4, E5) | - address_space_rw(E1, E2, E3, E4, E5, 0) + address_space_read(E1, E2, E3, E4, E5) | - address_space_rw(E1, E2, E3, E4, E5, true) + address_space_write(E1, E2, E3, E4, E5) | - address_space_rw(E1, E2, E3, E4, E5, 1) + address_space_write(E1, E2, E3, E4, E5) ) @@ expression E1, E2, E3; @@ ( - cpu_physical_memory_rw(E1, E2, E3, false) + cpu_physical_memory_read(E1, E2, E3) | - cpu_physical_memory_rw(E1, E2, E3, 0) + cpu_physical_memory_read(E1, E2, E3) | - cpu_physical_memory_rw(E1, E2, E3, true) + cpu_physical_memory_write(E1, E2, E3) | - cpu_physical_memory_rw(E1, E2, E3, 1) + cpu_physical_memory_write(E1, E2, E3) ) --- hw/xen/xen_pt_graphics.c | 2 +- target/i386/hax-all.c | 4 ++-- scripts/coccinelle/as_rw_const.cocci | 20 +++++++++++++++++++- 3 files changed, 22 insertions(+), 4 deletions(-)