| Message ID | 20220826141150.7201-16-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | QMP/HMP: add 'dumpdtb' and 'info fdt' commands | expand |
On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: > Reading the FDT requires that the user saves the fdt_blob and then use > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong > use case when we need to compare two FDTs, but it's a lot of steps if > you want to do quick check on a certain node or property. > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it > to the user. This can be used to check the FDT on running machines > without having to save the blob and use 'dtc'. > > The implementation is based on the premise that the machine thas a FDT > created using libfdt and pointed by 'machine->fdt'. As long as this > pre-requisite is met the machine should be able to support it. > > For now we're going to add the required QMP/HMP boilerplate and the > capability of printing the name of the properties of a given node. Next > patches will extend 'info fdt' to be able to print nodes recursively, > and then individual properties. > > This command will always be executed in-band (i.e. holding BQL), > avoiding potential race conditions with machines that might change the > FDT during runtime (e.g. PowerPC 'pseries' machine). > > 'info fdt' is not something that we expect to be used aside from debugging, > so we're implementing it in QMP as 'x-query-fdt'. > > This is an example of 'info fdt' fetching the '/chosen' node of the > pSeries machine: > > (qemu) info fdt /chosen > chosen { > ibm,architecture-vec-5; > rng-seed; > ibm,arch-vec-5-platform-support; > linux,pci-probe-only; > stdout-path; > linux,stdout-path; > qemu,graphic-depth; > qemu,graphic-height; > qemu,graphic-width; > }; > > And the same node for the aarch64 'virt' machine: > > (qemu) info fdt /chosen > chosen { > stdout-path; > rng-seed; > kaslr-seed; > }; So, I'm reasonably convinced allowing dumping the whole dtb from qmp/hmp is useful. I'm less convined that info fdt is worth the additional complexity it incurs. Note that as well as being able to decompile a whole dtb using dtc, you can also extract and list specific properties from a dtb blob using the 'fdtget' tool which is part of the dtc tree. > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hmp-commands-info.hx | 13 ++++++++++ > include/monitor/hmp.h | 1 + > include/sysemu/device_tree.h | 4 +++ > monitor/hmp-cmds.c | 13 ++++++++++ > monitor/qmp-cmds.c | 12 +++++++++ > qapi/machine.json | 19 +++++++++++++++ > softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ > 7 files changed, 109 insertions(+) > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > index 188d9ece3b..743b48865d 100644 > --- a/hmp-commands-info.hx > +++ b/hmp-commands-info.hx > @@ -921,3 +921,16 @@ SRST > ``stats`` > Show runtime-collected statistics > ERST > + > + { > + .name = "fdt", > + .args_type = "nodepath:s", > + .params = "nodepath", > + .help = "show firmware device tree node given its path", > + .cmd = hmp_info_fdt, > + }, > + > +SRST > + ``info fdt`` > + Show a firmware device tree node given its path. Requires libfdt. > +ERST > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > index d7f324da59..c0883dd1e3 100644 > --- a/include/monitor/hmp.h > +++ b/include/monitor/hmp.h > @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_dumpdtb(Monitor *mon, const QDict *qdict); > +void hmp_info_fdt(Monitor *mon, const QDict *qdict); > void hmp_human_readable_text_helper(Monitor *mon, > HumanReadableText *(*qmp_handler)(Error **)); > void hmp_info_stats(Monitor *mon, const QDict *qdict); > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index bf7684e4ed..057d13e397 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -14,6 +14,8 @@ > #ifndef DEVICE_TREE_H > #define DEVICE_TREE_H > > +#include "qapi/qapi-types-common.h" > + > void *create_device_tree(int *sizep); > void *load_device_tree(const char *filename_path, int *sizep); > #ifdef CONFIG_LINUX > @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); > > void qemu_fdt_dumpdtb(void *fdt, int size); > void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, > + Error **errp); > > /** > * qemu_fdt_setprop_sized_cells_from_array: > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 1c7bfd3b9d..93a4103afa 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, local_err); > } > } > + > +void hmp_info_fdt(Monitor *mon, const QDict *qdict) > +{ > + const char *nodepath = qdict_get_str(qdict, "nodepath"); > + Error *err = NULL; > + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); > + > + if (hmp_handle_error(mon, err)) { > + return; > + } > + > + monitor_printf(mon, "%s", info->human_readable_text); > +} > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 8415aca08c..db2c6aa7da 100644 > --- a/monitor/qmp-cmds.c > +++ b/monitor/qmp-cmds.c > @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) > { > return qemu_fdt_qmp_dumpdtb(filename, errp); > } > + > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > +{ > + return qemu_fdt_qmp_query_fdt(nodepath, errp); > +} > #else > void qmp_dumpdtb(const char *filename, Error **errp) > { > error_setg(errp, "dumpdtb requires libfdt"); > } > + > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > +{ > + error_setg(errp, "this command requires libfdt"); > + > + return NULL; > +} > #endif > diff --git a/qapi/machine.json b/qapi/machine.json > index aeb013f3dd..96cff541ca 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1681,3 +1681,22 @@ > ## > { 'command': 'dumpdtb', > 'data': { 'filename': 'str' } } > + > +## > +# @x-query-fdt: > +# > +# Query for FDT element (node or property). Requires 'libfdt'. > +# > +# @nodepath: the path of the FDT node to be retrieved > +# > +# Features: > +# @unstable: This command is meant for debugging. > +# > +# Returns: FDT node > +# > +# Since: 7.2 > +## > +{ 'command': 'x-query-fdt', > + 'data': { 'nodepath': 'str' }, > + 'returns': 'HumanReadableText', > + 'features': [ 'unstable' ] } > diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c > index cd487ddd4d..6b15f6ace2 100644 > --- a/softmmu/device_tree.c > +++ b/softmmu/device_tree.c > @@ -18,6 +18,7 @@ > #endif > > #include "qapi/error.h" > +#include "qapi/type-helpers.h" > #include "qemu/error-report.h" > #include "qemu/option.h" > #include "qemu/bswap.h" > @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp) > > error_setg(errp, "Error when saving machine FDT to file %s", filename); > } > + > +static void fdt_format_node(GString *buf, int node, int depth) > +{ > + const struct fdt_property *prop = NULL; > + const char *propname = NULL; > + void *fdt = current_machine->fdt; > + int padding = depth * 4; > + int property = 0; > + int prop_size; > + > + g_string_append_printf(buf, "%*s%s {\n", padding, "", > + fdt_get_name(fdt, node, NULL)); > + > + padding += 4; > + > + fdt_for_each_property_offset(property, fdt, node) { > + prop = fdt_get_property_by_offset(fdt, property, &prop_size); > + propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); > + > + g_string_append_printf(buf, "%*s%s;\n", padding, "", propname); > + } > + > + padding -= 4; > + g_string_append_printf(buf, "%*s};\n", padding, ""); > +} > + > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp) > +{ > + g_autoptr(GString) buf = g_string_new(""); > + int node; > + > + if (!current_machine->fdt) { > + error_setg(errp, "Unable to find the machine FDT"); > + return NULL; > + } > + > + node = fdt_path_offset(current_machine->fdt, nodepath); > + if (node < 0) { > + error_setg(errp, "node '%s' not found in FDT", nodepath); > + return NULL; > + } > + > + fdt_format_node(buf, node, 0); > + > + return human_readable_text_from_str(buf); > +}
On 8/29/22 00:34, David Gibson wrote: > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: >> Reading the FDT requires that the user saves the fdt_blob and then use >> 'dtc' to read the contents. Saving the file and using 'dtc' is a strong >> use case when we need to compare two FDTs, but it's a lot of steps if >> you want to do quick check on a certain node or property. >> >> 'info fdt' retrieves FDT nodes (and properties, later on) and print it >> to the user. This can be used to check the FDT on running machines >> without having to save the blob and use 'dtc'. >> >> The implementation is based on the premise that the machine thas a FDT >> created using libfdt and pointed by 'machine->fdt'. As long as this >> pre-requisite is met the machine should be able to support it. >> >> For now we're going to add the required QMP/HMP boilerplate and the >> capability of printing the name of the properties of a given node. Next >> patches will extend 'info fdt' to be able to print nodes recursively, >> and then individual properties. >> >> This command will always be executed in-band (i.e. holding BQL), >> avoiding potential race conditions with machines that might change the >> FDT during runtime (e.g. PowerPC 'pseries' machine). >> >> 'info fdt' is not something that we expect to be used aside from debugging, >> so we're implementing it in QMP as 'x-query-fdt'. >> >> This is an example of 'info fdt' fetching the '/chosen' node of the >> pSeries machine: >> >> (qemu) info fdt /chosen >> chosen { >> ibm,architecture-vec-5; >> rng-seed; >> ibm,arch-vec-5-platform-support; >> linux,pci-probe-only; >> stdout-path; >> linux,stdout-path; >> qemu,graphic-depth; >> qemu,graphic-height; >> qemu,graphic-width; >> }; >> >> And the same node for the aarch64 'virt' machine: >> >> (qemu) info fdt /chosen >> chosen { >> stdout-path; >> rng-seed; >> kaslr-seed; >> }; > > So, I'm reasonably convinced allowing dumping the whole dtb from > qmp/hmp is useful. I'm less convined that info fdt is worth the > additional complexity it incurs. Note that as well as being able to > decompile a whole dtb using dtc, you can also extract and list > specific properties from a dtb blob using the 'fdtget' tool which is > part of the dtc tree. What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted FDT in a file with an extra option? That was possible because of the format helpers introduced for 'info fdt'. The idea is that since we're able to format a FDT in DTS format, we can also write the FDT in text format without relying on DTC to decode it. If we think that this 'dumpdtb' capability is worth having, I can respin the patches without 'info fdt' but adding these helpers to enable this 'dumpdtb' support. If not, then we can just remove patches 15-21 and be done with it. Thanks, Daniel > >> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hmp-commands-info.hx | 13 ++++++++++ >> include/monitor/hmp.h | 1 + >> include/sysemu/device_tree.h | 4 +++ >> monitor/hmp-cmds.c | 13 ++++++++++ >> monitor/qmp-cmds.c | 12 +++++++++ >> qapi/machine.json | 19 +++++++++++++++ >> softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ >> 7 files changed, 109 insertions(+) >> >> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx >> index 188d9ece3b..743b48865d 100644 >> --- a/hmp-commands-info.hx >> +++ b/hmp-commands-info.hx >> @@ -921,3 +921,16 @@ SRST >> ``stats`` >> Show runtime-collected statistics >> ERST >> + >> + { >> + .name = "fdt", >> + .args_type = "nodepath:s", >> + .params = "nodepath", >> + .help = "show firmware device tree node given its path", >> + .cmd = hmp_info_fdt, >> + }, >> + >> +SRST >> + ``info fdt`` >> + Show a firmware device tree node given its path. Requires libfdt. >> +ERST >> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >> index d7f324da59..c0883dd1e3 100644 >> --- a/include/monitor/hmp.h >> +++ b/include/monitor/hmp.h >> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >> void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >> void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >> void hmp_dumpdtb(Monitor *mon, const QDict *qdict); >> +void hmp_info_fdt(Monitor *mon, const QDict *qdict); >> void hmp_human_readable_text_helper(Monitor *mon, >> HumanReadableText *(*qmp_handler)(Error **)); >> void hmp_info_stats(Monitor *mon, const QDict *qdict); >> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h >> index bf7684e4ed..057d13e397 100644 >> --- a/include/sysemu/device_tree.h >> +++ b/include/sysemu/device_tree.h >> @@ -14,6 +14,8 @@ >> #ifndef DEVICE_TREE_H >> #define DEVICE_TREE_H >> >> +#include "qapi/qapi-types-common.h" >> + >> void *create_device_tree(int *sizep); >> void *load_device_tree(const char *filename_path, int *sizep); >> #ifdef CONFIG_LINUX >> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); >> >> void qemu_fdt_dumpdtb(void *fdt, int size); >> void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); >> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, >> + Error **errp); >> >> /** >> * qemu_fdt_setprop_sized_cells_from_array: >> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >> index 1c7bfd3b9d..93a4103afa 100644 >> --- a/monitor/hmp-cmds.c >> +++ b/monitor/hmp-cmds.c >> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) >> hmp_handle_error(mon, local_err); >> } >> } >> + >> +void hmp_info_fdt(Monitor *mon, const QDict *qdict) >> +{ >> + const char *nodepath = qdict_get_str(qdict, "nodepath"); >> + Error *err = NULL; >> + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); >> + >> + if (hmp_handle_error(mon, err)) { >> + return; >> + } >> + >> + monitor_printf(mon, "%s", info->human_readable_text); >> +} >> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c >> index 8415aca08c..db2c6aa7da 100644 >> --- a/monitor/qmp-cmds.c >> +++ b/monitor/qmp-cmds.c >> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) >> { >> return qemu_fdt_qmp_dumpdtb(filename, errp); >> } >> + >> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) >> +{ >> + return qemu_fdt_qmp_query_fdt(nodepath, errp); >> +} >> #else >> void qmp_dumpdtb(const char *filename, Error **errp) >> { >> error_setg(errp, "dumpdtb requires libfdt"); >> } >> + >> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) >> +{ >> + error_setg(errp, "this command requires libfdt"); >> + >> + return NULL; >> +} >> #endif >> diff --git a/qapi/machine.json b/qapi/machine.json >> index aeb013f3dd..96cff541ca 100644 >> --- a/qapi/machine.json >> +++ b/qapi/machine.json >> @@ -1681,3 +1681,22 @@ >> ## >> { 'command': 'dumpdtb', >> 'data': { 'filename': 'str' } } >> + >> +## >> +# @x-query-fdt: >> +# >> +# Query for FDT element (node or property). Requires 'libfdt'. >> +# >> +# @nodepath: the path of the FDT node to be retrieved >> +# >> +# Features: >> +# @unstable: This command is meant for debugging. >> +# >> +# Returns: FDT node >> +# >> +# Since: 7.2 >> +## >> +{ 'command': 'x-query-fdt', >> + 'data': { 'nodepath': 'str' }, >> + 'returns': 'HumanReadableText', >> + 'features': [ 'unstable' ] } >> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c >> index cd487ddd4d..6b15f6ace2 100644 >> --- a/softmmu/device_tree.c >> +++ b/softmmu/device_tree.c >> @@ -18,6 +18,7 @@ >> #endif >> >> #include "qapi/error.h" >> +#include "qapi/type-helpers.h" >> #include "qemu/error-report.h" >> #include "qemu/option.h" >> #include "qemu/bswap.h" >> @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp) >> >> error_setg(errp, "Error when saving machine FDT to file %s", filename); >> } >> + >> +static void fdt_format_node(GString *buf, int node, int depth) >> +{ >> + const struct fdt_property *prop = NULL; >> + const char *propname = NULL; >> + void *fdt = current_machine->fdt; >> + int padding = depth * 4; >> + int property = 0; >> + int prop_size; >> + >> + g_string_append_printf(buf, "%*s%s {\n", padding, "", >> + fdt_get_name(fdt, node, NULL)); >> + >> + padding += 4; >> + >> + fdt_for_each_property_offset(property, fdt, node) { >> + prop = fdt_get_property_by_offset(fdt, property, &prop_size); >> + propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); >> + >> + g_string_append_printf(buf, "%*s%s;\n", padding, "", propname); >> + } >> + >> + padding -= 4; >> + g_string_append_printf(buf, "%*s};\n", padding, ""); >> +} >> + >> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp) >> +{ >> + g_autoptr(GString) buf = g_string_new(""); >> + int node; >> + >> + if (!current_machine->fdt) { >> + error_setg(errp, "Unable to find the machine FDT"); >> + return NULL; >> + } >> + >> + node = fdt_path_offset(current_machine->fdt, nodepath); >> + if (node < 0) { >> + error_setg(errp, "node '%s' not found in FDT", nodepath); >> + return NULL; >> + } >> + >> + fdt_format_node(buf, node, 0); >> + >> + return human_readable_text_from_str(buf); >> +} >
On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote: > > > On 8/29/22 00:34, David Gibson wrote: > > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: > > > Reading the FDT requires that the user saves the fdt_blob and then use > > > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong > > > use case when we need to compare two FDTs, but it's a lot of steps if > > > you want to do quick check on a certain node or property. > > > > > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it > > > to the user. This can be used to check the FDT on running machines > > > without having to save the blob and use 'dtc'. > > > > > > The implementation is based on the premise that the machine thas a FDT > > > created using libfdt and pointed by 'machine->fdt'. As long as this > > > pre-requisite is met the machine should be able to support it. > > > > > > For now we're going to add the required QMP/HMP boilerplate and the > > > capability of printing the name of the properties of a given node. Next > > > patches will extend 'info fdt' to be able to print nodes recursively, > > > and then individual properties. > > > > > > This command will always be executed in-band (i.e. holding BQL), > > > avoiding potential race conditions with machines that might change the > > > FDT during runtime (e.g. PowerPC 'pseries' machine). > > > > > > 'info fdt' is not something that we expect to be used aside from debugging, > > > so we're implementing it in QMP as 'x-query-fdt'. > > > > > > This is an example of 'info fdt' fetching the '/chosen' node of the > > > pSeries machine: > > > > > > (qemu) info fdt /chosen > > > chosen { > > > ibm,architecture-vec-5; > > > rng-seed; > > > ibm,arch-vec-5-platform-support; > > > linux,pci-probe-only; > > > stdout-path; > > > linux,stdout-path; > > > qemu,graphic-depth; > > > qemu,graphic-height; > > > qemu,graphic-width; > > > }; > > > > > > And the same node for the aarch64 'virt' machine: > > > > > > (qemu) info fdt /chosen > > > chosen { > > > stdout-path; > > > rng-seed; > > > kaslr-seed; > > > }; > > > > So, I'm reasonably convinced allowing dumping the whole dtb from > > qmp/hmp is useful. I'm less convined that info fdt is worth the > > additional complexity it incurs. Note that as well as being able to > > decompile a whole dtb using dtc, you can also extract and list > > specific properties from a dtb blob using the 'fdtget' tool which is > > part of the dtc tree. > > What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted > FDT in a file with an extra option? That was possible because of the > format helpers introduced for 'info fdt'. The idea is that since we're > able to format a FDT in DTS format, we can also write the FDT in text > format without relying on DTC to decode it. Since it's mostly the same code, I think it's reasonable to throw in if the info fdt stuff is there, but I don't think it's worth including without that. As a whole, I remain dubious that (info fdt + dumpdts) is worth the complexity cost. People with more practical experience debugging the embedded ARM platforms might have a different opinion if they thing info fdt would be really useful though. > If we think that this 'dumpdtb' capability is worth having, I can respin > the patches without 'info fdt' but adding these helpers to enable this > 'dumpdtb' support. If not, then we can just remove patches 15-21 and > be done with it. > > > Thanks, > > > Daniel > > > > > > > > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > > --- > > > hmp-commands-info.hx | 13 ++++++++++ > > > include/monitor/hmp.h | 1 + > > > include/sysemu/device_tree.h | 4 +++ > > > monitor/hmp-cmds.c | 13 ++++++++++ > > > monitor/qmp-cmds.c | 12 +++++++++ > > > qapi/machine.json | 19 +++++++++++++++ > > > softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ > > > 7 files changed, 109 insertions(+) > > > > > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > > > index 188d9ece3b..743b48865d 100644 > > > --- a/hmp-commands-info.hx > > > +++ b/hmp-commands-info.hx > > > @@ -921,3 +921,16 @@ SRST > > > ``stats`` > > > Show runtime-collected statistics > > > ERST > > > + > > > + { > > > + .name = "fdt", > > > + .args_type = "nodepath:s", > > > + .params = "nodepath", > > > + .help = "show firmware device tree node given its path", > > > + .cmd = hmp_info_fdt, > > > + }, > > > + > > > +SRST > > > + ``info fdt`` > > > + Show a firmware device tree node given its path. Requires libfdt. > > > +ERST > > > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > > > index d7f324da59..c0883dd1e3 100644 > > > --- a/include/monitor/hmp.h > > > +++ b/include/monitor/hmp.h > > > @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > > > void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > > > void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > > > void hmp_dumpdtb(Monitor *mon, const QDict *qdict); > > > +void hmp_info_fdt(Monitor *mon, const QDict *qdict); > > > void hmp_human_readable_text_helper(Monitor *mon, > > > HumanReadableText *(*qmp_handler)(Error **)); > > > void hmp_info_stats(Monitor *mon, const QDict *qdict); > > > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > > > index bf7684e4ed..057d13e397 100644 > > > --- a/include/sysemu/device_tree.h > > > +++ b/include/sysemu/device_tree.h > > > @@ -14,6 +14,8 @@ > > > #ifndef DEVICE_TREE_H > > > #define DEVICE_TREE_H > > > +#include "qapi/qapi-types-common.h" > > > + > > > void *create_device_tree(int *sizep); > > > void *load_device_tree(const char *filename_path, int *sizep); > > > #ifdef CONFIG_LINUX > > > @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); > > > void qemu_fdt_dumpdtb(void *fdt, int size); > > > void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); > > > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, > > > + Error **errp); > > > /** > > > * qemu_fdt_setprop_sized_cells_from_array: > > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > > > index 1c7bfd3b9d..93a4103afa 100644 > > > --- a/monitor/hmp-cmds.c > > > +++ b/monitor/hmp-cmds.c > > > @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) > > > hmp_handle_error(mon, local_err); > > > } > > > } > > > + > > > +void hmp_info_fdt(Monitor *mon, const QDict *qdict) > > > +{ > > > + const char *nodepath = qdict_get_str(qdict, "nodepath"); > > > + Error *err = NULL; > > > + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); > > > + > > > + if (hmp_handle_error(mon, err)) { > > > + return; > > > + } > > > + > > > + monitor_printf(mon, "%s", info->human_readable_text); > > > +} > > > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > > > index 8415aca08c..db2c6aa7da 100644 > > > --- a/monitor/qmp-cmds.c > > > +++ b/monitor/qmp-cmds.c > > > @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) > > > { > > > return qemu_fdt_qmp_dumpdtb(filename, errp); > > > } > > > + > > > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > > > +{ > > > + return qemu_fdt_qmp_query_fdt(nodepath, errp); > > > +} > > > #else > > > void qmp_dumpdtb(const char *filename, Error **errp) > > > { > > > error_setg(errp, "dumpdtb requires libfdt"); > > > } > > > + > > > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > > > +{ > > > + error_setg(errp, "this command requires libfdt"); > > > + > > > + return NULL; > > > +} > > > #endif > > > diff --git a/qapi/machine.json b/qapi/machine.json > > > index aeb013f3dd..96cff541ca 100644 > > > --- a/qapi/machine.json > > > +++ b/qapi/machine.json > > > @@ -1681,3 +1681,22 @@ > > > ## > > > { 'command': 'dumpdtb', > > > 'data': { 'filename': 'str' } } > > > + > > > +## > > > +# @x-query-fdt: > > > +# > > > +# Query for FDT element (node or property). Requires 'libfdt'. > > > +# > > > +# @nodepath: the path of the FDT node to be retrieved > > > +# > > > +# Features: > > > +# @unstable: This command is meant for debugging. > > > +# > > > +# Returns: FDT node > > > +# > > > +# Since: 7.2 > > > +## > > > +{ 'command': 'x-query-fdt', > > > + 'data': { 'nodepath': 'str' }, > > > + 'returns': 'HumanReadableText', > > > + 'features': [ 'unstable' ] } > > > diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c > > > index cd487ddd4d..6b15f6ace2 100644 > > > --- a/softmmu/device_tree.c > > > +++ b/softmmu/device_tree.c > > > @@ -18,6 +18,7 @@ > > > #endif > > > #include "qapi/error.h" > > > +#include "qapi/type-helpers.h" > > > #include "qemu/error-report.h" > > > #include "qemu/option.h" > > > #include "qemu/bswap.h" > > > @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp) > > > error_setg(errp, "Error when saving machine FDT to file %s", filename); > > > } > > > + > > > +static void fdt_format_node(GString *buf, int node, int depth) > > > +{ > > > + const struct fdt_property *prop = NULL; > > > + const char *propname = NULL; > > > + void *fdt = current_machine->fdt; > > > + int padding = depth * 4; > > > + int property = 0; > > > + int prop_size; > > > + > > > + g_string_append_printf(buf, "%*s%s {\n", padding, "", > > > + fdt_get_name(fdt, node, NULL)); > > > + > > > + padding += 4; > > > + > > > + fdt_for_each_property_offset(property, fdt, node) { > > > + prop = fdt_get_property_by_offset(fdt, property, &prop_size); > > > + propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); > > > + > > > + g_string_append_printf(buf, "%*s%s;\n", padding, "", propname); > > > + } > > > + > > > + padding -= 4; > > > + g_string_append_printf(buf, "%*s};\n", padding, ""); > > > +} > > > + > > > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp) > > > +{ > > > + g_autoptr(GString) buf = g_string_new(""); > > > + int node; > > > + > > > + if (!current_machine->fdt) { > > > + error_setg(errp, "Unable to find the machine FDT"); > > > + return NULL; > > > + } > > > + > > > + node = fdt_path_offset(current_machine->fdt, nodepath); > > > + if (node < 0) { > > > + error_setg(errp, "node '%s' not found in FDT", nodepath); > > > + return NULL; > > > + } > > > + > > > + fdt_format_node(buf, node, 0); > > > + > > > + return human_readable_text_from_str(buf); > > > +} > > >
On 8/29/22 22:50, David Gibson wrote: > On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote: >> >> >> On 8/29/22 00:34, David Gibson wrote: >>> On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: >>>> Reading the FDT requires that the user saves the fdt_blob and then use >>>> 'dtc' to read the contents. Saving the file and using 'dtc' is a strong >>>> use case when we need to compare two FDTs, but it's a lot of steps if >>>> you want to do quick check on a certain node or property. >>>> >>>> 'info fdt' retrieves FDT nodes (and properties, later on) and print it >>>> to the user. This can be used to check the FDT on running machines >>>> without having to save the blob and use 'dtc'. >>>> >>>> The implementation is based on the premise that the machine thas a FDT >>>> created using libfdt and pointed by 'machine->fdt'. As long as this >>>> pre-requisite is met the machine should be able to support it. >>>> >>>> For now we're going to add the required QMP/HMP boilerplate and the >>>> capability of printing the name of the properties of a given node. Next >>>> patches will extend 'info fdt' to be able to print nodes recursively, >>>> and then individual properties. >>>> >>>> This command will always be executed in-band (i.e. holding BQL), >>>> avoiding potential race conditions with machines that might change the >>>> FDT during runtime (e.g. PowerPC 'pseries' machine). >>>> >>>> 'info fdt' is not something that we expect to be used aside from debugging, >>>> so we're implementing it in QMP as 'x-query-fdt'. >>>> >>>> This is an example of 'info fdt' fetching the '/chosen' node of the >>>> pSeries machine: >>>> >>>> (qemu) info fdt /chosen >>>> chosen { >>>> ibm,architecture-vec-5; >>>> rng-seed; >>>> ibm,arch-vec-5-platform-support; >>>> linux,pci-probe-only; >>>> stdout-path; >>>> linux,stdout-path; >>>> qemu,graphic-depth; >>>> qemu,graphic-height; >>>> qemu,graphic-width; >>>> }; >>>> >>>> And the same node for the aarch64 'virt' machine: >>>> >>>> (qemu) info fdt /chosen >>>> chosen { >>>> stdout-path; >>>> rng-seed; >>>> kaslr-seed; >>>> }; >>> >>> So, I'm reasonably convinced allowing dumping the whole dtb from >>> qmp/hmp is useful. I'm less convined that info fdt is worth the >>> additional complexity it incurs. Note that as well as being able to >>> decompile a whole dtb using dtc, you can also extract and list >>> specific properties from a dtb blob using the 'fdtget' tool which is >>> part of the dtc tree. >> >> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted >> FDT in a file with an extra option? That was possible because of the >> format helpers introduced for 'info fdt'. The idea is that since we're >> able to format a FDT in DTS format, we can also write the FDT in text >> format without relying on DTC to decode it. > > Since it's mostly the same code, I think it's reasonable to throw in > if the info fdt stuff is there, but I don't think it's worth including > without that. As a whole, I remain dubious that (info fdt + dumpdts) > is worth the complexity cost. > > People with more practical experience debugging the embedded ARM > platforms might have a different opinion if they thing info fdt would > be really useful though. Fair enough. Let's wait to see if they have an option on that. Otherwise I'll prune 'info fdt' from the next version and roll out just with dumpdtb. Thanks, Daniel > >> If we think that this 'dumpdtb' capability is worth having, I can respin >> the patches without 'info fdt' but adding these helpers to enable this >> 'dumpdtb' support. If not, then we can just remove patches 15-21 and >> be done with it. >> >> >> Thanks, >> >> >> Daniel >> >>> >>>> >>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>>> --- >>>> hmp-commands-info.hx | 13 ++++++++++ >>>> include/monitor/hmp.h | 1 + >>>> include/sysemu/device_tree.h | 4 +++ >>>> monitor/hmp-cmds.c | 13 ++++++++++ >>>> monitor/qmp-cmds.c | 12 +++++++++ >>>> qapi/machine.json | 19 +++++++++++++++ >>>> softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ >>>> 7 files changed, 109 insertions(+) >>>> >>>> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx >>>> index 188d9ece3b..743b48865d 100644 >>>> --- a/hmp-commands-info.hx >>>> +++ b/hmp-commands-info.hx >>>> @@ -921,3 +921,16 @@ SRST >>>> ``stats`` >>>> Show runtime-collected statistics >>>> ERST >>>> + >>>> + { >>>> + .name = "fdt", >>>> + .args_type = "nodepath:s", >>>> + .params = "nodepath", >>>> + .help = "show firmware device tree node given its path", >>>> + .cmd = hmp_info_fdt, >>>> + }, >>>> + >>>> +SRST >>>> + ``info fdt`` >>>> + Show a firmware device tree node given its path. Requires libfdt. >>>> +ERST >>>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h >>>> index d7f324da59..c0883dd1e3 100644 >>>> --- a/include/monitor/hmp.h >>>> +++ b/include/monitor/hmp.h >>>> @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >>>> void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >>>> void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); >>>> void hmp_dumpdtb(Monitor *mon, const QDict *qdict); >>>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict); >>>> void hmp_human_readable_text_helper(Monitor *mon, >>>> HumanReadableText *(*qmp_handler)(Error **)); >>>> void hmp_info_stats(Monitor *mon, const QDict *qdict); >>>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h >>>> index bf7684e4ed..057d13e397 100644 >>>> --- a/include/sysemu/device_tree.h >>>> +++ b/include/sysemu/device_tree.h >>>> @@ -14,6 +14,8 @@ >>>> #ifndef DEVICE_TREE_H >>>> #define DEVICE_TREE_H >>>> +#include "qapi/qapi-types-common.h" >>>> + >>>> void *create_device_tree(int *sizep); >>>> void *load_device_tree(const char *filename_path, int *sizep); >>>> #ifdef CONFIG_LINUX >>>> @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); >>>> void qemu_fdt_dumpdtb(void *fdt, int size); >>>> void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); >>>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, >>>> + Error **errp); >>>> /** >>>> * qemu_fdt_setprop_sized_cells_from_array: >>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >>>> index 1c7bfd3b9d..93a4103afa 100644 >>>> --- a/monitor/hmp-cmds.c >>>> +++ b/monitor/hmp-cmds.c >>>> @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) >>>> hmp_handle_error(mon, local_err); >>>> } >>>> } >>>> + >>>> +void hmp_info_fdt(Monitor *mon, const QDict *qdict) >>>> +{ >>>> + const char *nodepath = qdict_get_str(qdict, "nodepath"); >>>> + Error *err = NULL; >>>> + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); >>>> + >>>> + if (hmp_handle_error(mon, err)) { >>>> + return; >>>> + } >>>> + >>>> + monitor_printf(mon, "%s", info->human_readable_text); >>>> +} >>>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c >>>> index 8415aca08c..db2c6aa7da 100644 >>>> --- a/monitor/qmp-cmds.c >>>> +++ b/monitor/qmp-cmds.c >>>> @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) >>>> { >>>> return qemu_fdt_qmp_dumpdtb(filename, errp); >>>> } >>>> + >>>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) >>>> +{ >>>> + return qemu_fdt_qmp_query_fdt(nodepath, errp); >>>> +} >>>> #else >>>> void qmp_dumpdtb(const char *filename, Error **errp) >>>> { >>>> error_setg(errp, "dumpdtb requires libfdt"); >>>> } >>>> + >>>> +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) >>>> +{ >>>> + error_setg(errp, "this command requires libfdt"); >>>> + >>>> + return NULL; >>>> +} >>>> #endif >>>> diff --git a/qapi/machine.json b/qapi/machine.json >>>> index aeb013f3dd..96cff541ca 100644 >>>> --- a/qapi/machine.json >>>> +++ b/qapi/machine.json >>>> @@ -1681,3 +1681,22 @@ >>>> ## >>>> { 'command': 'dumpdtb', >>>> 'data': { 'filename': 'str' } } >>>> + >>>> +## >>>> +# @x-query-fdt: >>>> +# >>>> +# Query for FDT element (node or property). Requires 'libfdt'. >>>> +# >>>> +# @nodepath: the path of the FDT node to be retrieved >>>> +# >>>> +# Features: >>>> +# @unstable: This command is meant for debugging. >>>> +# >>>> +# Returns: FDT node >>>> +# >>>> +# Since: 7.2 >>>> +## >>>> +{ 'command': 'x-query-fdt', >>>> + 'data': { 'nodepath': 'str' }, >>>> + 'returns': 'HumanReadableText', >>>> + 'features': [ 'unstable' ] } >>>> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c >>>> index cd487ddd4d..6b15f6ace2 100644 >>>> --- a/softmmu/device_tree.c >>>> +++ b/softmmu/device_tree.c >>>> @@ -18,6 +18,7 @@ >>>> #endif >>>> #include "qapi/error.h" >>>> +#include "qapi/type-helpers.h" >>>> #include "qemu/error-report.h" >>>> #include "qemu/option.h" >>>> #include "qemu/bswap.h" >>>> @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp) >>>> error_setg(errp, "Error when saving machine FDT to file %s", filename); >>>> } >>>> + >>>> +static void fdt_format_node(GString *buf, int node, int depth) >>>> +{ >>>> + const struct fdt_property *prop = NULL; >>>> + const char *propname = NULL; >>>> + void *fdt = current_machine->fdt; >>>> + int padding = depth * 4; >>>> + int property = 0; >>>> + int prop_size; >>>> + >>>> + g_string_append_printf(buf, "%*s%s {\n", padding, "", >>>> + fdt_get_name(fdt, node, NULL)); >>>> + >>>> + padding += 4; >>>> + >>>> + fdt_for_each_property_offset(property, fdt, node) { >>>> + prop = fdt_get_property_by_offset(fdt, property, &prop_size); >>>> + propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); >>>> + >>>> + g_string_append_printf(buf, "%*s%s;\n", padding, "", propname); >>>> + } >>>> + >>>> + padding -= 4; >>>> + g_string_append_printf(buf, "%*s};\n", padding, ""); >>>> +} >>>> + >>>> +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp) >>>> +{ >>>> + g_autoptr(GString) buf = g_string_new(""); >>>> + int node; >>>> + >>>> + if (!current_machine->fdt) { >>>> + error_setg(errp, "Unable to find the machine FDT"); >>>> + return NULL; >>>> + } >>>> + >>>> + node = fdt_path_offset(current_machine->fdt, nodepath); >>>> + if (node < 0) { >>>> + error_setg(errp, "node '%s' not found in FDT", nodepath); >>>> + return NULL; >>>> + } >>>> + >>>> + fdt_format_node(buf, node, 0); >>>> + >>>> + return human_readable_text_from_str(buf); >>>> +} >>> >> >
Daniel Henrique Barboza <danielhb413@gmail.com> writes: > Reading the FDT requires that the user saves the fdt_blob and then use > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong > use case when we need to compare two FDTs, but it's a lot of steps if > you want to do quick check on a certain node or property. > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it > to the user. This can be used to check the FDT on running machines > without having to save the blob and use 'dtc'. > > The implementation is based on the premise that the machine thas a FDT > created using libfdt and pointed by 'machine->fdt'. As long as this > pre-requisite is met the machine should be able to support it. > > For now we're going to add the required QMP/HMP boilerplate and the > capability of printing the name of the properties of a given node. Next > patches will extend 'info fdt' to be able to print nodes recursively, > and then individual properties. > > This command will always be executed in-band (i.e. holding BQL), > avoiding potential race conditions with machines that might change the > FDT during runtime (e.g. PowerPC 'pseries' machine). > > 'info fdt' is not something that we expect to be used aside from debugging, > so we're implementing it in QMP as 'x-query-fdt'. > > This is an example of 'info fdt' fetching the '/chosen' node of the > pSeries machine: > > (qemu) info fdt /chosen > chosen { > ibm,architecture-vec-5; > rng-seed; > ibm,arch-vec-5-platform-support; > linux,pci-probe-only; > stdout-path; > linux,stdout-path; > qemu,graphic-depth; > qemu,graphic-height; > qemu,graphic-width; > }; > > And the same node for the aarch64 'virt' machine: > > (qemu) info fdt /chosen > chosen { > stdout-path; > rng-seed; > kaslr-seed; > }; > > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> > Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hmp-commands-info.hx | 13 ++++++++++ > include/monitor/hmp.h | 1 + > include/sysemu/device_tree.h | 4 +++ > monitor/hmp-cmds.c | 13 ++++++++++ > monitor/qmp-cmds.c | 12 +++++++++ > qapi/machine.json | 19 +++++++++++++++ > softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ > 7 files changed, 109 insertions(+) > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > index 188d9ece3b..743b48865d 100644 > --- a/hmp-commands-info.hx > +++ b/hmp-commands-info.hx > @@ -921,3 +921,16 @@ SRST > ``stats`` > Show runtime-collected statistics > ERST > + > + { > + .name = "fdt", > + .args_type = "nodepath:s", > + .params = "nodepath", > + .help = "show firmware device tree node given its path", > + .cmd = hmp_info_fdt, > + }, > + > +SRST > + ``info fdt`` > + Show a firmware device tree node given its path. Requires libfdt. > +ERST > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h > index d7f324da59..c0883dd1e3 100644 > --- a/include/monitor/hmp.h > +++ b/include/monitor/hmp.h > @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); > void hmp_dumpdtb(Monitor *mon, const QDict *qdict); > +void hmp_info_fdt(Monitor *mon, const QDict *qdict); > void hmp_human_readable_text_helper(Monitor *mon, > HumanReadableText *(*qmp_handler)(Error **)); > void hmp_info_stats(Monitor *mon, const QDict *qdict); > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index bf7684e4ed..057d13e397 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -14,6 +14,8 @@ > #ifndef DEVICE_TREE_H > #define DEVICE_TREE_H > > +#include "qapi/qapi-types-common.h" > + > void *create_device_tree(int *sizep); > void *load_device_tree(const char *filename_path, int *sizep); > #ifdef CONFIG_LINUX > @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); > > void qemu_fdt_dumpdtb(void *fdt, int size); > void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); > +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, > + Error **errp); > > /** > * qemu_fdt_setprop_sized_cells_from_array: > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 1c7bfd3b9d..93a4103afa 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, local_err); > } > } > + > +void hmp_info_fdt(Monitor *mon, const QDict *qdict) > +{ > + const char *nodepath = qdict_get_str(qdict, "nodepath"); > + Error *err = NULL; > + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); > + > + if (hmp_handle_error(mon, err)) { > + return; > + } > + > + monitor_printf(mon, "%s", info->human_readable_text); > +} > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 8415aca08c..db2c6aa7da 100644 > --- a/monitor/qmp-cmds.c > +++ b/monitor/qmp-cmds.c > @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) > { > return qemu_fdt_qmp_dumpdtb(filename, errp); > } > + > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > +{ > + return qemu_fdt_qmp_query_fdt(nodepath, errp); > +} > #else > void qmp_dumpdtb(const char *filename, Error **errp) > { > error_setg(errp, "dumpdtb requires libfdt"); > } > + > +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) > +{ > + error_setg(errp, "this command requires libfdt"); > + > + return NULL; > +} > #endif > diff --git a/qapi/machine.json b/qapi/machine.json > index aeb013f3dd..96cff541ca 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1681,3 +1681,22 @@ > ## > { 'command': 'dumpdtb', > 'data': { 'filename': 'str' } } > + > +## > +# @x-query-fdt: > +# > +# Query for FDT element (node or property). Requires 'libfdt'. > +# > +# @nodepath: the path of the FDT node to be retrieved In a QAPI schema, we separate words by dashes unless there's a compelling reason not to. Thus: @node-path. > +# > +# Features: > +# @unstable: This command is meant for debugging. > +# > +# Returns: FDT node > +# > +# Since: 7.2 > +## > +{ 'command': 'x-query-fdt', > + 'data': { 'nodepath': 'str' }, > + 'returns': 'HumanReadableText', > + 'features': [ 'unstable' ] } Same question as for the previous patch: why isn't this 'if': 'CONFIG_FDT'? [...]
David Gibson <david@gibson.dropbear.id.au> writes: > On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote: >> >> >> On 8/29/22 00:34, David Gibson wrote: >> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: >> > > Reading the FDT requires that the user saves the fdt_blob and then use >> > > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong >> > > use case when we need to compare two FDTs, but it's a lot of steps if >> > > you want to do quick check on a certain node or property. >> > > >> > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it >> > > to the user. This can be used to check the FDT on running machines >> > > without having to save the blob and use 'dtc'. >> > > >> > > The implementation is based on the premise that the machine thas a FDT >> > > created using libfdt and pointed by 'machine->fdt'. As long as this >> > > pre-requisite is met the machine should be able to support it. >> > > >> > > For now we're going to add the required QMP/HMP boilerplate and the >> > > capability of printing the name of the properties of a given node. Next >> > > patches will extend 'info fdt' to be able to print nodes recursively, >> > > and then individual properties. >> > > >> > > This command will always be executed in-band (i.e. holding BQL), >> > > avoiding potential race conditions with machines that might change the >> > > FDT during runtime (e.g. PowerPC 'pseries' machine). >> > > >> > > 'info fdt' is not something that we expect to be used aside from debugging, >> > > so we're implementing it in QMP as 'x-query-fdt'. >> > > >> > > This is an example of 'info fdt' fetching the '/chosen' node of the >> > > pSeries machine: >> > > >> > > (qemu) info fdt /chosen >> > > chosen { >> > > ibm,architecture-vec-5; >> > > rng-seed; >> > > ibm,arch-vec-5-platform-support; >> > > linux,pci-probe-only; >> > > stdout-path; >> > > linux,stdout-path; >> > > qemu,graphic-depth; >> > > qemu,graphic-height; >> > > qemu,graphic-width; >> > > }; >> > > >> > > And the same node for the aarch64 'virt' machine: >> > > >> > > (qemu) info fdt /chosen >> > > chosen { >> > > stdout-path; >> > > rng-seed; >> > > kaslr-seed; >> > > }; >> > >> > So, I'm reasonably convinced allowing dumping the whole dtb from >> > qmp/hmp is useful. I'm less convined that info fdt is worth the >> > additional complexity it incurs. Note that as well as being able to >> > decompile a whole dtb using dtc, you can also extract and list >> > specific properties from a dtb blob using the 'fdtget' tool which is >> > part of the dtc tree. >> >> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted >> FDT in a file with an extra option? That was possible because of the >> format helpers introduced for 'info fdt'. The idea is that since we're >> able to format a FDT in DTS format, we can also write the FDT in text >> format without relying on DTC to decode it. > > Since it's mostly the same code, I think it's reasonable to throw in > if the info fdt stuff is there, but I don't think it's worth including > without that. As a whole, I remain dubious that (info fdt + dumpdts) > is worth the complexity cost. How much code does it take, and who's going to maintain it? > People with more practical experience debugging the embedded ARM > platforms might have a different opinion if they thing info fdt would > be really useful though. They better speak up then :) >> If we think that this 'dumpdtb' capability is worth having, I can respin >> the patches without 'info fdt' but adding these helpers to enable this >> 'dumpdtb' support. If not, then we can just remove patches 15-21 and >> be done with it. >> >> >> Thanks, >> >> >> Daniel
On Tue, Aug 30, 2022 at 12:43:23PM +0200, Markus Armbruster wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > On Mon, Aug 29, 2022 at 07:00:55PM -0300, Daniel Henrique Barboza wrote: > >> > >> > >> On 8/29/22 00:34, David Gibson wrote: > >> > On Fri, Aug 26, 2022 at 11:11:44AM -0300, Daniel Henrique Barboza wrote: > >> > > Reading the FDT requires that the user saves the fdt_blob and then use > >> > > 'dtc' to read the contents. Saving the file and using 'dtc' is a strong > >> > > use case when we need to compare two FDTs, but it's a lot of steps if > >> > > you want to do quick check on a certain node or property. > >> > > > >> > > 'info fdt' retrieves FDT nodes (and properties, later on) and print it > >> > > to the user. This can be used to check the FDT on running machines > >> > > without having to save the blob and use 'dtc'. > >> > > > >> > > The implementation is based on the premise that the machine thas a FDT > >> > > created using libfdt and pointed by 'machine->fdt'. As long as this > >> > > pre-requisite is met the machine should be able to support it. > >> > > > >> > > For now we're going to add the required QMP/HMP boilerplate and the > >> > > capability of printing the name of the properties of a given node. Next > >> > > patches will extend 'info fdt' to be able to print nodes recursively, > >> > > and then individual properties. > >> > > > >> > > This command will always be executed in-band (i.e. holding BQL), > >> > > avoiding potential race conditions with machines that might change the > >> > > FDT during runtime (e.g. PowerPC 'pseries' machine). > >> > > > >> > > 'info fdt' is not something that we expect to be used aside from debugging, > >> > > so we're implementing it in QMP as 'x-query-fdt'. > >> > > > >> > > This is an example of 'info fdt' fetching the '/chosen' node of the > >> > > pSeries machine: > >> > > > >> > > (qemu) info fdt /chosen > >> > > chosen { > >> > > ibm,architecture-vec-5; > >> > > rng-seed; > >> > > ibm,arch-vec-5-platform-support; > >> > > linux,pci-probe-only; > >> > > stdout-path; > >> > > linux,stdout-path; > >> > > qemu,graphic-depth; > >> > > qemu,graphic-height; > >> > > qemu,graphic-width; > >> > > }; > >> > > > >> > > And the same node for the aarch64 'virt' machine: > >> > > > >> > > (qemu) info fdt /chosen > >> > > chosen { > >> > > stdout-path; > >> > > rng-seed; > >> > > kaslr-seed; > >> > > }; > >> > > >> > So, I'm reasonably convinced allowing dumping the whole dtb from > >> > qmp/hmp is useful. I'm less convined that info fdt is worth the > >> > additional complexity it incurs. Note that as well as being able to > >> > decompile a whole dtb using dtc, you can also extract and list > >> > specific properties from a dtb blob using the 'fdtget' tool which is > >> > part of the dtc tree. > >> > >> What's your opinion on patch 21/21, where 'dumpdtb' can write a formatted > >> FDT in a file with an extra option? That was possible because of the > >> format helpers introduced for 'info fdt'. The idea is that since we're > >> able to format a FDT in DTS format, we can also write the FDT in text > >> format without relying on DTC to decode it. > > > > Since it's mostly the same code, I think it's reasonable to throw in > > if the info fdt stuff is there, but I don't think it's worth including > > without that. As a whole, I remain dubious that (info fdt + dumpdts) > > is worth the complexity cost. > > How much code does it take, and who's going to maintain it? It's not especially big, but it's not negligible. Perhaps the part that I'm most uncomfortable about is that it requires a bunch of messy heuristics to guess how to format the output - DT properties are just bytestrings, any internal interpretation is based on the specific bindings for them. dtc already has these and I don't love having a second, potentially different copy of necessarily imperfect heuristics out in the wild. > > People with more practical experience debugging the embedded ARM > > platforms might have a different opinion if they thing info fdt would > > be really useful though. > > They better speak up then :) Just so.
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index 188d9ece3b..743b48865d 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -921,3 +921,16 @@ SRST ``stats`` Show runtime-collected statistics ERST + + { + .name = "fdt", + .args_type = "nodepath:s", + .params = "nodepath", + .help = "show firmware device tree node given its path", + .cmd = hmp_info_fdt, + }, + +SRST + ``info fdt`` + Show a firmware device tree node given its path. Requires libfdt. +ERST diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index d7f324da59..c0883dd1e3 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -135,6 +135,7 @@ void hmp_set_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); void hmp_cancel_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); void hmp_dumpdtb(Monitor *mon, const QDict *qdict); +void hmp_info_fdt(Monitor *mon, const QDict *qdict); void hmp_human_readable_text_helper(Monitor *mon, HumanReadableText *(*qmp_handler)(Error **)); void hmp_info_stats(Monitor *mon, const QDict *qdict); diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h index bf7684e4ed..057d13e397 100644 --- a/include/sysemu/device_tree.h +++ b/include/sysemu/device_tree.h @@ -14,6 +14,8 @@ #ifndef DEVICE_TREE_H #define DEVICE_TREE_H +#include "qapi/qapi-types-common.h" + void *create_device_tree(int *sizep); void *load_device_tree(const char *filename_path, int *sizep); #ifdef CONFIG_LINUX @@ -137,6 +139,8 @@ int qemu_fdt_add_path(void *fdt, const char *path); void qemu_fdt_dumpdtb(void *fdt, int size); void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp); +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, + Error **errp); /** * qemu_fdt_setprop_sized_cells_from_array: diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 1c7bfd3b9d..93a4103afa 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -2484,3 +2484,16 @@ void hmp_dumpdtb(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, local_err); } } + +void hmp_info_fdt(Monitor *mon, const QDict *qdict) +{ + const char *nodepath = qdict_get_str(qdict, "nodepath"); + Error *err = NULL; + g_autoptr(HumanReadableText) info = qmp_x_query_fdt(nodepath, &err); + + if (hmp_handle_error(mon, err)) { + return; + } + + monitor_printf(mon, "%s", info->human_readable_text); +} diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 8415aca08c..db2c6aa7da 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -603,9 +603,21 @@ void qmp_dumpdtb(const char *filename, Error **errp) { return qemu_fdt_qmp_dumpdtb(filename, errp); } + +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) +{ + return qemu_fdt_qmp_query_fdt(nodepath, errp); +} #else void qmp_dumpdtb(const char *filename, Error **errp) { error_setg(errp, "dumpdtb requires libfdt"); } + +HumanReadableText *qmp_x_query_fdt(const char *nodepath, Error **errp) +{ + error_setg(errp, "this command requires libfdt"); + + return NULL; +} #endif diff --git a/qapi/machine.json b/qapi/machine.json index aeb013f3dd..96cff541ca 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1681,3 +1681,22 @@ ## { 'command': 'dumpdtb', 'data': { 'filename': 'str' } } + +## +# @x-query-fdt: +# +# Query for FDT element (node or property). Requires 'libfdt'. +# +# @nodepath: the path of the FDT node to be retrieved +# +# Features: +# @unstable: This command is meant for debugging. +# +# Returns: FDT node +# +# Since: 7.2 +## +{ 'command': 'x-query-fdt', + 'data': { 'nodepath': 'str' }, + 'returns': 'HumanReadableText', + 'features': [ 'unstable' ] } diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c index cd487ddd4d..6b15f6ace2 100644 --- a/softmmu/device_tree.c +++ b/softmmu/device_tree.c @@ -18,6 +18,7 @@ #endif #include "qapi/error.h" +#include "qapi/type-helpers.h" #include "qemu/error-report.h" #include "qemu/option.h" #include "qemu/bswap.h" @@ -661,3 +662,49 @@ void qemu_fdt_qmp_dumpdtb(const char *filename, Error **errp) error_setg(errp, "Error when saving machine FDT to file %s", filename); } + +static void fdt_format_node(GString *buf, int node, int depth) +{ + const struct fdt_property *prop = NULL; + const char *propname = NULL; + void *fdt = current_machine->fdt; + int padding = depth * 4; + int property = 0; + int prop_size; + + g_string_append_printf(buf, "%*s%s {\n", padding, "", + fdt_get_name(fdt, node, NULL)); + + padding += 4; + + fdt_for_each_property_offset(property, fdt, node) { + prop = fdt_get_property_by_offset(fdt, property, &prop_size); + propname = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); + + g_string_append_printf(buf, "%*s%s;\n", padding, "", propname); + } + + padding -= 4; + g_string_append_printf(buf, "%*s};\n", padding, ""); +} + +HumanReadableText *qemu_fdt_qmp_query_fdt(const char *nodepath, Error **errp) +{ + g_autoptr(GString) buf = g_string_new(""); + int node; + + if (!current_machine->fdt) { + error_setg(errp, "Unable to find the machine FDT"); + return NULL; + } + + node = fdt_path_offset(current_machine->fdt, nodepath); + if (node < 0) { + error_setg(errp, "node '%s' not found in FDT", nodepath); + return NULL; + } + + fdt_format_node(buf, node, 0); + + return human_readable_text_from_str(buf); +}
Reading the FDT requires that the user saves the fdt_blob and then use 'dtc' to read the contents. Saving the file and using 'dtc' is a strong use case when we need to compare two FDTs, but it's a lot of steps if you want to do quick check on a certain node or property. 'info fdt' retrieves FDT nodes (and properties, later on) and print it to the user. This can be used to check the FDT on running machines without having to save the blob and use 'dtc'. The implementation is based on the premise that the machine thas a FDT created using libfdt and pointed by 'machine->fdt'. As long as this pre-requisite is met the machine should be able to support it. For now we're going to add the required QMP/HMP boilerplate and the capability of printing the name of the properties of a given node. Next patches will extend 'info fdt' to be able to print nodes recursively, and then individual properties. This command will always be executed in-band (i.e. holding BQL), avoiding potential race conditions with machines that might change the FDT during runtime (e.g. PowerPC 'pseries' machine). 'info fdt' is not something that we expect to be used aside from debugging, so we're implementing it in QMP as 'x-query-fdt'. This is an example of 'info fdt' fetching the '/chosen' node of the pSeries machine: (qemu) info fdt /chosen chosen { ibm,architecture-vec-5; rng-seed; ibm,arch-vec-5-platform-support; linux,pci-probe-only; stdout-path; linux,stdout-path; qemu,graphic-depth; qemu,graphic-height; qemu,graphic-width; }; And the same node for the aarch64 'virt' machine: (qemu) info fdt /chosen chosen { stdout-path; rng-seed; kaslr-seed; }; Cc: Dr. David Alan Gilbert <dgilbert@redhat.com> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hmp-commands-info.hx | 13 ++++++++++ include/monitor/hmp.h | 1 + include/sysemu/device_tree.h | 4 +++ monitor/hmp-cmds.c | 13 ++++++++++ monitor/qmp-cmds.c | 12 +++++++++ qapi/machine.json | 19 +++++++++++++++ softmmu/device_tree.c | 47 ++++++++++++++++++++++++++++++++++++ 7 files changed, 109 insertions(+)