| Message ID | 20250408160802.49870-15-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 > > > Add three properties, memory, mem-min, and mem-max, to the domain node device > tree parsing to define the memory allocation for a domain. All three fields are > expressed in kb and written as a u64 in the device tree entries. > > Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com > > Reviewed-by: Jason Andryuk jason.andryuk@amd.com > > --- > xen/arch/x86/dom0_build.c | 8 ++++++ > xen/arch/x86/domain-builder/fdt.c | 34 ++++++++++++++++++++++++++ > xen/arch/x86/include/asm/boot-domain.h | 4 +++ > xen/include/xen/libfdt/libfdt-xen.h | 10 ++++++++ > 4 files changed, 56 insertions(+) > > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c > index 0b467fd4a4..36fb090643 100644 > --- a/xen/arch/x86/dom0_build.c > +++ b/xen/arch/x86/dom0_build.c > @@ -627,6 +627,14 @@ int __init construct_dom0(const struct boot_domain bd) > > process_pending_softirqs(); > > + / If param dom0_size was not set and HL config provided memory size */ > + if ( !get_memsize(&dom0_size, LONG_MAX) && bd->mem_pages ) > > + dom0_size.nr_pages = bd->mem_pages; > > + if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages ) > > + dom0_size.nr_pages = bd->min_pages; > > + if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) > > + dom0_size.nr_pages = bd->max_pages; > > + > if ( is_hvm_domain(d) ) > rc = dom0_construct_pvh(bd); > else if ( is_pv_domain(d) ) > diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c > index da65f6a5a0..338b4838c2 100644 > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -6,6 +6,7 @@ > #include <xen/init.h> > > #include <xen/lib.h> > > #include <xen/libfdt/libfdt.h> > > +#include <xen/sizes.h> > > > #include <asm/bootinfo.h> > > #include <asm/guest.h> > > @@ -212,6 +213,39 @@ static int __init process_domain_node( > else > printk("PV\n"); > } > + else if ( strncmp(prop_name, "memory", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > + { > + printk(" failed processing memory for domain %s\n", name); > + return -EINVAL; > + } > + bd->mem_pages = PFN_DOWN(kb * SZ_1K); Perhaps use shorter form of KB(kb) (KB() from include/xen/config.h)? What do you think? > > + printk(" memory: %ld kb\n", kb); > + } > + else if ( strncmp(prop_name, "mem-min", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > + { > + printk(" failed processing memory for domain %s\n", name); > + return -EINVAL; > + } > + bd->min_pages = PFN_DOWN(kb * SZ_1K); > > + printk(" min memory: %ld kb\n", kb); > + } > + else if ( strncmp(prop_name, "mem-max", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > + { > + printk(" failed processing memory for domain %s\n", name); > + return -EINVAL; > + } > + bd->max_pages = PFN_DOWN(kb * SZ_1K); > > + printk(" max memory: %ld kb\n", kb); > + } > } > > fdt_for_each_subnode(node, fdt, dom_node) > diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h > index e316d4bcde..fa8ea1cc66 100644 > --- a/xen/arch/x86/include/asm/boot-domain.h > +++ b/xen/arch/x86/include/asm/boot-domain.h > @@ -18,6 +18,10 @@ struct boot_domain { > #define BUILD_MODE_ENABLE_DM (1 << 1) /* HVM | PVH */ > uint32_t mode; > > + unsigned long mem_pages; > + unsigned long min_pages; > + unsigned long max_pages; > + > struct boot_module *kernel; > struct boot_module *module; > const char *cmdline; > diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h > index 3031bec90e..da43e12e38 100644 > --- a/xen/include/xen/libfdt/libfdt-xen.h > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -34,6 +34,16 @@ static inline int __init fdt_prop_as_u32( > return 0; > } > > +static inline int __init fdt_prop_as_u64( > + const struct fdt_property *prop, uint64_t *val) > +{ > + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) ) > > + return -EINVAL; > + > + *val = fdt_cell_as_u64((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: > @@ -212,6 +213,39 @@ static int __init process_domain_node( > else > printk("PV\n"); > } > + else if ( strncmp(prop_name, "memory", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) Nit (you know what I have to say here, and again below.) > + { > + printk(" failed processing memory for domain %s\n", name); > + return -EINVAL; Any reason to override fdt_prop_as_u64()'s return value here? > + } > + bd->mem_pages = PFN_DOWN(kb * SZ_1K); > + printk(" memory: %ld kb\n", kb); > + } > + else if ( strncmp(prop_name, "mem-min", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > + { > + printk(" failed processing memory for domain %s\n", name); > + return -EINVAL; > + } > + bd->min_pages = PFN_DOWN(kb * SZ_1K); > + printk(" min memory: %ld kb\n", kb); > + } > + else if ( strncmp(prop_name, "mem-max", name_len) == 0 ) > + { > + uint64_t kb; > + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > + { > + printk(" failed processing memory for domain %s\n", name); All three error messages being identical doesn't help diagnosing issues. > --- a/xen/include/xen/libfdt/libfdt-xen.h > +++ b/xen/include/xen/libfdt/libfdt-xen.h > @@ -34,6 +34,16 @@ static inline int __init fdt_prop_as_u32( > return 0; > } > > +static inline int __init fdt_prop_as_u64( > + const struct fdt_property *prop, uint64_t *val) > +{ > + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) ) > + return -EINVAL; > + > + *val = fdt_cell_as_u64((fdt32_t *)prop->data); Please avoid casting away const. Looks like I overlooked this in fdt_prop_as_u32() that was introduced by an earlier patch. Jan
On Wed Apr 9, 2025 at 11:29 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 >> >> >> Add three properties, memory, mem-min, and mem-max, to the domain node device >> tree parsing to define the memory allocation for a domain. All three fields are >> expressed in kb and written as a u64 in the device tree entries. >> >> Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com >> >> Reviewed-by: Jason Andryuk jason.andryuk@amd.com >> >> --- >> xen/arch/x86/dom0_build.c | 8 ++++++ >> xen/arch/x86/domain-builder/fdt.c | 34 ++++++++++++++++++++++++++ >> xen/arch/x86/include/asm/boot-domain.h | 4 +++ >> xen/include/xen/libfdt/libfdt-xen.h | 10 ++++++++ >> 4 files changed, 56 insertions(+) >> >> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c >> index 0b467fd4a4..36fb090643 100644 >> --- a/xen/arch/x86/dom0_build.c >> +++ b/xen/arch/x86/dom0_build.c >> @@ -627,6 +627,14 @@ int __init construct_dom0(const struct boot_domain bd) >> >> process_pending_softirqs(); >> >> + / If param dom0_size was not set and HL config provided memory size */ >> + if ( !get_memsize(&dom0_size, LONG_MAX) && bd->mem_pages ) >> >> + dom0_size.nr_pages = bd->mem_pages; >> >> + if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages ) >> >> + dom0_size.nr_pages = bd->min_pages; >> >> + if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) >> >> + dom0_size.nr_pages = bd->max_pages; >> >> + >> if ( is_hvm_domain(d) ) >> rc = dom0_construct_pvh(bd); >> else if ( is_pv_domain(d) ) >> diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c >> index da65f6a5a0..338b4838c2 100644 >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -6,6 +6,7 @@ >> #include <xen/init.h> >> >> #include <xen/lib.h> >> >> #include <xen/libfdt/libfdt.h> >> >> +#include <xen/sizes.h> >> >> >> #include <asm/bootinfo.h> >> >> #include <asm/guest.h> >> >> @@ -212,6 +213,39 @@ static int __init process_domain_node( >> else >> printk("PV\n"); >> } >> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >> + { >> + uint64_t kb; >> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >> + { >> + printk(" failed processing memory for domain %s\n", name); >> + return -EINVAL; >> + } >> + bd->mem_pages = PFN_DOWN(kb * SZ_1K); > > Perhaps use shorter form of KB(kb) (KB() from include/xen/config.h)? > > What do you think? Sure. Cheers, Alejandro
On Thu Apr 10, 2025 at 1:03 PM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> @@ -212,6 +213,39 @@ static int __init process_domain_node( >> else >> printk("PV\n"); >> } >> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >> + { >> + uint64_t kb; >> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > > Nit (you know what I have to say here, and again below.) Ack > >> + { >> + printk(" failed processing memory for domain %s\n", name); >> + return -EINVAL; > > Any reason to override fdt_prop_as_u64()'s return value here? Mostly to avoid needing to recover the error code. I'll just do it. > >> + } >> + bd->mem_pages = PFN_DOWN(kb * SZ_1K); >> + printk(" memory: %ld kb\n", kb); >> + } >> + else if ( strncmp(prop_name, "mem-min", name_len) == 0 ) >> + { >> + uint64_t kb; >> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >> + { >> + printk(" failed processing memory for domain %s\n", name); >> + return -EINVAL; >> + } >> + bd->min_pages = PFN_DOWN(kb * SZ_1K); >> + printk(" min memory: %ld kb\n", kb); >> + } >> + else if ( strncmp(prop_name, "mem-max", name_len) == 0 ) >> + { >> + uint64_t kb; >> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >> + { >> + printk(" failed processing memory for domain %s\n", name); > > All three error messages being identical doesn't help diagnosing issues. Indeed. Will add the prop that trigger each. > >> --- a/xen/include/xen/libfdt/libfdt-xen.h >> +++ b/xen/include/xen/libfdt/libfdt-xen.h >> @@ -34,6 +34,16 @@ static inline int __init fdt_prop_as_u32( >> return 0; >> } >> >> +static inline int __init fdt_prop_as_u64( >> + const struct fdt_property *prop, uint64_t *val) >> +{ >> + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) ) >> + return -EINVAL; >> + >> + *val = fdt_cell_as_u64((fdt32_t *)prop->data); > > Please avoid casting away const. Looks like I overlooked this in > fdt_prop_as_u32() that was introduced by an earlier patch. As part of v4 I moved this and fdt_prop_as_u32() earlier to patch8 and already adjusted accordingly. Cheers, Alejandro
On 4/10/25 08:03, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> @@ -212,6 +213,39 @@ static int __init process_domain_node( >> else >> printk("PV\n"); >> } >> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >> + { >> + uint64_t kb; >> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) > > Nit (you know what I have to say here, and again below.) > >> + { >> + printk(" failed processing memory for domain %s\n", name); >> + return -EINVAL; > > Any reason to override fdt_prop_as_u64()'s return value here? > IMHO this should be a function that libfdt should provide, but altering libftd directly would make uprev'ing it challenging. The least I could do is make the function behave like the rest of libfdt's helper functions. v/r, dps
On 16.04.2025 15:37, Daniel P. Smith wrote: > On 4/10/25 08:03, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> @@ -212,6 +213,39 @@ static int __init process_domain_node( >>> else >>> printk("PV\n"); >>> } >>> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >>> + { >>> + uint64_t kb; >>> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >> >> Nit (you know what I have to say here, and again below.) >> >>> + { >>> + printk(" failed processing memory for domain %s\n", name); >>> + return -EINVAL; >> >> Any reason to override fdt_prop_as_u64()'s return value here? > > IMHO this should be a function that libfdt should provide, but altering > libftd directly would make uprev'ing it challenging. The least I could > do is make the function behave like the rest of libfdt's helper functions. How's this related to the question that I raised? I didn't question the function, but a particular aspect of the specific use that is being made of it here. Jan
V/r, Daniel P. Smith Apertus Solutions, LLC On 4/16/25 09:41, Jan Beulich wrote: > On 16.04.2025 15:37, Daniel P. Smith wrote: >> On 4/10/25 08:03, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> @@ -212,6 +213,39 @@ static int __init process_domain_node( >>>> else >>>> printk("PV\n"); >>>> } >>>> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >>>> + { >>>> + uint64_t kb; >>>> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >>> >>> Nit (you know what I have to say here, and again below.) >>> >>>> + { >>>> + printk(" failed processing memory for domain %s\n", name); >>>> + return -EINVAL; >>> >>> Any reason to override fdt_prop_as_u64()'s return value here? >> >> IMHO this should be a function that libfdt should provide, but altering >> libftd directly would make uprev'ing it challenging. The least I could >> do is make the function behave like the rest of libfdt's helper functions. > > How's this related to the question that I raised? I didn't question the > function, but a particular aspect of the specific use that is being made > of it here. Your question was, "Any reason to override fdt_prop_as_u64()'s return value here?" And my answer was, I copied libfdt's behavior for its helper functions. IOW, to have the helper behave like libfdt's other helper functions. v/r, dps
On 16.04.2025 16:12, Daniel P. Smith wrote: > On 4/16/25 09:41, Jan Beulich wrote: >> On 16.04.2025 15:37, Daniel P. Smith wrote: >>> On 4/10/25 08:03, Jan Beulich wrote: >>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>> @@ -212,6 +213,39 @@ static int __init process_domain_node( >>>>> else >>>>> printk("PV\n"); >>>>> } >>>>> + else if ( strncmp(prop_name, "memory", name_len) == 0 ) >>>>> + { >>>>> + uint64_t kb; >>>>> + if ( fdt_prop_as_u64(prop, &kb) != 0 ) >>>> >>>> Nit (you know what I have to say here, and again below.) >>>> >>>>> + { >>>>> + printk(" failed processing memory for domain %s\n", name); >>>>> + return -EINVAL; >>>> >>>> Any reason to override fdt_prop_as_u64()'s return value here? >>> >>> IMHO this should be a function that libfdt should provide, but altering >>> libftd directly would make uprev'ing it challenging. The least I could >>> do is make the function behave like the rest of libfdt's helper functions. >> >> How's this related to the question that I raised? I didn't question the >> function, but a particular aspect of the specific use that is being made >> of it here. > > Your question was, "Any reason to override fdt_prop_as_u64()'s return > value here?" > > And my answer was, I copied libfdt's behavior for its helper functions. > IOW, to have the helper behave like libfdt's other helper functions. I'm sorry, but no. It meanwhile feels like you're intentionally misunderstanding. We're in process_domain_node() here. That's not a libfdt- like helper function? And the question was why this function throws away the return value it got from fdt_prop_as_u64(). Jan
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 0b467fd4a4..36fb090643 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -627,6 +627,14 @@ int __init construct_dom0(const struct boot_domain *bd) process_pending_softirqs(); + /* If param dom0_size was not set and HL config provided memory size */ + if ( !get_memsize(&dom0_size, LONG_MAX) && bd->mem_pages ) + dom0_size.nr_pages = bd->mem_pages; + if ( !get_memsize(&dom0_min_size, LONG_MAX) && bd->min_pages ) + dom0_size.nr_pages = bd->min_pages; + if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) + dom0_size.nr_pages = bd->max_pages; + if ( is_hvm_domain(d) ) rc = dom0_construct_pvh(bd); else if ( is_pv_domain(d) ) diff --git a/xen/arch/x86/domain-builder/fdt.c b/xen/arch/x86/domain-builder/fdt.c index da65f6a5a0..338b4838c2 100644 --- a/xen/arch/x86/domain-builder/fdt.c +++ b/xen/arch/x86/domain-builder/fdt.c @@ -6,6 +6,7 @@ #include <xen/init.h> #include <xen/lib.h> #include <xen/libfdt/libfdt.h> +#include <xen/sizes.h> #include <asm/bootinfo.h> #include <asm/guest.h> @@ -212,6 +213,39 @@ static int __init process_domain_node( else printk("PV\n"); } + else if ( strncmp(prop_name, "memory", name_len) == 0 ) + { + uint64_t kb; + if ( fdt_prop_as_u64(prop, &kb) != 0 ) + { + printk(" failed processing memory for domain %s\n", name); + return -EINVAL; + } + bd->mem_pages = PFN_DOWN(kb * SZ_1K); + printk(" memory: %ld kb\n", kb); + } + else if ( strncmp(prop_name, "mem-min", name_len) == 0 ) + { + uint64_t kb; + if ( fdt_prop_as_u64(prop, &kb) != 0 ) + { + printk(" failed processing memory for domain %s\n", name); + return -EINVAL; + } + bd->min_pages = PFN_DOWN(kb * SZ_1K); + printk(" min memory: %ld kb\n", kb); + } + else if ( strncmp(prop_name, "mem-max", name_len) == 0 ) + { + uint64_t kb; + if ( fdt_prop_as_u64(prop, &kb) != 0 ) + { + printk(" failed processing memory for domain %s\n", name); + return -EINVAL; + } + bd->max_pages = PFN_DOWN(kb * SZ_1K); + printk(" max memory: %ld kb\n", kb); + } } fdt_for_each_subnode(node, fdt, dom_node) diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h index e316d4bcde..fa8ea1cc66 100644 --- a/xen/arch/x86/include/asm/boot-domain.h +++ b/xen/arch/x86/include/asm/boot-domain.h @@ -18,6 +18,10 @@ struct boot_domain { #define BUILD_MODE_ENABLE_DM (1 << 1) /* HVM | PVH */ uint32_t mode; + unsigned long mem_pages; + unsigned long min_pages; + unsigned long max_pages; + struct boot_module *kernel; struct boot_module *module; const char *cmdline; diff --git a/xen/include/xen/libfdt/libfdt-xen.h b/xen/include/xen/libfdt/libfdt-xen.h index 3031bec90e..da43e12e38 100644 --- a/xen/include/xen/libfdt/libfdt-xen.h +++ b/xen/include/xen/libfdt/libfdt-xen.h @@ -34,6 +34,16 @@ static inline int __init fdt_prop_as_u32( return 0; } +static inline int __init fdt_prop_as_u64( + const struct fdt_property *prop, uint64_t *val) +{ + if ( !prop || fdt32_to_cpu(prop->len) < sizeof(u64) ) + return -EINVAL; + + *val = fdt_cell_as_u64((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) {