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>
---
xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
xen/arch/x86/setup.c | 3 ++-
3 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
index bc8054a80ec1..3a6b4fbc09a9 100644
--- a/xen/arch/x86/domain_builder/fdt.c
+++ b/xen/arch/x86/domain_builder/fdt.c
@@ -9,6 +9,7 @@
#include <xen/rangeset.h> /* required for asm/setup.h */
#include <asm/bootinfo.h>
+#include <asm/guest.h>
#include <asm/page.h>
#include <asm/setup.h>
@@ -107,7 +108,7 @@ static int __init dom0less_module_node(
static int __init process_domain_node(
struct boot_info *bi, 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);
int address_size = fdt_address_cells(fdt, dom_node);
@@ -120,6 +121,28 @@ static int __init process_domain_node(
return -EINVAL;
}
+ fdt_for_each_property_offset(property, fdt, dom_node)
+ {
+ const struct fdt_property *prop;
+
+ prop = fdt_get_property_by_offset(fdt, property, NULL);
+ if ( !prop )
+ continue; /* silently skip */
+
+ if ( match_fdt_property(fdt, prop, "domid" ) )
+ {
+ uint32_t val = DOMID_INVALID;
+ if ( fdt_prop_as_u32(prop, &val) != 0 )
+ {
+ printk(" failed processing domain id for domain %s\n",
+ name == NULL ? "unknown" : 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 )
@@ -186,6 +209,12 @@ static int __init process_domain_node(
return -EFAULT;
}
+ if ( bd->domid == DOMID_INVALID )
+ bd->domid = get_initial_domain_id();
+ else
+ if ( bd->domid != get_initial_domain_id() )
+ printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");
+
return 0;
}
diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
index ab2b43872e25..06ead05a2583 100644
--- a/xen/arch/x86/domain_builder/fdt.h
+++ b/xen/arch/x86/domain_builder/fdt.h
@@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
return 0;
}
+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;
+
+ return fdt_cell_as_u32((fdt32_t *)prop->data, val);
+}
+
+static inline bool __init match_fdt_property(
+ const void *fdt, const struct fdt_property *prop, const char *s)
+{
+ int slen, len = strlen(s);
+ const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &slen);
+
+ return p && (slen == len) && (memcmp(p, s, len) == 0);
+}
+
static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
{
int ret;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eaac87b02f78..317349b80ac6 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
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 )
+ 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));
--
2.30.2On 23.11.2024 19:20, Daniel P. Smith wrote:
> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> return 0;
> }
>
> +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;
> +
> + return fdt_cell_as_u32((fdt32_t *)prop->data, val);
> +}
> +
> +static inline bool __init match_fdt_property(
> + const void *fdt, const struct fdt_property *prop, const char *s)
> +{
> + int slen, len = strlen(s);
Plain int isn't quite appropriate for strlen()'s return. It doesn't strictly
need to be size_t, but it should be at least unsigned int.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> 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 )
> + bd->domid = get_initial_domain_id();
Looks like the comment then wants to move, too.
Jan
On 12/2/24 07:02, Jan Beulich wrote:
> On 23.11.2024 19:20, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
>> return 0;
>> }
>>
>> +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;
>> +
>> + return fdt_cell_as_u32((fdt32_t *)prop->data, val);
>> +}
>> +
>> +static inline bool __init match_fdt_property(
>> + const void *fdt, const struct fdt_property *prop, const char *s)
>> +{
>> + int slen, len = strlen(s);
>
> Plain int isn't quite appropriate for strlen()'s return. It doesn't strictly
> need to be size_t, but it should be at least unsigned int.
Ack.
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
>> 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 )
>> + bd->domid = get_initial_domain_id();
>
> Looks like the comment then wants to move, too.
Okay.
v/r,
dps
On 12/11/24 11:21, Daniel P. Smith wrote:
> On 12/2/24 07:02, Jan Beulich wrote:
>> On 23.11.2024 19:20, Daniel P. Smith wrote:
>>> --- a/xen/arch/x86/domain_builder/fdt.h
>>> +++ b/xen/arch/x86/domain_builder/fdt.h
>>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const
>>> fdt32_t *cell, uint64_t *val)
>>> return 0;
>>> }
>>> +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;
>>> +
>>> + return fdt_cell_as_u32((fdt32_t *)prop->data, val);
>>> +}
>>> +
>>> +static inline bool __init match_fdt_property(
>>> + const void *fdt, const struct fdt_property *prop, const char *s)
>>> +{
>>> + int slen, len = strlen(s);
>>
>> Plain int isn't quite appropriate for strlen()'s return. It doesn't
>> strictly
>> need to be size_t, but it should be at least unsigned int.
>
> Ack.
I wanted to reply back on this one before posting the next version. By
implementing Jason's recommendation to unroll this function, the
strlen() call goes away and slen becomes name_len. But name_len remains
plain int to align with the fdt_get_string() parameter type.
v/r,
dps
On 2024-11-23 13:20, Daniel P. Smith wrote:
> 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>
> ---
> xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
> xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
> xen/arch/x86/setup.c | 3 ++-
> 3 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/domain_builder/fdt.c
> index bc8054a80ec1..3a6b4fbc09a9 100644
> --- a/xen/arch/x86/domain_builder/fdt.c
> +++ b/xen/arch/x86/domain_builder/fdt.c
> @@ -120,6 +121,28 @@ static int __init process_domain_node(
> return -EINVAL;
> }
>
> + fdt_for_each_property_offset(property, fdt, dom_node)
> + {
> + const struct fdt_property *prop;
> +
> + prop = fdt_get_property_by_offset(fdt, property, NULL);
> + if ( !prop )
> + continue; /* silently skip */
> +
> + if ( match_fdt_property(fdt, prop, "domid" ) )
> + {
> + uint32_t val = DOMID_INVALID;
> + if ( fdt_prop_as_u32(prop, &val) != 0 )
> + {
> + printk(" failed processing domain id for domain %s\n",
> + name == NULL ? "unknown" : name);
> + return -EINVAL;
> + }
Bounds check against DOMID_FIRST_RESERVED?
> + 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 )
> @@ -186,6 +209,12 @@ static int __init process_domain_node(
> return -EFAULT;
> }
>
> + if ( bd->domid == DOMID_INVALID )
> + bd->domid = get_initial_domain_id();
> + else
> + if ( bd->domid != get_initial_domain_id() )
single line "else if"?
> + printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n");
"unsupported"
Maybe "Booting without initial domid not supported"?
> +
> return 0;
> }
>
> diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/domain_builder/fdt.h
> index ab2b43872e25..06ead05a2583 100644
> --- a/xen/arch/x86/domain_builder/fdt.h
> +++ b/xen/arch/x86/domain_builder/fdt.h
> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const fdt32_t *cell, uint64_t *val)
> +static inline bool __init match_fdt_property(
> + const void *fdt, const struct fdt_property *prop, const char *s)
> +{
> + int slen, len = strlen(s);
> + const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff), &slen);
> +
> + return p && (slen == len) && (memcmp(p, s, len) == 0);
match_fdt_property() gets called more in later patches. I wonder if
const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff),
&slen);
should move into process_domain_node, and then the string is just
compared? Maybe it already gets optimized?
(Is there a way to disassemble .init.text with symbols?)
> +}
> +
> static inline int __init fdt_cmdline_prop_size(const void *fdt, int offset)
> {
> int ret;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index eaac87b02f78..317349b80ac6 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct boot_info *bi)
> 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 )
> + bd->domid = get_initial_domain_id();
This seems redundant with the earlier DOMID_INVALID check & setting. Or
does this handle the non-hyperlaunch case? Maybe it should move to
builder_init() for other non-hyperlaunch configuration?
> 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));
On 11/25/24 18:45, Jason Andryuk wrote:
> On 2024-11-23 13:20, Daniel P. Smith wrote:
>> 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>
>> ---
>> xen/arch/x86/domain_builder/fdt.c | 31 ++++++++++++++++++++++++++++++-
>> xen/arch/x86/domain_builder/fdt.h | 18 ++++++++++++++++++
>> xen/arch/x86/setup.c | 3 ++-
>> 3 files changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/domain_builder/fdt.c b/xen/arch/x86/
>> domain_builder/fdt.c
>> index bc8054a80ec1..3a6b4fbc09a9 100644
>> --- a/xen/arch/x86/domain_builder/fdt.c
>> +++ b/xen/arch/x86/domain_builder/fdt.c
>
>> @@ -120,6 +121,28 @@ static int __init process_domain_node(
>> return -EINVAL;
>> }
>> + fdt_for_each_property_offset(property, fdt, dom_node)
>> + {
>> + const struct fdt_property *prop;
>> +
>> + prop = fdt_get_property_by_offset(fdt, property, NULL);
>> + if ( !prop )
>> + continue; /* silently skip */
>> +
>> + if ( match_fdt_property(fdt, prop, "domid" ) )
>> + {
>> + uint32_t val = DOMID_INVALID;
>> + if ( fdt_prop_as_u32(prop, &val) != 0 )
>> + {
>> + printk(" failed processing domain id for domain %s\n",
>> + name == NULL ? "unknown" : name);
>> + return -EINVAL;
>> + }
>
> Bounds check against DOMID_FIRST_RESERVED?
Yah, that would be good.
>> + 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 )
>> @@ -186,6 +209,12 @@ static int __init process_domain_node(
>> return -EFAULT;
>> }
>> + if ( bd->domid == DOMID_INVALID )
>> + bd->domid = get_initial_domain_id();
>> + else
>> + if ( bd->domid != get_initial_domain_id() )
>
> single line "else if"?
Yep.
>> + printk(XENLOG_WARNING "WARN: unsuported booting not using
>> initial domid\n");
>
> "unsupported"
>
> Maybe "Booting without initial domid not supported"?
I am good with that phrasing.
>> +
>> return 0;
>> }
>> diff --git a/xen/arch/x86/domain_builder/fdt.h b/xen/arch/x86/
>> domain_builder/fdt.h
>> index ab2b43872e25..06ead05a2583 100644
>> --- a/xen/arch/x86/domain_builder/fdt.h
>> +++ b/xen/arch/x86/domain_builder/fdt.h
>> @@ -27,6 +27,24 @@ static inline int __init fdt_cell_as_u64(const
>> fdt32_t *cell, uint64_t *val)
>
>> +static inline bool __init match_fdt_property(
>> + const void *fdt, const struct fdt_property *prop, const char *s)
>> +{
>> + int slen, len = strlen(s);
>> + const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff),
>> &slen);
>> +
>> + return p && (slen == len) && (memcmp(p, s, len) == 0);
>
> match_fdt_property() gets called more in later patches. I wonder if
>
> const char *p = fdt_get_string(fdt, fdt32_to_cpu(prop->nameoff),
> &slen);
>
> should move into process_domain_node, and then the string is just
> compared? Maybe it already gets optimized?
Your approach would explicitly force a single fdt_get_string() call,
logic would remain readable, all while not hoping the optimizer can
catch it.
> (Is there a way to disassemble .init.text with symbols?)
>
>> +}
>> +
>> static inline int __init fdt_cmdline_prop_size(const void *fdt, int
>> offset)
>> {
>> int ret;
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index eaac87b02f78..317349b80ac6 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1020,7 +1020,8 @@ static struct domain *__init create_dom0(struct
>> boot_info *bi)
>> 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 )
>> + bd->domid = get_initial_domain_id();
>
> This seems redundant with the earlier DOMID_INVALID check & setting. Or
> does this handle the non-hyperlaunch case? Maybe it should move to
> builder_init() for other non-hyperlaunch configuration?
Even if the call to get_initial_domain_id() is moved to builder_init(),
I would not feel comfortable with assuming bd->domid is valid when
getting here. So I would still have the check, but with panic instead.
I'm open to making the change if the x86 maintainers are comfortable
with making the assumption.
v/r,
dps
On 26.11.2024 00:45, Jason Andryuk wrote: > On 2024-11-23 13:20, Daniel P. Smith wrote: >> @@ -186,6 +209,12 @@ static int __init process_domain_node( >> return -EFAULT; >> } >> >> + if ( bd->domid == DOMID_INVALID ) >> + bd->domid = get_initial_domain_id(); >> + else >> + if ( bd->domid != get_initial_domain_id() ) > > single line "else if"? Yes. >> + printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n"); > > "unsupported" > > Maybe "Booting without initial domid not supported"? Plus the line then wants splitting after XENLOG_WARNING. Jan
On 12/2/24 07:00, Jan Beulich wrote: > On 26.11.2024 00:45, Jason Andryuk wrote: >> On 2024-11-23 13:20, Daniel P. Smith wrote: >>> @@ -186,6 +209,12 @@ static int __init process_domain_node( >>> return -EFAULT; >>> } >>> >>> + if ( bd->domid == DOMID_INVALID ) >>> + bd->domid = get_initial_domain_id(); >>> + else >>> + if ( bd->domid != get_initial_domain_id() ) >> >> single line "else if"? > > Yes. Agreed. >>> + printk(XENLOG_WARNING "WARN: unsuported booting not using initial domid\n"); >> >> "unsupported" >> >> Maybe "Booting without initial domid not supported"? > > Plus the line then wants splitting after XENLOG_WARNING. Okay. v/r, dps
© 2016 - 2024 Red Hat, Inc.