| Message ID | 20250408160802.49870-13-agarciav@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hyperlaunch device tree for dom0 | expand |
On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > > > From: "Daniel P. Smith" dpsmith@apertussolutions.com > > > Introduce the ability to specify the desired domain id for the domain > definition. The domain id will be populated in the domid property of the > domain > node in the device tree configuration. > > Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com > > --- > v3: > * Remove ramdisk parsing > * Add missing xen/errno.h include > --- > xen/arch/x86/domain-builder/fdt.c | 39 ++++++++++++++++++++++++++++- > xen/arch/x86/setup.c | 5 ++-- > xen/include/xen/libfdt/libfdt-xen.h | 11 ++++++++ > 3 files changed, 52 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c > index 0f5fd01557..4c6aafe195 100644 > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -8,6 +8,7 @@ > #include <xen/libfdt/libfdt.h> > > > #include <asm/bootinfo.h> > > +#include <asm/guest.h> > > #include <asm/page.h> > > #include <asm/setup.h> > > > @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, > static int __init process_domain_node( > struct boot_info *bi, const void *fdt, int dom_node) > { > - int node; > + int node, property; > struct boot_domain *bd = &bi->domains[bi->nr_domains]; > > const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; > int address_cells = fdt_address_cells(fdt, dom_node); > int size_cells = fdt_size_cells(fdt, dom_node); > > + fdt_for_each_property_offset(property, fdt, dom_node) > + { > + const struct fdt_property *prop; > + const char prop_name; > + int name_len; > + > + prop = fdt_get_property_by_offset(fdt, property, NULL); > + if ( !prop ) > + continue; / silently skip */ > + > + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); > > + > + if ( strncmp(prop_name, "domid", name_len) == 0 ) > + { > + uint32_t val = DOMID_INVALID; > + if ( fdt_prop_as_u32(prop, &val) != 0 ) > + { > + printk(" failed processing domain id for domain %s\n", name); Add XENLOG_ERR ? > + return -EINVAL; > + } > + if ( val >= DOMID_FIRST_RESERVED ) > > + { > + printk(" invalid domain id for domain %s\n", name); Add XENLOG_ERR ? > + return -EINVAL; > + } > + bd->domid = (domid_t)val; > > + printk(" domid: %d\n", bd->domid); > > + } > + } > + > fdt_for_each_subnode(node, fdt, dom_node) > { > if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) > @@ -233,6 +264,12 @@ static int __init process_domain_node( > return -ENODATA; > } > > + if ( bd->domid == DOMID_INVALID ) > > + bd->domid = get_initial_domain_id(); > > + else if ( bd->domid != get_initial_domain_id() ) > > + printk(XENLOG_WARNING > + "WARN: Booting without initial domid not supported.\n"); Drop WARN since the log message is XENLOG_WARNING level already? > + > return 0; > } > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 3dfa81b48c..db7280225e 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1033,8 +1033,9 @@ static struct domain *__init create_dom0(struct boot_info bi) > if ( iommu_enabled ) > dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > > - / Create initial domain. Not d0 for pvshim. */ > - bd->domid = get_initial_domain_id(); > > + if ( bd->domid == DOMID_INVALID ) > > + /* Create initial domain. Not d0 for pvshim. */ > + bd->domid = get_initial_domain_id(); > > d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged); > > if ( IS_ERR(d) ) > panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d)); > > diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h > index e473fbaf0c..3031bec90e 100644 > --- a/xen/include/xen/libfdt/libfdt-xen.h > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -12,6 +12,7 @@ > #define LIBFDT_XEN_H > > #include <xen/libfdt/libfdt.h> > > +#include <xen/errno.h> > > > static inline int __init fdt_cell_as_u32(const fdt32_t *cell) > { > @@ -23,6 +24,16 @@ static inline uint64_t __init fdt_cell_as_u64(const fdt32_t *cell) > return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]); > } > > +static inline int __init fdt_prop_as_u32( > + const struct fdt_property *prop, uint32_t *val) > +{ > + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) ) > > + return -EINVAL; > + > + *val = fdt_cell_as_u32((fdt32_t *)prop->data); > > + return 0; > +} > + > static inline bool __init fdt_get_prop_offset( > const void *fdt, int node, const char *name, unsigned long *offset) > { > -- > 2.43.0
On 08.04.2025 18:07, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > Introduce the ability to specify the desired domain id for the domain > definition. The domain id will be populated in the domid property of the > domain > node in the device tree configuration. Nit: Odd splitting of lines. > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -8,6 +8,7 @@ > #include <xen/libfdt/libfdt.h> > > #include <asm/bootinfo.h> > +#include <asm/guest.h> What is this needed for? > @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, > static int __init process_domain_node( > struct boot_info *bi, const void *fdt, int dom_node) > { > - int node; > + int node, property; > struct boot_domain *bd = &bi->domains[bi->nr_domains]; > const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; > int address_cells = fdt_address_cells(fdt, dom_node); > int size_cells = fdt_size_cells(fdt, dom_node); > > + fdt_for_each_property_offset(property, fdt, dom_node) > + { > + const struct fdt_property *prop; > + const char *prop_name; > + int name_len; > + > + prop = fdt_get_property_by_offset(fdt, property, NULL); > + if ( !prop ) > + continue; /* silently skip */ > + > + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); > + > + if ( strncmp(prop_name, "domid", name_len) == 0 ) > + { > + uint32_t val = DOMID_INVALID; > + if ( fdt_prop_as_u32(prop, &val) != 0 ) Nit: Blank line please between declaration(s) and statement(s). > + { > + printk(" failed processing domain id for domain %s\n", name); > + return -EINVAL; > + } > + if ( val >= DOMID_FIRST_RESERVED ) > + { > + printk(" invalid domain id for domain %s\n", name); > + return -EINVAL; > + } > + bd->domid = (domid_t)val; And a conflict with other domains' IDs will not be complained about? > + printk(" domid: %d\n", bd->domid); If the error messages log "name" for (I suppose) disambiguation, why would the success message here not also do so? > @@ -233,6 +264,12 @@ static int __init process_domain_node( > return -ENODATA; > } > > + if ( bd->domid == DOMID_INVALID ) > + bd->domid = get_initial_domain_id(); > + else if ( bd->domid != get_initial_domain_id() ) > + printk(XENLOG_WARNING > + "WARN: Booting without initial domid not supported.\n"); I'm not a native speaker, but (or perhaps because of that) "without" feels wrong here. Also nit: No full stops please at the end of log messages, at least in the common case. Is the resolving of DOMID_INVALID invalid really needed both here and ... > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1033,8 +1033,9 @@ static struct domain *__init create_dom0(struct boot_info *bi) > if ( iommu_enabled ) > dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; > > - /* Create initial domain. Not d0 for pvshim. */ > - bd->domid = get_initial_domain_id(); > + if ( bd->domid == DOMID_INVALID ) > + /* Create initial domain. Not d0 for pvshim. */ > + bd->domid = get_initial_domain_id(); ... here? > @@ -23,6 +24,16 @@ static inline uint64_t __init fdt_cell_as_u64(const fdt32_t *cell) > return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]); > } > > +static inline int __init fdt_prop_as_u32( > + const struct fdt_property *prop, uint32_t *val) > +{ > + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) ) > + return -EINVAL; > + > + *val = fdt_cell_as_u32((fdt32_t *)prop->data); > + return 0; > +} Path 08 looks to (partly) open-code this. Perhaps better to introduce already there? Jan
On Wed Apr 9, 2025 at 11:15 PM BST, Denis Mukhin wrote: > On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: > >> >> >> From: "Daniel P. Smith" dpsmith@apertussolutions.com >> >> >> Introduce the ability to specify the desired domain id for the domain >> definition. The domain id will be populated in the domid property of the >> domain >> node in the device tree configuration. >> >> Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com >> >> --- >> v3: >> * Remove ramdisk parsing >> * Add missing xen/errno.h include >> --- >> xen/arch/x86/domain-builder/fdt.c | 39 ++++++++++++++++++++++++++++- >> xen/arch/x86/setup.c | 5 ++-- >> xen/include/xen/libfdt/libfdt-xen.h | 11 ++++++++ >> 3 files changed, 52 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c >> index 0f5fd01557..4c6aafe195 100644 >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -8,6 +8,7 @@ >> #include <xen/libfdt/libfdt.h> >> >> >> #include <asm/bootinfo.h> >> >> +#include <asm/guest.h> >> >> #include <asm/page.h> >> >> #include <asm/setup.h> >> >> >> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >> static int __init process_domain_node( >> struct boot_info *bi, const void *fdt, int dom_node) >> { >> - int node; >> + int node, property; >> struct boot_domain *bd = &bi->domains[bi->nr_domains]; >> >> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >> int address_cells = fdt_address_cells(fdt, dom_node); >> int size_cells = fdt_size_cells(fdt, dom_node); >> >> + fdt_for_each_property_offset(property, fdt, dom_node) >> + { >> + const struct fdt_property *prop; >> + const char prop_name; >> + int name_len; >> + >> + prop = fdt_get_property_by_offset(fdt, property, NULL); >> + if ( !prop ) >> + continue; / silently skip */ >> + >> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); >> >> + >> + if ( strncmp(prop_name, "domid", name_len) == 0 ) >> + { >> + uint32_t val = DOMID_INVALID; >> + if ( fdt_prop_as_u32(prop, &val) != 0 ) >> + { >> + printk(" failed processing domain id for domain %s\n", name); > > Add XENLOG_ERR ? Yes, and... > >> + return -EINVAL; >> + } >> + if ( val >= DOMID_FIRST_RESERVED ) >> >> + { >> + printk(" invalid domain id for domain %s\n", name); > > Add XENLOG_ERR ? ... yes. > >> + return -EINVAL; >> + } >> + bd->domid = (domid_t)val; >> >> + printk(" domid: %d\n", bd->domid); >> >> + } >> + } >> + >> fdt_for_each_subnode(node, fdt, dom_node) >> { >> if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) >> @@ -233,6 +264,12 @@ static int __init process_domain_node( >> return -ENODATA; >> } >> >> + if ( bd->domid == DOMID_INVALID ) >> >> + bd->domid = get_initial_domain_id(); >> >> + else if ( bd->domid != get_initial_domain_id() ) >> >> + printk(XENLOG_WARNING >> + "WARN: Booting without initial domid not supported.\n"); > > Drop WARN since the log message is XENLOG_WARNING level already? As mentioned elsewhere, the point of those prefixes are to be readable. Though I'm starting to get urges to rewrite many of this error handlers as asserts, on the basis that "why do we think it's ok to boot with malformed DTBs"? A safe system that doesn't boot is more helpful than an unsafe one that boots everything except a critical component for you to find later on. Cheers, Alejandro
On Thu Apr 10, 2025 at 12:49 PM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Introduce the ability to specify the desired domain id for the domain >> definition. The domain id will be populated in the domid property of the >> domain >> node in the device tree configuration. > > Nit: Odd splitting of lines. Fixed > >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -8,6 +8,7 @@ >> #include <xen/libfdt/libfdt.h> >> >> #include <asm/bootinfo.h> >> +#include <asm/guest.h> > > What is this needed for? get_initial_domain_id(), but that ought to come from xen/domain.h instead. Fixed. > >> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >> static int __init process_domain_node( >> struct boot_info *bi, const void *fdt, int dom_node) >> { >> - int node; >> + int node, property; >> struct boot_domain *bd = &bi->domains[bi->nr_domains]; >> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >> int address_cells = fdt_address_cells(fdt, dom_node); >> int size_cells = fdt_size_cells(fdt, dom_node); >> >> + fdt_for_each_property_offset(property, fdt, dom_node) >> + { >> + const struct fdt_property *prop; >> + const char *prop_name; >> + int name_len; >> + >> + prop = fdt_get_property_by_offset(fdt, property, NULL); >> + if ( !prop ) >> + continue; /* silently skip */ >> + >> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); >> + >> + if ( strncmp(prop_name, "domid", name_len) == 0 ) >> + { >> + uint32_t val = DOMID_INVALID; >> + if ( fdt_prop_as_u32(prop, &val) != 0 ) > > Nit: Blank line please between declaration(s) and statement(s). Ack. > >> + { >> + printk(" failed processing domain id for domain %s\n", name); >> + return -EINVAL; >> + } >> + if ( val >= DOMID_FIRST_RESERVED ) >> + { >> + printk(" invalid domain id for domain %s\n", name); >> + return -EINVAL; >> + } >> + bd->domid = (domid_t)val; > > And a conflict with other domains' IDs will not be complained about? Hmmm... sure, I can iterate the domlist and check. > >> + printk(" domid: %d\n", bd->domid); > > If the error messages log "name" for (I suppose) disambiguation, why would > the success message here not also do so? > >> @@ -233,6 +264,12 @@ static int __init process_domain_node( >> return -ENODATA; >> } >> >> + if ( bd->domid == DOMID_INVALID ) >> + bd->domid = get_initial_domain_id(); >> + else if ( bd->domid != get_initial_domain_id() ) >> + printk(XENLOG_WARNING >> + "WARN: Booting without initial domid not supported.\n"); > > I'm not a native speaker, but (or perhaps because of that) "without" feels > wrong here. It's probably the compound effect of without and "not supported". The statement is correct, but it's arguably a bit obtuse. I'll replace it with "WARN: Unsupported boot with missing initial domid.". As for the first branch of that clause... I'm not sure we want to support running with DTs that are missing the domid property. > > Also nit: No full stops please at the end of log messages, at least in the > common case. Ack > > Is the resolving of DOMID_INVALID invalid really needed both here and ... > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1033,8 +1033,9 @@ static struct domain *__init create_dom0(struct boot_info *bi) >> if ( iommu_enabled ) >> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; >> >> - /* Create initial domain. Not d0 for pvshim. */ >> - bd->domid = get_initial_domain_id(); >> + if ( bd->domid == DOMID_INVALID ) >> + /* Create initial domain. Not d0 for pvshim. */ >> + bd->domid = get_initial_domain_id(); > > ... here? I'll rationatise all that on v4. > >> @@ -23,6 +24,16 @@ static inline uint64_t __init fdt_cell_as_u64(const fdt32_t *cell) >> return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]); >> } >> >> +static inline int __init fdt_prop_as_u32( >> + const struct fdt_property *prop, uint32_t *val) >> +{ >> + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) ) >> + return -EINVAL; >> + >> + *val = fdt_cell_as_u32((fdt32_t *)prop->data); >> + return 0; >> +} > > Path 08 looks to (partly) open-code this. Perhaps better to introduce already > there? Already done. Cheers, Alejandro
On Mon, 14 Apr 2025, Alejandro Vallejo wrote: > Though I'm starting to get urges to rewrite many of this error handlers > as asserts, on the basis that "why do we think it's ok to boot with > malformed DTBs"? A safe system that doesn't boot is more helpful than an > unsafe one that boots everything except a critical component for you to > find later on. It is totally OK to panic on boot if a malformed DTB was passed. See the number of panics in xen/arch/arm/dom0less-build.c.
On 14.04.2025 20:07, Alejandro Vallejo wrote: > On Wed Apr 9, 2025 at 11:15 PM BST, Denis Mukhin wrote: >> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: >>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>> return -ENODATA; >>> } >>> >>> + if ( bd->domid == DOMID_INVALID ) >>> >>> + bd->domid = get_initial_domain_id(); >>> >>> + else if ( bd->domid != get_initial_domain_id() ) >>> >>> + printk(XENLOG_WARNING >>> + "WARN: Booting without initial domid not supported.\n"); >> >> Drop WARN since the log message is XENLOG_WARNING level already? > > As mentioned elsewhere, the point of those prefixes are to be readable. This, however, imo is a matter of consistency across the codebase, not just within hyperlaunch. Plus (again imo) if anything, prefixes that are part of the log output should contain proper words ("Warning:" or "Error:"), and they shouldn't needlessly "shout" (i.e. "FATAL:" is okay-ish to be all caps, but the others aren't). Jan
On 14.04.2025 20:35, Alejandro Vallejo wrote: > On Thu Apr 10, 2025 at 12:49 PM BST, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >>> static int __init process_domain_node( >>> struct boot_info *bi, const void *fdt, int dom_node) >>> { >>> - int node; >>> + int node, property; >>> struct boot_domain *bd = &bi->domains[bi->nr_domains]; >>> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >>> int address_cells = fdt_address_cells(fdt, dom_node); >>> int size_cells = fdt_size_cells(fdt, dom_node); >>> >>> + fdt_for_each_property_offset(property, fdt, dom_node) >>> + { >>> + const struct fdt_property *prop; >>> + const char *prop_name; >>> + int name_len; >>> + >>> + prop = fdt_get_property_by_offset(fdt, property, NULL); >>> + if ( !prop ) >>> + continue; /* silently skip */ >>> + >>> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); >>> + >>> + if ( strncmp(prop_name, "domid", name_len) == 0 ) >>> + { >>> + uint32_t val = DOMID_INVALID; >>> + if ( fdt_prop_as_u32(prop, &val) != 0 ) >>> + { >>> + printk(" failed processing domain id for domain %s\n", name); >>> + return -EINVAL; >>> + } >>> + if ( val >= DOMID_FIRST_RESERVED ) >>> + { >>> + printk(" invalid domain id for domain %s\n", name); >>> + return -EINVAL; >>> + } >>> + bd->domid = (domid_t)val; >> >> And a conflict with other domains' IDs will not be complained about? > > Hmmm... sure, I can iterate the domlist and check. Well, just to clarify: The checking doesn't necessarily need to happen here and now. It may also happen as domains are actually created. Yet then I think a pointer there (in a code comment) would be helpful here. >>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>> return -ENODATA; >>> } >>> >>> + if ( bd->domid == DOMID_INVALID ) >>> + bd->domid = get_initial_domain_id(); >>> + else if ( bd->domid != get_initial_domain_id() ) >>> + printk(XENLOG_WARNING >>> + "WARN: Booting without initial domid not supported.\n"); >> >> I'm not a native speaker, but (or perhaps because of that) "without" feels >> wrong here. > > It's probably the compound effect of without and "not supported". The > statement is correct, but it's arguably a bit obtuse. > > I'll replace it with "WARN: Unsupported boot with missing initial domid.". But that still doesn't fit the check, which compares the given ID (i.e. there's nothing "missing" here) with the expected one. The "no ID given" is handled by the plain if() that's first. > As for the first branch of that clause... I'm not sure we want to > support running with DTs that are missing the domid property. This I can't really judge on. Jan
On Tue Apr 15, 2025 at 7:21 AM BST, Jan Beulich wrote: > On 14.04.2025 20:07, Alejandro Vallejo wrote: >> On Wed Apr 9, 2025 at 11:15 PM BST, Denis Mukhin wrote: >>> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: >>>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>>> return -ENODATA; >>>> } >>>> >>>> + if ( bd->domid == DOMID_INVALID ) >>>> >>>> + bd->domid = get_initial_domain_id(); >>>> >>>> + else if ( bd->domid != get_initial_domain_id() ) >>>> >>>> + printk(XENLOG_WARNING >>>> + "WARN: Booting without initial domid not supported.\n"); >>> >>> Drop WARN since the log message is XENLOG_WARNING level already? >> >> As mentioned elsewhere, the point of those prefixes are to be readable. > > This, however, imo is a matter of consistency across the codebase, not just > within hyperlaunch. I agree. There is precedent though for certain printks to have a collective pattern for ease of reading (e.g: spec_ctrl.c when printing mitigations). With I'm merely justifying the 2 spaces followed by lowercase. I did try to remove them and it was strictly harder to know what they referred to. > Plus (again imo) if anything, prefixes that are part of > the log output should contain proper words ("Warning:" or "Error:"), and > they shouldn't needlessly "shout" (i.e. "FATAL:" is okay-ish to be all caps, > but the others aren't). > > Jan Right. I'm happy to rewrite them as "Warning: ..." and "Error: ..." Cheers, Alejandro
On Tue Apr 15, 2025 at 7:27 AM BST, Jan Beulich wrote: > On 14.04.2025 20:35, Alejandro Vallejo wrote: >> On Thu Apr 10, 2025 at 12:49 PM BST, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >>>> static int __init process_domain_node( >>>> struct boot_info *bi, const void *fdt, int dom_node) >>>> { >>>> - int node; >>>> + int node, property; >>>> struct boot_domain *bd = &bi->domains[bi->nr_domains]; >>>> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >>>> int address_cells = fdt_address_cells(fdt, dom_node); >>>> int size_cells = fdt_size_cells(fdt, dom_node); >>>> >>>> + fdt_for_each_property_offset(property, fdt, dom_node) >>>> + { >>>> + const struct fdt_property *prop; >>>> + const char *prop_name; >>>> + int name_len; >>>> + >>>> + prop = fdt_get_property_by_offset(fdt, property, NULL); >>>> + if ( !prop ) >>>> + continue; /* silently skip */ >>>> + >>>> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); >>>> + >>>> + if ( strncmp(prop_name, "domid", name_len) == 0 ) >>>> + { >>>> + uint32_t val = DOMID_INVALID; >>>> + if ( fdt_prop_as_u32(prop, &val) != 0 ) >>>> + { >>>> + printk(" failed processing domain id for domain %s\n", name); >>>> + return -EINVAL; >>>> + } >>>> + if ( val >= DOMID_FIRST_RESERVED ) >>>> + { >>>> + printk(" invalid domain id for domain %s\n", name); >>>> + return -EINVAL; >>>> + } >>>> + bd->domid = (domid_t)val; >>> >>> And a conflict with other domains' IDs will not be complained about? >> >> Hmmm... sure, I can iterate the domlist and check. > > Well, just to clarify: The checking doesn't necessarily need to happen here > and now. It may also happen as domains are actually created. Yet then I > think a pointer there (in a code comment) would be helpful here. That'd be fairly unsafe. In the case of parallel boot it'd be indeterminate which VMs end up running if they happen to have a domid clash. It's better to detect the error earlier and crash before any get to start up. > >>>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>>> return -ENODATA; >>>> } >>>> >>>> + if ( bd->domid == DOMID_INVALID ) >>>> + bd->domid = get_initial_domain_id(); >>>> + else if ( bd->domid != get_initial_domain_id() ) >>>> + printk(XENLOG_WARNING >>>> + "WARN: Booting without initial domid not supported.\n"); >>> >>> I'm not a native speaker, but (or perhaps because of that) "without" feels >>> wrong here. >> >> It's probably the compound effect of without and "not supported". The >> statement is correct, but it's arguably a bit obtuse. >> >> I'll replace it with "WARN: Unsupported boot with missing initial domid.". > > But that still doesn't fit the check, which compares the given ID (i.e. > there's nothing "missing" here) with the expected one. The "no ID given" > is handled by the plain if() that's first. It's not that the domid is missing from the node, but that the domid in the node doesn't match the initial domid. Maybe s/domid/domain, then? "Warning: Unsupported boot with missing initial domain" Cheers, Alejandro
On 15.04.2025 13:37, Alejandro Vallejo wrote: > On Tue Apr 15, 2025 at 7:21 AM BST, Jan Beulich wrote: >> On 14.04.2025 20:07, Alejandro Vallejo wrote: >>> On Wed Apr 9, 2025 at 11:15 PM BST, Denis Mukhin wrote: >>>> On Tuesday, April 8th, 2025 at 9:07 AM, Alejandro Vallejo <agarciav@amd.com> wrote: >>>>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>>>> return -ENODATA; >>>>> } >>>>> >>>>> + if ( bd->domid == DOMID_INVALID ) >>>>> >>>>> + bd->domid = get_initial_domain_id(); >>>>> >>>>> + else if ( bd->domid != get_initial_domain_id() ) >>>>> >>>>> + printk(XENLOG_WARNING >>>>> + "WARN: Booting without initial domid not supported.\n"); >>>> >>>> Drop WARN since the log message is XENLOG_WARNING level already? >>> >>> As mentioned elsewhere, the point of those prefixes are to be readable. >> >> This, however, imo is a matter of consistency across the codebase, not just >> within hyperlaunch. > > I agree. There is precedent though for certain printks to have a > collective pattern for ease of reading (e.g: spec_ctrl.c when printing > mitigations). With I'm merely justifying the 2 spaces followed by > lowercase. > > I did try to remove them and it was strictly harder to know what they > referred to. > >> Plus (again imo) if anything, prefixes that are part of >> the log output should contain proper words ("Warning:" or "Error:"), and >> they shouldn't needlessly "shout" (i.e. "FATAL:" is okay-ish to be all caps, >> but the others aren't). > > Right. I'm happy to rewrite them as "Warning: ..." and "Error: ..." FTAOD - in the common case I'd prefer such prefixes to be omitted. Which still means that there may be special cases where having them is warranted. Jan
On 15.04.2025 14:05, Alejandro Vallejo wrote: > On Tue Apr 15, 2025 at 7:27 AM BST, Jan Beulich wrote: >> On 14.04.2025 20:35, Alejandro Vallejo wrote: >>> On Thu Apr 10, 2025 at 12:49 PM BST, Jan Beulich wrote: >>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, >>>>> static int __init process_domain_node( >>>>> struct boot_info *bi, const void *fdt, int dom_node) >>>>> { >>>>> - int node; >>>>> + int node, property; >>>>> struct boot_domain *bd = &bi->domains[bi->nr_domains]; >>>>> const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; >>>>> int address_cells = fdt_address_cells(fdt, dom_node); >>>>> int size_cells = fdt_size_cells(fdt, dom_node); >>>>> >>>>> + fdt_for_each_property_offset(property, fdt, dom_node) >>>>> + { >>>>> + const struct fdt_property *prop; >>>>> + const char *prop_name; >>>>> + int name_len; >>>>> + >>>>> + prop = fdt_get_property_by_offset(fdt, property, NULL); >>>>> + if ( !prop ) >>>>> + continue; /* silently skip */ >>>>> + >>>>> + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); >>>>> + >>>>> + if ( strncmp(prop_name, "domid", name_len) == 0 ) >>>>> + { >>>>> + uint32_t val = DOMID_INVALID; >>>>> + if ( fdt_prop_as_u32(prop, &val) != 0 ) >>>>> + { >>>>> + printk(" failed processing domain id for domain %s\n", name); >>>>> + return -EINVAL; >>>>> + } >>>>> + if ( val >= DOMID_FIRST_RESERVED ) >>>>> + { >>>>> + printk(" invalid domain id for domain %s\n", name); >>>>> + return -EINVAL; >>>>> + } >>>>> + bd->domid = (domid_t)val; >>>> >>>> And a conflict with other domains' IDs will not be complained about? >>> >>> Hmmm... sure, I can iterate the domlist and check. >> >> Well, just to clarify: The checking doesn't necessarily need to happen here >> and now. It may also happen as domains are actually created. Yet then I >> think a pointer there (in a code comment) would be helpful here. > > That'd be fairly unsafe. In the case of parallel boot it'd be > indeterminate which VMs end up running if they happen to have a domid > clash. It's better to detect the error earlier and crash before any get > to start up. What's the unsafe aspect here? We'd crash either way; the domain(s) that may be successfully launched wouldn't make it very far. Yet irrespective - my request is _that_ collisions are checked for. I don't mind much _where_ that checking lives. >>>>> @@ -233,6 +264,12 @@ static int __init process_domain_node( >>>>> return -ENODATA; >>>>> } >>>>> >>>>> + if ( bd->domid == DOMID_INVALID ) >>>>> + bd->domid = get_initial_domain_id(); >>>>> + else if ( bd->domid != get_initial_domain_id() ) >>>>> + printk(XENLOG_WARNING >>>>> + "WARN: Booting without initial domid not supported.\n"); >>>> >>>> I'm not a native speaker, but (or perhaps because of that) "without" feels >>>> wrong here. >>> >>> It's probably the compound effect of without and "not supported". The >>> statement is correct, but it's arguably a bit obtuse. >>> >>> I'll replace it with "WARN: Unsupported boot with missing initial domid.". >> >> But that still doesn't fit the check, which compares the given ID (i.e. >> there's nothing "missing" here) with the expected one. The "no ID given" >> is handled by the plain if() that's first. > > It's not that the domid is missing from the node, but that the domid in > the node doesn't match the initial domid. Maybe s/domid/domain, then? > > "Warning: Unsupported boot with missing initial domain" I must be missing something: When it's "don't match" why would the message say "missing"? Jan
diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c index 0f5fd01557..4c6aafe195 100644 --- a/xen/arch/x86/domain-builder/fdt.c +++ b/xen/arch/x86/domain-builder/fdt.c @@ -8,6 +8,7 @@ #include <xen/libfdt/libfdt.h> #include <asm/bootinfo.h> +#include <asm/guest.h> #include <asm/page.h> #include <asm/setup.h> @@ -158,12 +159,42 @@ int __init fdt_read_multiboot_module(const void *fdt, int node, static int __init process_domain_node( struct boot_info *bi, const void *fdt, int dom_node) { - int node; + int node, property; struct boot_domain *bd = &bi->domains[bi->nr_domains]; const char *name = fdt_get_name(fdt, dom_node, NULL) ?: "unknown"; int address_cells = fdt_address_cells(fdt, dom_node); int size_cells = fdt_size_cells(fdt, dom_node); + fdt_for_each_property_offset(property, fdt, dom_node) + { + const struct fdt_property *prop; + const char *prop_name; + int name_len; + + prop = fdt_get_property_by_offset(fdt, property, NULL); + if ( !prop ) + continue; /* silently skip */ + + prop_name = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &name_len); + + if ( strncmp(prop_name, "domid", name_len) == 0 ) + { + uint32_t val = DOMID_INVALID; + if ( fdt_prop_as_u32(prop, &val) != 0 ) + { + printk(" failed processing domain id for domain %s\n", name); + return -EINVAL; + } + if ( val >= DOMID_FIRST_RESERVED ) + { + printk(" invalid domain id for domain %s\n", name); + return -EINVAL; + } + bd->domid = (domid_t)val; + printk(" domid: %d\n", bd->domid); + } + } + fdt_for_each_subnode(node, fdt, dom_node) { if ( fdt_node_check_compatible(fdt, node, "multiboot,kernel") == 0 ) @@ -233,6 +264,12 @@ static int __init process_domain_node( return -ENODATA; } + if ( bd->domid == DOMID_INVALID ) + bd->domid = get_initial_domain_id(); + else if ( bd->domid != get_initial_domain_id() ) + printk(XENLOG_WARNING + "WARN: Booting without initial domid not supported.\n"); + return 0; } diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 3dfa81b48c..db7280225e 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1033,8 +1033,9 @@ static struct domain *__init create_dom0(struct boot_info *bi) if ( iommu_enabled ) dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu; - /* Create initial domain. Not d0 for pvshim. */ - bd->domid = get_initial_domain_id(); + if ( bd->domid == DOMID_INVALID ) + /* Create initial domain. Not d0 for pvshim. */ + bd->domid = get_initial_domain_id(); d = domain_create(bd->domid, &dom0_cfg, pv_shim ? 0 : CDF_privileged); if ( IS_ERR(d) ) panic("Error creating d%u: %ld\n", bd->domid, PTR_ERR(d)); diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h index e473fbaf0c..3031bec90e 100644 --- a/xen/include/xen/libfdt/libfdt-xen.h +++ b/xen/include/xen/libfdt/libfdt-xen.h @@ -12,6 +12,7 @@ #define LIBFDT_XEN_H #include <xen/libfdt/libfdt.h> +#include <xen/errno.h> static inline int __init fdt_cell_as_u32(const fdt32_t *cell) { @@ -23,6 +24,16 @@ static inline uint64_t __init fdt_cell_as_u64(const fdt32_t *cell) return ((uint64_t)fdt32_to_cpu(cell[0]) << 32) | fdt32_to_cpu(cell[1]); } +static inline int __init fdt_prop_as_u32( + const struct fdt_property *prop, uint32_t *val) +{ + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u32) ) + return -EINVAL; + + *val = fdt_cell_as_u32((fdt32_t *)prop->data); + return 0; +} + static inline bool __init fdt_get_prop_offset( const void *fdt, int node, const char *name, unsigned long *offset) {