| Message ID | 20220805093948.82561-11-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | QMP/HMP: add 'dumpdtb' and 'info fdt' commands | expand |
On Fri, Aug 05, 2022 at 06:39:38AM -0300, Daniel Henrique Barboza wrote: > The pSeries machine never bothered with the common machine->fdt > attribute. We do all the FDT related work using spapr->fdt_blob. > > We're going to introduce HMP commands to read and save the FDT, which > will rely on setting machine->fdt properly to work across all machine > archs/types. > > Let's set machine->fdt in the two places where we manipulate the FDT: > spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what > we want is a way to access the FDT from HMP, not replace > spapr->fdt_blob. Given there is now an fdt field in the generic MACHINE structure, we should be able to remove the one in spapr->fdt_blob, yes? > > Cc: Cédric Le Goater <clg@kaod.org> > Cc: qemu-ppc@nongnu.org > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr.c | 6 ++++++ > hw/ppc/spapr_hcall.c | 8 ++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bc9ba6e6dc..94c90f0351 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine) > spapr->fdt_initial_size = spapr->fdt_size; > spapr->fdt_blob = fdt; > > + /* > + * Set the common machine->fdt pointer to enable support > + * for 'dumpdtb' and 'info fdt' commands. > + */ > + machine->fdt = fdt; > + > /* Set up the entry state */ > first_ppc_cpu->env.gpr[5] = 0; > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index a8d4a6bcf0..0079bc6fdc 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, > spapr->fdt_initial_size = spapr->fdt_size; > spapr->fdt_blob = fdt; > > + /* > + * Set the machine->fdt pointer again since we just freed > + * it above (by freeing spapr->fdt_blob). We set this > + * pointer to enable support for 'dumpdtb' and 'info fdt' > + * HMP commands. > + */ > + MACHINE(spapr)->fdt = fdt; > + > return H_SUCCESS; > } >
On 8/8/22 00:26, David Gibson wrote: > On Fri, Aug 05, 2022 at 06:39:38AM -0300, Daniel Henrique Barboza wrote: >> The pSeries machine never bothered with the common machine->fdt >> attribute. We do all the FDT related work using spapr->fdt_blob. >> >> We're going to introduce HMP commands to read and save the FDT, which >> will rely on setting machine->fdt properly to work across all machine >> archs/types. >> >> Let's set machine->fdt in the two places where we manipulate the FDT: >> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what >> we want is a way to access the FDT from HMP, not replace >> spapr->fdt_blob. > > Given there is now an fdt field in the generic MACHINE structure, we > should be able to remove the one in spapr->fdt_blob, yes? I thought about it but I backed down when I realized that spapr->fdt_blob is being migrated. At first glance it would be a matter of migrating ms->fdt and then removing spapr->fdt_blob, but then I got confused about how a migration to an older version would occur (e.g. QEMU 7.2 with ms->fdt to a QEMU 7.0 with spapr->fdt_blob). Migration to a newer QEMU would require us to move the spapr->version_id to 4 and then handle the old version accordingly in spapr_post_load(). Probably something to think about after this work is accepted. Thanks, Daniel > >> >> Cc: Cédric Le Goater <clg@kaod.org> >> Cc: qemu-ppc@nongnu.org >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/ppc/spapr.c | 6 ++++++ >> hw/ppc/spapr_hcall.c | 8 ++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index bc9ba6e6dc..94c90f0351 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine) >> spapr->fdt_initial_size = spapr->fdt_size; >> spapr->fdt_blob = fdt; >> >> + /* >> + * Set the common machine->fdt pointer to enable support >> + * for 'dumpdtb' and 'info fdt' commands. >> + */ >> + machine->fdt = fdt; >> + >> /* Set up the entry state */ >> first_ppc_cpu->env.gpr[5] = 0; >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index a8d4a6bcf0..0079bc6fdc 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, >> spapr->fdt_initial_size = spapr->fdt_size; >> spapr->fdt_blob = fdt; >> >> + /* >> + * Set the machine->fdt pointer again since we just freed >> + * it above (by freeing spapr->fdt_blob). We set this >> + * pointer to enable support for 'dumpdtb' and 'info fdt' >> + * HMP commands. >> + */ >> + MACHINE(spapr)->fdt = fdt; >> + >> return H_SUCCESS; >> } >> >
On Fri, Aug 12, 2022 at 07:23:09PM -0300, Daniel Henrique Barboza wrote: > > > On 8/8/22 00:26, David Gibson wrote: > > On Fri, Aug 05, 2022 at 06:39:38AM -0300, Daniel Henrique Barboza wrote: > > > The pSeries machine never bothered with the common machine->fdt > > > attribute. We do all the FDT related work using spapr->fdt_blob. > > > > > > We're going to introduce HMP commands to read and save the FDT, which > > > will rely on setting machine->fdt properly to work across all machine > > > archs/types. > > > > > > Let's set machine->fdt in the two places where we manipulate the FDT: > > > spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what > > > we want is a way to access the FDT from HMP, not replace > > > spapr->fdt_blob. > > > > Given there is now an fdt field in the generic MACHINE structure, we > > should be able to remove the one in spapr->fdt_blob, yes? > > I thought about it but I backed down when I realized that spapr->fdt_blob is > being migrated. > > At first glance it would be a matter of migrating ms->fdt and then removing > spapr->fdt_blob, but then I got confused about how a migration to an older > version would occur (e.g. QEMU 7.2 with ms->fdt to a QEMU 7.0 with > spapr->fdt_blob). > > Migration to a newer QEMU would require us to move the spapr->version_id to 4 > and then handle the old version accordingly in spapr_post_load(). > > Probably something to think about after this work is accepted. Fair enough. I'm confident the migration semantics can be worked out, but it will require some care.
On 05/08/2022 19:39, Daniel Henrique Barboza wrote: > The pSeries machine never bothered with the common machine->fdt > attribute. We do all the FDT related work using spapr->fdt_blob. > > We're going to introduce HMP commands to read and save the FDT, which > will rely on setting machine->fdt properly to work across all machine > archs/types. Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? Another thing is that on every HMP dump I'd probably rebuild the entire FDT for the reasons David explained. Thanks, > > Let's set machine->fdt in the two places where we manipulate the FDT: > spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what > we want is a way to access the FDT from HMP, not replace > spapr->fdt_blob. > > Cc: Cédric Le Goater <clg@kaod.org> > Cc: qemu-ppc@nongnu.org > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr.c | 6 ++++++ > hw/ppc/spapr_hcall.c | 8 ++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bc9ba6e6dc..94c90f0351 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine) > spapr->fdt_initial_size = spapr->fdt_size; > spapr->fdt_blob = fdt; > > + /* > + * Set the common machine->fdt pointer to enable support > + * for 'dumpdtb' and 'info fdt' commands. > + */ > + machine->fdt = fdt; > + > /* Set up the entry state */ > first_ppc_cpu->env.gpr[5] = 0; > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index a8d4a6bcf0..0079bc6fdc 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, > spapr->fdt_initial_size = spapr->fdt_size; > spapr->fdt_blob = fdt; > > + /* > + * Set the machine->fdt pointer again since we just freed > + * it above (by freeing spapr->fdt_blob). We set this > + * pointer to enable support for 'dumpdtb' and 'info fdt' > + * HMP commands. > + */ > + MACHINE(spapr)->fdt = fdt; > + > return H_SUCCESS; > } >
On Fri, Aug 19, 2022 at 12:11:40PM +1000, Alexey Kardashevskiy wrote: > > > On 05/08/2022 19:39, Daniel Henrique Barboza wrote: > > The pSeries machine never bothered with the common machine->fdt > > attribute. We do all the FDT related work using spapr->fdt_blob. > > > > We're going to introduce HMP commands to read and save the FDT, which > > will rely on setting machine->fdt properly to work across all machine > > archs/types. > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property > enough? Huh.. I didn't think of that. For dumpdtb you could be right, that you might be able to use existing qom commands to extract the property. Would need to check that the size is is handled properly, fdt's are a bit weird in having their size "in band". "info fdt" etc. obviously have additional funtionality in formatting the contents more helpfully. > Another thing is that on every HMP dump I'd probably rebuild the entire FDT > for the reasons David explained. Thanks, This would require per-machine hooks, however.
On 8/18/22 23:11, Alexey Kardashevskiy wrote: > > > On 05/08/2022 19:39, Daniel Henrique Barboza wrote: >> The pSeries machine never bothered with the common machine->fdt >> attribute. We do all the FDT related work using spapr->fdt_blob. >> >> We're going to introduce HMP commands to read and save the FDT, which >> will rely on setting machine->fdt properly to work across all machine >> archs/types. > > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? I tried to do the minimal changes needed for the commands to work. ms::fdt is one of the few MachineState fields that hasn't been QOMified by machine_class_init() yet. All pre-existing code that uses ms::fdt are using the pointer directly. To make a QOMified use of it would require extra patches in machine.c to QOMify the property first. There's also the issue with how each machine is creating the FDT. Most are using helpers from device_tree.c, some are creating it from scratch, others required a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify the use of ms::fdt we would need some machine hooks that standardize all that. I believe it's worth the trouble, but it would be too much to do right now. Thanks, Daniel > > Another thing is that on every HMP dump I'd probably rebuild the entire FDT for the reasons David explained. Thanks, > > >> >> Let's set machine->fdt in the two places where we manipulate the FDT: >> spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what >> we want is a way to access the FDT from HMP, not replace >> spapr->fdt_blob. >> >> Cc: Cédric Le Goater <clg@kaod.org> >> Cc: qemu-ppc@nongnu.org >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/ppc/spapr.c | 6 ++++++ >> hw/ppc/spapr_hcall.c | 8 ++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index bc9ba6e6dc..94c90f0351 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine) >> spapr->fdt_initial_size = spapr->fdt_size; >> spapr->fdt_blob = fdt; >> + /* >> + * Set the common machine->fdt pointer to enable support >> + * for 'dumpdtb' and 'info fdt' commands. >> + */ >> + machine->fdt = fdt; >> + >> /* Set up the entry state */ >> first_ppc_cpu->env.gpr[5] = 0; >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index a8d4a6bcf0..0079bc6fdc 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, >> spapr->fdt_initial_size = spapr->fdt_size; >> spapr->fdt_blob = fdt; >> + /* >> + * Set the machine->fdt pointer again since we just freed >> + * it above (by freeing spapr->fdt_blob). We set this >> + * pointer to enable support for 'dumpdtb' and 'info fdt' >> + * HMP commands. >> + */ >> + MACHINE(spapr)->fdt = fdt; >> + >> return H_SUCCESS; >> } >
On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: > > > On 8/18/22 23:11, Alexey Kardashevskiy wrote: > > > > > > On 05/08/2022 19:39, Daniel Henrique Barboza wrote: > > > The pSeries machine never bothered with the common machine->fdt > > > attribute. We do all the FDT related work using spapr->fdt_blob. > > > > > > We're going to introduce HMP commands to read and save the FDT, which > > > will rely on setting machine->fdt properly to work across all machine > > > archs/types. > > > > > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? > > I tried to do the minimal changes needed for the commands to work. ms::fdt is > one of the few MachineState fields that hasn't been QOMified by > machine_class_init() yet. All pre-existing code that uses ms::fdt are using the > pointer directly. To make a QOMified use of it would require extra patches > in machine.c to QOMify the property first. > > There's also the issue with how each machine is creating the FDT. Most are using > helpers from device_tree.c, some are creating it from scratch, others required > a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify > the use of ms::fdt we would need some machine hooks that standardize all that. > I believe it's worth the trouble, but it would be too much to do > right now. Hmm.. I think this depends on what you mean by "QOM"ify exactly. If you're meaning make the full DT representation QOM objects, that you can look into in detail, then, yes, that's pretty complicated. I suspect what Alexey was suggesting though, was merely to make ms::fdt accessible as a single bytestring property on the machine QOM object. Effectively it's just "dumpdtb" but as a property get. I'm not 100% certain if QOM can safely represent arbitrary bytestrings as QOM properties, which would need checking.
On 22/08/2022 13:05, David Gibson wrote: > On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: >> >> >> On 8/18/22 23:11, Alexey Kardashevskiy wrote: >>> >>> >>> On 05/08/2022 19:39, Daniel Henrique Barboza wrote: >>>> The pSeries machine never bothered with the common machine->fdt >>>> attribute. We do all the FDT related work using spapr->fdt_blob. >>>> >>>> We're going to introduce HMP commands to read and save the FDT, which >>>> will rely on setting machine->fdt properly to work across all machine >>>> archs/types. >>> >>> >>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? >> >> I tried to do the minimal changes needed for the commands to work. ms::fdt is >> one of the few MachineState fields that hasn't been QOMified by >> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the >> pointer directly. To make a QOMified use of it would require extra patches >> in machine.c to QOMify the property first. >> >> There's also the issue with how each machine is creating the FDT. Most are using >> helpers from device_tree.c, some are creating it from scratch, others required >> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify >> the use of ms::fdt we would need some machine hooks that standardize all that. >> I believe it's worth the trouble, but it would be too much to do >> right now. > > Hmm.. I think this depends on what you mean by "QOM"ify exactly. If > you're meaning make the full DT representation QOM objects, that you > can look into in detail, then, yes, that's pretty complicated. > > I suspect what Alexey was suggesting though, was merely to make > ms::fdt accessible as a single bytestring property on the machine QOM > object. Effectively it's just "dumpdtb" but as a property get. Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. > I'm not 100% certain if QOM can safely represent arbitrary bytestrings > as QOM properties, which would need checking. I am not sure either but rather than adding another command to HMP, I'd explore this option first.
On 8/22/22 00:29, Alexey Kardashevskiy wrote: > > > On 22/08/2022 13:05, David Gibson wrote: >> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: >>> >>> >>> On 8/18/22 23:11, Alexey Kardashevskiy wrote: >>>> >>>> >>>> On 05/08/2022 19:39, Daniel Henrique Barboza wrote: >>>>> The pSeries machine never bothered with the common machine->fdt >>>>> attribute. We do all the FDT related work using spapr->fdt_blob. >>>>> >>>>> We're going to introduce HMP commands to read and save the FDT, which >>>>> will rely on setting machine->fdt properly to work across all machine >>>>> archs/types. >>>> >>>> >>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? >>> >>> I tried to do the minimal changes needed for the commands to work. ms::fdt is >>> one of the few MachineState fields that hasn't been QOMified by >>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the >>> pointer directly. To make a QOMified use of it would require extra patches >>> in machine.c to QOMify the property first. >>> >>> There's also the issue with how each machine is creating the FDT. Most are using >>> helpers from device_tree.c, some are creating it from scratch, others required >>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify >>> the use of ms::fdt we would need some machine hooks that standardize all that. >>> I believe it's worth the trouble, but it would be too much to do >>> right now. >> >> Hmm.. I think this depends on what you mean by "QOM"ify exactly. If >> you're meaning make the full DT representation QOM objects, that you >> can look into in detail, then, yes, that's pretty complicated. >> >> I suspect what Alexey was suggesting though, was merely to make >> ms::fdt accessible as a single bytestring property on the machine QOM >> object. Effectively it's just "dumpdtb" but as a property get. > > > Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. > > >> I'm not 100% certain if QOM can safely represent arbitrary bytestrings >> as QOM properties, which would need checking. > > I am not sure either but rather than adding another command to HMP, I'd explore this option first. I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible that the current "-machine dumpdtb", an extra machine option that would cause the guest to exit after writing the dtb. And 'info fdt' is a new command that makes it easier to inspect specific nodes/props. I don't see how making ms::fdt being retrievable by object_property_get() internally (remember that ms::fdt it's not fully QOMified, so there's no introspection of its value from the QEMU monitor) would make any of these new HMP commands obsolete. Thanks, Daniel > > >
On 22/08/2022 20:30, Daniel Henrique Barboza wrote: > > > On 8/22/22 00:29, Alexey Kardashevskiy wrote: >> >> >> On 22/08/2022 13:05, David Gibson wrote: >>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: >>>> >>>> >>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote: >>>>> >>>>> >>>>> On 05/08/2022 19:39, Daniel Henrique Barboza wrote: >>>>>> The pSeries machine never bothered with the common machine->fdt >>>>>> attribute. We do all the FDT related work using spapr->fdt_blob. >>>>>> >>>>>> We're going to introduce HMP commands to read and save the FDT, which >>>>>> will rely on setting machine->fdt properly to work across all machine >>>>>> archs/types. >>>>> >>>>> >>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt >>>>> property enough? >>>> >>>> I tried to do the minimal changes needed for the commands to work. >>>> ms::fdt is >>>> one of the few MachineState fields that hasn't been QOMified by >>>> machine_class_init() yet. All pre-existing code that uses ms::fdt >>>> are using the >>>> pointer directly. To make a QOMified use of it would require extra >>>> patches >>>> in machine.c to QOMify the property first. >>>> >>>> There's also the issue with how each machine is creating the FDT. >>>> Most are using >>>> helpers from device_tree.c, some are creating it from scratch, >>>> others required >>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To >>>> really QOMify >>>> the use of ms::fdt we would need some machine hooks that standardize >>>> all that. >>>> I believe it's worth the trouble, but it would be too much to do >>>> right now. >>> >>> Hmm.. I think this depends on what you mean by "QOM"ify exactly. If >>> you're meaning make the full DT representation QOM objects, that you >>> can look into in detail, then, yes, that's pretty complicated. >>> >>> I suspect what Alexey was suggesting though, was merely to make >>> ms::fdt accessible as a single bytestring property on the machine QOM >>> object. Effectively it's just "dumpdtb" but as a property get. >> >> >> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. >> >> >>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings >>> as QOM properties, which would need checking. >> >> I am not sure either but rather than adding another command to HMP, >> I'd explore this option first. > > > I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more > flexible > that the current "-machine dumpdtb", an extra machine option that would > cause > the guest to exit after writing the dtb True. Especially with CAS :) > And 'info fdt' is a new command > that > makes it easier to inspect specific nodes/props. btw what is this new command going to do? decompile the tree or save dtb? > I don't see how making ms::fdt being retrievable by > object_property_get() internally > (remember that ms::fdt it's not fully QOMified, so there's no > introspection of its > value from the QEMU monitor) would make any of these new HMP commands > obsolete. Well, there are QMP and HMP and my feeling was that HMP is slowly getting deprecated or something and QMP is the superior one. So I thought since this FDT is a property and there is no associated action with it, making it a property would do. For ages I've been using a python3 script to talk to QMP as HMP is really quite limited, the only thing in HMP which is not in QMP is dumping memory ("x", "xp"), in this case I wrap HMP into QMP and keep using QMP :) > > > Thanks, > > > Daniel > >> >> >>
On 8/23/22 05:58, Alexey Kardashevskiy wrote: > > > On 22/08/2022 20:30, Daniel Henrique Barboza wrote: >> >> >> On 8/22/22 00:29, Alexey Kardashevskiy wrote: >>> >>> >>> On 22/08/2022 13:05, David Gibson wrote: >>>> On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: >>>>> >>>>> >>>>> On 8/18/22 23:11, Alexey Kardashevskiy wrote: >>>>>> >>>>>> >>>>>> On 05/08/2022 19:39, Daniel Henrique Barboza wrote: >>>>>>> The pSeries machine never bothered with the common machine->fdt >>>>>>> attribute. We do all the FDT related work using spapr->fdt_blob. >>>>>>> >>>>>>> We're going to introduce HMP commands to read and save the FDT, which >>>>>>> will rely on setting machine->fdt properly to work across all machine >>>>>>> archs/types. >>>>>> >>>>>> >>>>>> Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? >>>>> >>>>> I tried to do the minimal changes needed for the commands to work. ms::fdt is >>>>> one of the few MachineState fields that hasn't been QOMified by >>>>> machine_class_init() yet. All pre-existing code that uses ms::fdt are using the >>>>> pointer directly. To make a QOMified use of it would require extra patches >>>>> in machine.c to QOMify the property first. >>>>> >>>>> There's also the issue with how each machine is creating the FDT. Most are using >>>>> helpers from device_tree.c, some are creating it from scratch, others required >>>>> a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify >>>>> the use of ms::fdt we would need some machine hooks that standardize all that. >>>>> I believe it's worth the trouble, but it would be too much to do >>>>> right now. >>>> >>>> Hmm.. I think this depends on what you mean by "QOM"ify exactly. If >>>> you're meaning make the full DT representation QOM objects, that you >>>> can look into in detail, then, yes, that's pretty complicated. >>>> >>>> I suspect what Alexey was suggesting though, was merely to make >>>> ms::fdt accessible as a single bytestring property on the machine QOM >>>> object. Effectively it's just "dumpdtb" but as a property get. >>> >>> >>> Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. >>> >>> >>>> I'm not 100% certain if QOM can safely represent arbitrary bytestrings >>>> as QOM properties, which would need checking. >>> >>> I am not sure either but rather than adding another command to HMP, I'd explore this option first. >> >> >> I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible >> that the current "-machine dumpdtb", an extra machine option that would cause >> the guest to exit after writing the dtb > > True. Especially with CAS :) > >> And 'info fdt' is a new command that >> makes it easier to inspect specific nodes/props. > > btw what is this new command going to do? decompile the tree or save dtb? At this moment, 'info fdt <node> [prop]' is using the current ms->fdt bytestream plus libfdt to output nodes and properties. > >> I don't see how making ms::fdt being retrievable by object_property_get() internally >> (remember that ms::fdt it's not fully QOMified, so there's no introspection of its >> value from the QEMU monitor) would make any of these new HMP commands obsolete. > > Well, there are QMP and HMP and my feeling was that HMP is slowly getting deprecated or something and QMP is the superior one. So I thought since this FDT is a property and there is no associated action with it, making it a property would do. I don't have an option about HMP being deprecated and QMP being the preferable choice. Perhaps someone else reading this thread can tell more about it. > > For ages I've been using a python3 script to talk to QMP as HMP is really quite limited, the only thing in HMP which is not in QMP is dumping memory ("x", "xp"), in this case I wrap HMP into QMP and keep using QMP :) Apparently we have a QMP enthusiast over here ;) Daniel > > >> >> >> Thanks, >> >> >> Daniel >> >>> >>> >>> >
On Mon, Aug 22, 2022 at 07:30:36AM -0300, Daniel Henrique Barboza wrote: > > > On 8/22/22 00:29, Alexey Kardashevskiy wrote: > > > > > > On 22/08/2022 13:05, David Gibson wrote: > > > On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: > > > > > > > > > > > > On 8/18/22 23:11, Alexey Kardashevskiy wrote: > > > > > > > > > > > > > > > On 05/08/2022 19:39, Daniel Henrique Barboza wrote: > > > > > > The pSeries machine never bothered with the common machine->fdt > > > > > > attribute. We do all the FDT related work using spapr->fdt_blob. > > > > > > > > > > > > We're going to introduce HMP commands to read and save the FDT, which > > > > > > will rely on setting machine->fdt properly to work across all machine > > > > > > archs/types. > > > > > > > > > > > > > > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? > > > > > > > > I tried to do the minimal changes needed for the commands to work. ms::fdt is > > > > one of the few MachineState fields that hasn't been QOMified by > > > > machine_class_init() yet. All pre-existing code that uses ms::fdt are using the > > > > pointer directly. To make a QOMified use of it would require extra patches > > > > in machine.c to QOMify the property first. > > > > > > > > There's also the issue with how each machine is creating the FDT. Most are using > > > > helpers from device_tree.c, some are creating it from scratch, others required > > > > a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify > > > > the use of ms::fdt we would need some machine hooks that standardize all that. > > > > I believe it's worth the trouble, but it would be too much to do > > > > right now. > > > > > > Hmm.. I think this depends on what you mean by "QOM"ify exactly. If > > > you're meaning make the full DT representation QOM objects, that you > > > can look into in detail, then, yes, that's pretty complicated. > > > > > > I suspect what Alexey was suggesting though, was merely to make > > > ms::fdt accessible as a single bytestring property on the machine QOM > > > object. Effectively it's just "dumpdtb" but as a property get. > > > > > > Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. > > > > > > > I'm not 100% certain if QOM can safely represent arbitrary bytestrings > > > as QOM properties, which would need checking. > > > > I am not sure either but rather than adding another command to HMP, I'd explore this option first. > > > I'm not sure what you mean by that. The HMP version of 'dumpdtb' is more flexible > that the current "-machine dumpdtb", an extra machine option that would cause > the guest to exit after writing the dtb. And 'info fdt' is a new command that > makes it easier to inspect specific nodes/props. > > I don't see how making ms::fdt being retrievable by object_property_get() internally > (remember that ms::fdt it's not fully QOMified, so there's no introspection of its > value from the QEMU monitor) would make any of these new HMP commands obsolete. I believe what we were thinking is if the dtb (as a single bytestring) can be retrieved with a qom-get on a suitable property on the machine, that might make things marginally simpler than adding a new command. I'm not certain if the JSON format of the QMP responses can safely encode an arbitrary bytestring, though (as opoosed to a Unicode string).
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bc9ba6e6dc..94c90f0351 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1713,6 +1713,12 @@ static void spapr_machine_reset(MachineState *machine) spapr->fdt_initial_size = spapr->fdt_size; spapr->fdt_blob = fdt; + /* + * Set the common machine->fdt pointer to enable support + * for 'dumpdtb' and 'info fdt' commands. + */ + machine->fdt = fdt; + /* Set up the entry state */ first_ppc_cpu->env.gpr[5] = 0; diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index a8d4a6bcf0..0079bc6fdc 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1256,6 +1256,14 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, spapr->fdt_initial_size = spapr->fdt_size; spapr->fdt_blob = fdt; + /* + * Set the machine->fdt pointer again since we just freed + * it above (by freeing spapr->fdt_blob). We set this + * pointer to enable support for 'dumpdtb' and 'info fdt' + * HMP commands. + */ + MACHINE(spapr)->fdt = fdt; + return H_SUCCESS; }
The pSeries machine never bothered with the common machine->fdt attribute. We do all the FDT related work using spapr->fdt_blob. We're going to introduce HMP commands to read and save the FDT, which will rely on setting machine->fdt properly to work across all machine archs/types. Let's set machine->fdt in the two places where we manipulate the FDT: spapr_machine_reset() and CAS. spapr->fdt_blob is left untouched: what we want is a way to access the FDT from HMP, not replace spapr->fdt_blob. Cc: Cédric Le Goater <clg@kaod.org> Cc: qemu-ppc@nongnu.org Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr.c | 6 ++++++ hw/ppc/spapr_hcall.c | 8 ++++++++ 2 files changed, 14 insertions(+)