Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access
On Sun Jul 7, 2024 at 9:46 AM AEST, David Gibson wrote:
> On Sat, Jul 06, 2024 at 11:37:08AM +0100, Peter Maydell wrote:
> > On Fri, 5 Jul 2024 at 06:13, David Gibson
> > wrote:
> > >
> > > On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > > > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Akihiko Odaki
> > > > > > > > > > ---
> > > > > > > > > > hw/ppc/vof.c | 2 +-
> > > > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > > > > > > --- a/hw/ppc/vof.c
> > > > > > > > > > +++ b/hw/ppc/vof.c
> > > > > > > > > > @@ -646,7 +646,7 @@ static void
> > > > > > > > > > vof_dt_memory_available(void *fdt, GArray *claimed,
> > > > > > > > > > uint64_t base)
> > > > > > > > > > mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > > > > > > > > > g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac
> > > > > > > > > > + sc));
> > > > > > > > > > if (sc == 2) {
> > > > > > > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg +
> > > > > > > > > > sizeof(uint32_t) * ac));
> > > > > > > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) *
> > > > > > > > > > ac);
> > > > > > > > > > } else {
> > > > > > > > > > mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg +
> > > > > > > > > > sizeof(uint32_t) * ac));
> > > > > > > > > > }
> > > > > > > > >
> > > > > > > > > I did wonder if there was a better way to do what this is
> > > > > > > > > doing,
> > > > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > > > provide one.
> > > > > > > >
> > > > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.),
> > > > > > > > but
> > > > > > > > not an automatic aligned-or-unaligned helper. Maybe we should
> > > > > > > > add that?
> > > > > > >
> > > > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > > > part, which we already have QEMU utility functions for (and which
> > > > > > > are this patch uses).
> > > > > > >
> > > > > > > This particular bit of code is dealing with an fdt property
> > > > > > > ("memory")
> > > > > > > that is an array of (address, size) tuples where address and size
> > > > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > > > size value of tuple 0. So the missing functionality is something
> > > > > > > at
> > > > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > > > is an awkward kind of API to write in C.)
> > > > > > >
> > > > > > > Slight
Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access
On Fri Jul 5, 2024 at 3:12 PM AEST, David Gibson wrote:
> On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> > On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > > On Sat, 29 Jun 2024 at 04:17, David Gibson
> > > > > wrote:
> > > > > >
> > > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > > > > >
> > > > > > > > Signed-off-by: Akihiko Odaki
> > > > > > > > ---
> > > > > > > > hw/ppc/vof.c | 2 +-
> > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > > > > --- a/hw/ppc/vof.c
> > > > > > > > +++ b/hw/ppc/vof.c
> > > > > > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void
> > > > > > > > *fdt, GArray *claimed, uint64_t base)
> > > > > > > > mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > > > > > > > g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac +
> > > > > > > > sc));
> > > > > > > > if (sc == 2) {
> > > > > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg +
> > > > > > > > sizeof(uint32_t) * ac));
> > > > > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > > > > > } else {
> > > > > > > > mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg +
> > > > > > > > sizeof(uint32_t) * ac));
> > > > > > > > }
> > > > > > >
> > > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > > provide one.
> > > > > >
> > > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > > not an automatic aligned-or-unaligned helper. Maybe we should add
> > > > > > that?
> > > > >
> > > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > > part, which we already have QEMU utility functions for (and which
> > > > > are this patch uses).
> > > > >
> > > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > > that is an array of (address, size) tuples where address and size
> > > > > can independently be either 32 or 64 bits, and it wants the
> > > > > size value of tuple 0. So the missing functionality is something at
> > > > > a higher level than fdt32_ld() which would let you say "give me
> > > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > > is an awkward kind of API to write in C.)
> > > > >
> > > > > Slightly less general, but for this case we could perhaps have
> > > > > something like the getprop equivalent of
> > > > > qemu_fdt_setprop_sized_cells():
> > > > >
> > > > > uint64_t value_array[2];
> > > > > qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > > > >ac, sc);
> > > > > /*
> > > > >* fills in value_array[0] with address, value_array[1] with size,
> > > > >* probably barfs if the varargs-list of cell-sizes doesn't
> > > > >* cover the whole property, similar to the current assert on
> > > > >* proplen.
> > > > >*/
> > > > > mem0_end = value_array[0];
> > > >
> > > > Since 4/8 byte cells are most common and size is probably
> > > > norma
Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access
On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > On Sat, 29 Jun 2024 at 04:17, David Gibson
> > > wrote:
> > > >
> > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki
> > > > > wrote:
> > > > > >
> > > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > > >
> > > > > > Signed-off-by: Akihiko Odaki
> > > > > > ---
> > > > > > hw/ppc/vof.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > > --- a/hw/ppc/vof.c
> > > > > > +++ b/hw/ppc/vof.c
> > > > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt,
> > > > > > GArray *claimed, uint64_t base)
> > > > > > mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > > > > > g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > > > > if (sc == 2) {
> > > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg +
> > > > > > sizeof(uint32_t) * ac));
> > > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > > > } else {
> > > > > > mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg +
> > > > > > sizeof(uint32_t) * ac));
> > > > > > }
> > > > >
> > > > > I did wonder if there was a better way to do what this is doing,
> > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > provide one.
> > > >
> > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > not an automatic aligned-or-unaligned helper. Maybe we should add
> > > > that?
> > >
> > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > part, which we already have QEMU utility functions for (and which
> > > are this patch uses).
> > >
> > > This particular bit of code is dealing with an fdt property ("memory")
> > > that is an array of (address, size) tuples where address and size
> > > can independently be either 32 or 64 bits, and it wants the
> > > size value of tuple 0. So the missing functionality is something at
> > > a higher level than fdt32_ld() which would let you say "give me
> > > tuple N field X" with some way to specify the tuple layout. (Which
> > > is an awkward kind of API to write in C.)
> > >
> > > Slightly less general, but for this case we could perhaps have
> > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > >
> > > uint64_t value_array[2];
> > > qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
> > >ac, sc);
> > > /*
> > >* fills in value_array[0] with address, value_array[1] with size,
> > >* probably barfs if the varargs-list of cell-sizes doesn't
> > >* cover the whole property, similar to the current assert on
> > >* proplen.
> > >*/
> > > mem0_end = value_array[0];
> >
> > Since 4/8 byte cells are most common and size is probably
> > normally known, what about something simpler to start with?
>
> Hrm, I don't think this helps much. As Peter points out the actual
> load isn't really the issue, it's locating the right spot for it.
I don't really see why that's a problem, it's just a pointer
addition - base + fdt_address_cells * 4. The problem was in
the memory access (yes it's fixed with the patch but you could
add a general libfdt way to do it).
Some fancy function like above could be used, But is it really
worth implementing such a thing for this?
Thanks,
Nick
Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access
On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 04:17, David Gibson
> wrote:
> >
> > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki
> > > wrote:
> > > >
> > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > >
> > > > Signed-off-by: Akihiko Odaki
> > > > ---
> > > > hw/ppc/vof.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > --- a/hw/ppc/vof.c
> > > > +++ b/hw/ppc/vof.c
> > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt,
> > > > GArray *claimed, uint64_t base)
> > > > mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > > > g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > > if (sc == 2) {
> > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg +
> > > > sizeof(uint32_t) * ac));
> > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > } else {
> > > > mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg +
> > > > sizeof(uint32_t) * ac));
> > > > }
> > >
> > > I did wonder if there was a better way to do what this is doing,
> > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > provide one.
> >
> > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > not an automatic aligned-or-unaligned helper. Maybe we should add that?
>
> fdt32_ld() and friends only do the "load from this bit of memory"
> part, which we already have QEMU utility functions for (and which
> are this patch uses).
>
> This particular bit of code is dealing with an fdt property ("memory")
> that is an array of (address, size) tuples where address and size
> can independently be either 32 or 64 bits, and it wants the
> size value of tuple 0. So the missing functionality is something at
> a higher level than fdt32_ld() which would let you say "give me
> tuple N field X" with some way to specify the tuple layout. (Which
> is an awkward kind of API to write in C.)
>
> Slightly less general, but for this case we could perhaps have
> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
>
> uint64_t value_array[2];
> qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", &value_array,
>ac, sc);
> /*
>* fills in value_array[0] with address, value_array[1] with size,
>* probably barfs if the varargs-list of cell-sizes doesn't
>* cover the whole property, similar to the current assert on
>* proplen.
>*/
> mem0_end = value_array[0];
Since 4/8 byte cells are most common and size is probably
normally known, what about something simpler to start with?
Thanks,
Nick
---
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 0677fea..c4b6355 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -148,6 +148,15 @@ static inline uint32_t fdt32_ld(const fdt32_t *p)
| bp[3];
}
+/*
+ * Load the value from a 32-bit cell of a property. Cells are 32-bit aligned
+ * so can use a single load.
+ */
+static inline uint32_t fdt32_ld_prop(const fdt32_t *p)
+{
+ return fdt32_to_cpu(*p);
+}
+
static inline void fdt32_st(void *property, uint32_t value)
{
uint8_t *bp = (uint8_t *)property;
@@ -172,6 +181,18 @@ static inline uint64_t fdt64_ld(const fdt64_t *p)
| bp[7];
}
+/*
+ * Load the value from a 64-bit cell of a property. Cells are 32-bit aligned
+ * so can use two loads.
+ */
+static inline uint64_t fdt64_ld_prop(const fdt64_t *p)
+{
+ const fdt64_t *_p = p;
+
+ return ((uint64_t)fdt32_to_cpu(_p[0]) << 32)
+ | fdt32_to_cpu(_p[1]);
+}
+
static inline void fdt64_st(void *property, uint64_t value)
{
uint8_t *bp = (uint8_t *)property;
Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access
On Sat Jun 29, 2024 at 1:16 PM AEST, David Gibson wrote:
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki
> > wrote:
> > >
> > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > >
> > > Signed-off-by: Akihiko Odaki
> > > ---
> > > hw/ppc/vof.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > index e3b430a81f4f..b5b6514d79fc 100644
> > > --- a/hw/ppc/vof.c
> > > +++ b/hw/ppc/vof.c
> > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray
> > > *claimed, uint64_t base)
> > > mem0_reg = fdt_getprop(fdt, offset, "reg", &proplen);
> > > g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > if (sc == 2) {
> > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t)
> > > * ac));
> > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > } else {
> > > mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t)
> > > * ac));
> > > }
> >
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper. Maybe we should add that?
Runtime test if the pointer is aligned?
What about just fdt_prop32_ld() and fdt_prop64_ld() where you know it's
4 byte aligned. Then just do 2 x 4 byte loads for the 64-bit, I don't
think performance would matter so much to try get a single load.
Thanks,
Nick
Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors
On Tue Jul 2, 2024 at 4:23 PM AEST, Thomas Huth wrote:
> On 02/07/2024 00.23, BALATON Zoltan wrote:
> > On Mon, 1 Jul 2024, Michael S. Tsirkin wrote:
> >> On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
> >>> Based-on:
> >>> <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathb...@quicinc.com>
> >>> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
> >>>
> >>> I saw various sanitizer errors when running check-qtest-ppc64. While
> >>> I could just turn off sanitizers, I decided to tackle them this time.
> >>>
> >>> Unfortunately, GLib does not free test data in some cases so some
> >>> sanitizer errors remain. All sanitizer errors will be gone with this
> >>> patch series combined with the following change for GLib:
> >>> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120
> >>>
> >>> Signed-off-by: Akihiko Odaki
> >>
> >>
> >> Reviewed-by: Michael S. Tsirkin
> >>
> >> who's merging all this?
> >
> > Maybe needs to be split. Mark had an alternative for macio which was picked
> > up by Philippe if I'm not mistaken. I've sent an alternative for vt82c686
> > which is still discussed but could belong to Philippe as well. PPC parts
> > could be taken by the PPC maintainers if there were any active at the
> > moment
> > and I don't know who maintains tests normally or other misc areas.
>
> I can take the qtest patches through my tree.
> cpu: Free cpu_ases
> migration: Free removed SaveStateEntry
> memory: Do not create circular reference with subregion
> hw/virtio: Free vqs after vhost_dev_cleanup()
These are all core code, maybe Philippe, Peter, or Richard,
Fabiano for migration perhaps?
> hw/ide: Convert macio ide_irq into GPIO line
> hw/ide: Remove internal DMA qemu_irq
> hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259
I guess I'll let you wrangle these and go through Philippe then?
> spapr: Free stdout path
> ppc/vof: Fix unaligned FDT property access
These ppc ones I can take, they seem okay. Sorry for my recent
inactivity.
Thanks,
Nick
> tests/qtest: Use qtest_add_data_func_full()
> tests/qtest: Free unused QMP response
> tests/qtest: Free old machine variable name
> tests/qtest: Delete previous boot file
> tests/qtest: Free paths
> tests/qtest: Free GThread
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri Oct 20, 2023 at 6:33 PM AEST, Greg Kurz wrote:
> On Fri, 20 Oct 2023 17:49:38 +1000
> "Nicholas Piggin" wrote:
>
> > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> > > On Thu, 19 Oct 2023 21:08:25 +0200
> > > Juan Quintela wrote:
> > >
> > > > Current code does:
> > > > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > > > dependinfg on cpu number
> > > > - for newer machines, it register vmstate_icp with "icp/server" name
> > > > and instance 0
> > > > - now it unregisters "icp/server" for the 1st instance.
> > > >
> > > > This is wrong at many levels:
> > > > - we shouldn't have two VMSTATEDescriptions with the same name
> > > > - In case this is the only solution that we can came with, it needs to
> > > > be:
> > > > * register pre_2_10_vmstate_dummy_icp
> > > > * unregister pre_2_10_vmstate_dummy_icp
> > > > * register real vmstate_icp
> > > >
> > > > As the initialization of this machine is already complex enough, I
> > > > need help from PPC maintainers to fix this.
> > > >
> > > > Volunteers?
> > > >
> > > > CC: Cedric Le Goater
> > > > CC: Daniel Henrique Barboza
> > > > CC: David Gibson
> > > > CC: Greg Kurz
> > > >
> > > > Signed-off-by: Juan Quintela
> > > > ---
> > > > hw/ppc/spapr.c | 7 ++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index cb840676d3..8531d13492 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> > > > *opaque)
> > > > }
> > > >
> > > > static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > -.name = "icp/server",
> > > > +/*
> > > > + * Hack ahead. We can't have two devices with the same name and
> > > > + * instance id. So I rename this to pass make check.
> > > > + * Real help from people who knows the hardware is needed.
> > > > + */
> > > > +.name = "pre-2.10-icp/server",
> > > > .version_id = 1,
> > > > .minimum_version_id = 1,
> > > > .needed = pre_2_10_vmstate_dummy_icp_needed,
> > >
> > > I guess this fix is acceptable as well and a lot simpler than
> > > reverting the hack actually. Outcome is the same : drop
> > > compat with pseries-2.9 and older.
> > >
> > > Reviewed-by: Greg Kurz
> >
> > So the reason we can't have duplicate names registered, aside from it
> > surely going bad if we actually send or receive a stream at the point
> > they are registered, is the duplcate check introduced in patch 9? But
> > before that, this hack does seem to actually work because the duplicate
> > is unregistered right away.
> >
>
> Correct.
>
> > If I understand the workaround, there is an asymmetry in the migration
> > sequence in that receiving an unexpected object would cause a failure,
> > but going from newer to older would just skip some "expected" objects
> > and that didn't cause a problem. So you only have to deal with ignoring
> > the unexpected ones going form older to newer.
> >
>
> Correct.
>
> > Side question, is it possible to flag the problem of *not* receiving
> > an object that you did expect? That might be a source of bugs too.
> >
>
> AFAICR we try to only migrate state that differs from reset : the
> destination cannot really assume it will receive anything for a
> given device.
That's true, I guess you could always add some flag yourself if
you certainly need something.
>
> > Anyway, I wonder if we could fix this spapr problem by adding a special
> > case wild card instance matcher to ignore it? It's still a bit hacky
> > but maybe a bit nicer. I don't mind deprecating the machine soon if
> > you want to clear the wildcard hack away soon, but it would be nice to
> > separate the deprecation and removal from the fix, if possible.
> >
> > This patch is not tested but hopefully helps illustrate the idea.
> >
>
> I'm not sure this will fly with older QEMUs that don't know about
> VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that.
You could be right about that. He gave a simpler solution now
anyway.
Thanks,
Nick
Re: [PATCH v2 07/13] migration: Hack to maintain backwards compatibility for ppc
On Fri Oct 20, 2023 at 7:07 PM AEST, Juan Quintela wrote:
> Current code does:
> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> dependinfg on cpu number
> - for newer machines, it register vmstate_icp with "icp/server" name
> and instance 0
> - now it unregisters "icp/server" for the 1st instance.
>
> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name
> - In case this is the only solution that we can came with, it needs to
> be:
> * register pre_2_10_vmstate_dummy_icp
> * unregister pre_2_10_vmstate_dummy_icp
> * register real vmstate_icp
>
> Created vmstate_replace_hack_for_ppc() with warnings left and right
> that it is a hack.
Thanks. We'll look at deprecating 2.9 soon so this can all be removed.
Reviewed-by: Nicholas Piggin
>
> CC: Cedric Le Goater
> CC: Daniel Henrique Barboza
> CC: David Gibson
> CC: Greg Kurz
>
> Signed-off-by: Juan Quintela
> ---
> include/migration/vmstate.h | 11 +++
> hw/intc/xics.c | 17 +++--
> hw/ppc/spapr.c | 25 +++--
> migration/savevm.c | 18 ++
> 4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9ca7e9cc48..65deaecc92 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int
> instance_id,
>opaque, -1, 0, NULL);
> }
>
> +/**
> + * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
> + *
> + * Don't even think about using this function in new code.
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> + const VMStateDescription *vmsd,
> + void *opaque);
> +
> /**
> * vmstate_register_any() - legacy function to register state
> * serialisation description and let the function choose the id
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7f8abd71e..a03979e72a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
> return;
> }
> }
> -
> -vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +/*
> + * The way that pre_2_10_icp is handling is really, really hacky.
> + * We used to have here this call:
> + *
> + * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> + *
> + * But we were doing:
> + * pre_2_10_vmstate_register_dummy_icp()
> + * this vmstate_register()
> + * pre_2_10_vmstate_unregister_dummy_icp()
> + *
> + * So for a short amount of time we had to vmstate entries with
> + * the same name. This fixes it.
> + */
> +vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index,
> &vmstate_icp_server, icp);
> }
>
> static void icp_unrealize(DeviceState *dev)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..a75cf475ad 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> *opaque)
> }
>
> static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +/*
> + * Hack ahead. We can't have two devices with the same name and
> + * instance id. So I rename this to pass make check.
> + * Real help from people who knows the hardware is needed.
> + */
> .name = "icp/server",
> .version_id = 1,
> .minimum_version_id = 1,
> @@ -155,16 +160,32 @@ static const VMStateDescription
> pre_2_10_vmstate_dummy_icp = {
> },
> };
>
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
> static void pre_2_10_vmstate_register_dummy_icp(int i)
> {
> vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
> (void *)(uintptr_t) i);
> }
>
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
> static void pre_2_10_vmstate_unregister_dummy_icp(int i)
> {
> -vmstate_unregister(NULL, &
Re: [PATCH 07/13] RFC migration: icp/server is a mess
On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela wrote:
>
> > Current code does:
> > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > dependinfg on cpu number
> > - for newer machines, it register vmstate_icp with "icp/server" name
> > and instance 0
> > - now it unregisters "icp/server" for the 1st instance.
> >
> > This is wrong at many levels:
> > - we shouldn't have two VMSTATEDescriptions with the same name
> > - In case this is the only solution that we can came with, it needs to
> > be:
> > * register pre_2_10_vmstate_dummy_icp
> > * unregister pre_2_10_vmstate_dummy_icp
> > * register real vmstate_icp
> >
> > As the initialization of this machine is already complex enough, I
> > need help from PPC maintainers to fix this.
> >
> > Volunteers?
> >
> > CC: Cedric Le Goater
> > CC: Daniel Henrique Barboza
> > CC: David Gibson
> > CC: Greg Kurz
> >
> > Signed-off-by: Juan Quintela
> > ---
> > hw/ppc/spapr.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index cb840676d3..8531d13492 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void
> > *opaque)
> > }
> >
> > static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > -.name = "icp/server",
> > +/*
> > + * Hack ahead. We can't have two devices with the same name and
> > + * instance id. So I rename this to pass make check.
> > + * Real help from people who knows the hardware is needed.
> > + */
> > +.name = "pre-2.10-icp/server",
> > .version_id = 1,
> > .minimum_version_id = 1,
> > .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz
So the reason we can't have duplicate names registered, aside from it
surely going bad if we actually send or receive a stream at the point
they are registered, is the duplcate check introduced in patch 9? But
before that, this hack does seem to actually work because the duplicate
is unregistered right away.
If I understand the workaround, there is an asymmetry in the migration
sequence in that receiving an unexpected object would cause a failure,
but going from newer to older would just skip some "expected" objects
and that didn't cause a problem. So you only have to deal with ignoring
the unexpected ones going form older to newer.
Side question, is it possible to flag the problem of *not* receiving
an object that you did expect? That might be a source of bugs too.
Anyway, I wonder if we could fix this spapr problem by adding a special
case wild card instance matcher to ignore it? It's still a bit hacky
but maybe a bit nicer. I don't mind deprecating the machine soon if
you want to clear the wildcard hack away soon, but it would be nice to
separate the deprecation and removal from the fix, if possible.
This patch is not tested but hopefully helps illustrate the idea.
Thanks,
Nick
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..8ce03edefa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const
VMStateDescription *vmsd,
bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
#define VMSTATE_INSTANCE_ID_ANY -1
+#define VMSTATE_INSTANCE_ID_WILD -2
/* Returns: 0 on success, -1 on failure */
int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..2418899dd4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -155,16 +155,10 @@ static const VMStateDescription
pre_2_10_vmstate_dummy_icp = {
},
};
-static void pre_2_10_vmstate_register_dummy_icp(int i)
+static void pre_2_10_vmstate_register_dummy_icp(void)
{
-vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
- (void *)(uintptr_t) i);
-}
-
-static void pre_2_10_vmstate_unregister_dummy_icp(int i)
-{
-vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
- (void *)(uintptr_t) i);
+vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD,
+ &pre_2_10_vmstate_dummy_icp, NULL);
}
int spapr_max_server_number(SpaprMachineState *spapr)
@@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
}
if (smc->pre_2_10_has_unused_icps) {
-for (i = 0; i < spapr_max_server_number(spapr); i++) {
-/* Dummy entries get deregistered when real ICPState objects
- * are registered during CPU core hotplug.
- */
-pre_2_10_vmstate_register_dummy_icp(i);
-}
+/* Dummy entries get deregistered when
Re: [PATCH v3 0/9] bulk: Replace CONFIG_SOFTMMU by !CONFIG_USER_ONLY/CONFIG_SYSTEM_ONLY
On Tue Jun 13, 2023 at 11:33 PM AEST, Philippe Mathieu-Daudé wrote: > Missing review: 1, 7, 8 > > Since v2: > - Rebased > - Added R-b tags > - Rework i386_tr_init_disas_context() patch (Richard) > - Dropped RFC prefix > > This series aims to clarify the CONFIG_[USER|SYSTEM] vs CONFIG_SOFTMMU > confusion [*] by using explicit definitions, removing mentions of > CONFIG_SOFTMMU in non-TCG code. > > We replace CONFIG_SOFTMMU by !CONFIG_USER_ONLY in C code and > by CONFIG_SYSTEM_ONLY in meson config files. I like the change in general, SOFTMMU does not read well (and is not exactly correct for system code as pointed out). Sorry for chiming in late and if I missed it, but was there a reason not to define a complementary CONFIG_SYSTEM so system code does not have to test !CONFIG_USER_ONLY and invert a bunch of the tests? Actually I thought you would have CONFIG_SYSTEM and CONFIG_USER options and the _ONLY variants could be derivative for convenience, but I'm probably missing some detail. Thanks, Nick
Re: [PATCH v3 4/9] target/ppc: Check for USER_ONLY definition instead of SOFTMMU one
On Tue Jun 13, 2023 at 11:33 PM AEST, Philippe Mathieu-Daudé wrote:
> Since we *might* have user emulation with softmmu,
> replace the system emulation check by !user emulation one.
>
> Signed-off-by: Philippe Mathieu-Daudé
> Reviewed-by: Richard Henderson
Reviewed-by: Nicholas Piggin
> ---
> target/ppc/cpu_init.c| 20 ++--
> target/ppc/helper_regs.c | 6 ++
> 2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 9f97222655..7bce421a7c 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5841,7 +5841,7 @@ POWERPC_FAMILY(970)(ObjectClass *oc, void *data)
> (1ull << MSR_PMM) |
> (1ull << MSR_RI);
> pcc->mmu_model = POWERPC_MMU_64B;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> pcc->hash64_opts = &ppc_hash64_opts_basic;
> #endif
> pcc->excp_model = POWERPC_EXCP_970;
> @@ -5920,7 +5920,7 @@ POWERPC_FAMILY(POWER5P)(ObjectClass *oc, void *data)
> pcc->lpcr_mask = LPCR_RMLS | LPCR_ILE | LPCR_LPES0 | LPCR_LPES1 |
> LPCR_RMI | LPCR_HDICE;
> pcc->mmu_model = POWERPC_MMU_2_03;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> pcc->hash64_opts = &ppc_hash64_opts_basic;
> pcc->lrg_decr_bits = 32;
> #endif
> @@ -6037,7 +6037,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
> LPCR_LPES0 | LPCR_LPES1 | LPCR_HDICE;
> pcc->lpcr_pm = LPCR_P7_PECE0 | LPCR_P7_PECE1 | LPCR_P7_PECE2;
> pcc->mmu_model = POWERPC_MMU_2_06;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> pcc->hash64_opts = &ppc_hash64_opts_POWER7;
> pcc->lrg_decr_bits = 32;
> #endif
> @@ -6181,7 +6181,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
> pcc->lpcr_pm = LPCR_P8_PECE0 | LPCR_P8_PECE1 | LPCR_P8_PECE2 |
> LPCR_P8_PECE3 | LPCR_P8_PECE4;
> pcc->mmu_model = POWERPC_MMU_2_07;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> pcc->hash64_opts = &ppc_hash64_opts_POWER7;
> pcc->lrg_decr_bits = 32;
> pcc->n_host_threads = 8;
> @@ -6197,7 +6197,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
> pcc->l1_icache_size = 0x8000;
> }
>
> -#ifdef CONFIG_SOFTMMU
> +#ifndef CONFIG_USER_ONLY
> /*
> * Radix pg sizes and AP encodings for dt node
> ibm,processor-radix-AP-encodings
> * Encoded as array of int_32s in the form:
> @@ -6214,7 +6214,7 @@ static struct ppc_radix_page_info
> POWER9_radix_page_info = {
> 0x401e /* 1G - enc: 0x2 */
> }
> };
> -#endif /* CONFIG_SOFTMMU */
> +#endif /* CONFIG_USER_ONLY */
>
> static void init_proc_POWER9(CPUPPCState *env)
> {
> @@ -6371,7 +6371,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
> LPCR_HEIC | LPCR_LPES0 | LPCR_HVICE | LPCR_HDICE;
> pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
> pcc->mmu_model = POWERPC_MMU_3_00;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> /* segment page size remain the same */
> pcc->hash64_opts = &ppc_hash64_opts_POWER7;
> pcc->radix_page_info = &POWER9_radix_page_info;
> @@ -6389,7 +6389,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void *data)
> pcc->l1_icache_size = 0x8000;
> }
>
> -#ifdef CONFIG_SOFTMMU
> +#ifndef CONFIG_USER_ONLY
> /*
> * Radix pg sizes and AP encodings for dt node
> ibm,processor-radix-AP-encodings
> * Encoded as array of int_32s in the form:
> @@ -6406,7 +6406,7 @@ static struct ppc_radix_page_info
> POWER10_radix_page_info = {
> 0x401e /* 1G - enc: 0x2 */
> }
> };
> -#endif /* CONFIG_SOFTMMU */
> +#endif /* !CONFIG_USER_ONLY */
>
> static void init_proc_POWER10(CPUPPCState *env)
> {
> @@ -6547,7 +6547,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>
> pcc->lpcr_pm = LPCR_PDEE | LPCR_HDEE | LPCR_EEE | LPCR_DEE | LPCR_OEE;
> pcc->mmu_model = POWERPC_MMU_3_00;
> -#if defined(CONFIG_SOFTMMU)
> +#if !defined(CONFIG_USER_ONLY)
> /* segment page size remain the same */
> pcc->hash64_opts = &ppc_hash64_opts_POWER7;
> pcc->radix_page_info = &POWER10_radix_page_info;
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index bc7e9d7eda..e27f4a75a4 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -310,7 +310,7 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value,
> int alter_hv)
> return e
