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(-)
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(-)
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)
+)
--
2.21.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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~ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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~ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Am 18.02.20 um 19:59 schrieb Stefan Weil: > Indeed, fixing such inconsitencies would be good. s/inconsitencies/inconsistencies/ :-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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
© 2016 - 2024 Red Hat, Inc.