| Message ID | 20250408160802.49870-16-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 `cpus` property, named as such for dom0less compatibility, that > represents the maximum number of vpcus to allocate for a domain. In the device > tree, it will be encoded as a u32 value. > > Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com > > Reviewed-by: Jason Andryuk jason.andryuk@amd.com > > --- > xen/arch/x86/dom0_build.c | 3 +++ > xen/arch/x86/domain-builder/fdt.c | 11 +++++++++++ > xen/arch/x86/include/asm/boot-domain.h | 2 ++ > 3 files changed, 16 insertions(+) > > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c > index 36fb090643..7b3e31a08f 100644 > --- a/xen/arch/x86/dom0_build.c > +++ b/xen/arch/x86/dom0_build.c > @@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd) > if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) > > dom0_size.nr_pages = bd->max_pages; > > > + if ( opt_dom0_max_vcpus_max == UINT_MAX && bd->max_vcpus ) > > + opt_dom0_max_vcpus_max = bd->max_vcpus; > > + > 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 338b4838c2..5fcb767bdd 100644 > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -246,6 +246,17 @@ static int __init process_domain_node( > bd->max_pages = PFN_DOWN(kb * SZ_1K); > > printk(" max memory: %ld kb\n", kb); > } > + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) > + { > + uint32_t val = UINT_MAX; > + if ( fdt_prop_as_u32(prop, &val) != 0 ) > + { > + printk(" failed processing max_vcpus for domain %s\n", name); Suggest adding XENLOG_ERR to the error message. > + return -EINVAL; > + } > + bd->max_vcpus = val; > > + printk(" max vcpus: %d\n", bd->max_vcpus); > > + } > } > > 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 fa8ea1cc66..969c02a6ea 100644 > --- a/xen/arch/x86/include/asm/boot-domain.h > +++ b/xen/arch/x86/include/asm/boot-domain.h > @@ -22,6 +22,8 @@ struct boot_domain { > unsigned long min_pages; > unsigned long max_pages; > > + unsigned int max_vcpus; > + > struct boot_module *kernel; > struct boot_module *module; > const char *cmdline; > -- > 2.43.0
On 08.04.2025 18:07, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > Introduce the `cpus` property, named as such for dom0less compatibility, that > represents the maximum number of vpcus to allocate for a domain. In the device Nit: vcpus > --- a/xen/arch/x86/domain-builder/fdt.c > +++ b/xen/arch/x86/domain-builder/fdt.c > @@ -246,6 +246,17 @@ static int __init process_domain_node( > bd->max_pages = PFN_DOWN(kb * SZ_1K); > printk(" max memory: %ld kb\n", kb); > } > + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) > + { > + uint32_t val = UINT_MAX; > + if ( fdt_prop_as_u32(prop, &val) != 0 ) And again the same nit. > + { > + printk(" failed processing max_vcpus for domain %s\n", name); There's no "max_vcpus" being processed here; that purely ... > + return -EINVAL; > + } > + bd->max_vcpus = val; ... the internal name we use for the struct field etc. The user observing the message ought to be able to easily associate it back with the DT item. Jan
On Wed Apr 9, 2025 at 11:33 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 `cpus` property, named as such for dom0less compatibility, that >> represents the maximum number of vpcus to allocate for a domain. In the device >> tree, it will be encoded as a u32 value. >> >> Signed-off-by: Daniel P. Smith dpsmith@apertussolutions.com >> >> Reviewed-by: Jason Andryuk jason.andryuk@amd.com >> >> --- >> xen/arch/x86/dom0_build.c | 3 +++ >> xen/arch/x86/domain-builder/fdt.c | 11 +++++++++++ >> xen/arch/x86/include/asm/boot-domain.h | 2 ++ >> 3 files changed, 16 insertions(+) >> >> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c >> index 36fb090643..7b3e31a08f 100644 >> --- a/xen/arch/x86/dom0_build.c >> +++ b/xen/arch/x86/dom0_build.c >> @@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd) >> if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) >> >> dom0_size.nr_pages = bd->max_pages; >> >> >> + if ( opt_dom0_max_vcpus_max == UINT_MAX && bd->max_vcpus ) >> >> + opt_dom0_max_vcpus_max = bd->max_vcpus; >> >> + >> 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 338b4838c2..5fcb767bdd 100644 >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -246,6 +246,17 @@ static int __init process_domain_node( >> bd->max_pages = PFN_DOWN(kb * SZ_1K); >> >> printk(" max memory: %ld kb\n", kb); >> } >> + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) >> + { >> + uint32_t val = UINT_MAX; >> + if ( fdt_prop_as_u32(prop, &val) != 0 ) >> + { >> + printk(" failed processing max_vcpus for domain %s\n", name); > > Suggest adding XENLOG_ERR to the error message. And XENLOG_INFO to the one below. Ack. Cheers, Alejandro
On Thu Apr 10, 2025 at 1:08 PM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Introduce the `cpus` property, named as such for dom0less compatibility, that >> represents the maximum number of vpcus to allocate for a domain. In the device > > Nit: vcpus Ack, and same below > >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -246,6 +246,17 @@ static int __init process_domain_node( >> bd->max_pages = PFN_DOWN(kb * SZ_1K); >> printk(" max memory: %ld kb\n", kb); >> } >> + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) >> + { >> + uint32_t val = UINT_MAX; >> + if ( fdt_prop_as_u32(prop, &val) != 0 ) > > And again the same nit. > >> + { >> + printk(" failed processing max_vcpus for domain %s\n", name); > > There's no "max_vcpus" being processed here; that purely ... > >> + return -EINVAL; >> + } >> + bd->max_vcpus = val; > > ... the internal name we use for the struct field etc. The user observing the > message ought to be able to easily associate it back with the DT item. > > Jan Very true. I agree, and will change accordingly. Cheers, Alejandro
On 4/10/25 08:08, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> Introduce the `cpus` property, named as such for dom0less compatibility, that >> represents the maximum number of vpcus to allocate for a domain. In the device > > Nit: vcpus I agree with you here, the issue is that it was requested that we keep this field in line with Arm's DT, and they unfortunately used `cpus` to specify the vcpu allocation. >> --- a/xen/arch/x86/domain-builder/fdt.c >> +++ b/xen/arch/x86/domain-builder/fdt.c >> @@ -246,6 +246,17 @@ static int __init process_domain_node( >> bd->max_pages = PFN_DOWN(kb * SZ_1K); >> printk(" max memory: %ld kb\n", kb); >> } >> + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) >> + { >> + uint32_t val = UINT_MAX; >> + if ( fdt_prop_as_u32(prop, &val) != 0 ) > > And again the same nit. > >> + { >> + printk(" failed processing max_vcpus for domain %s\n", name); > > There's no "max_vcpus" being processed here; that purely ... > >> + return -EINVAL; >> + } >> + bd->max_vcpus = val; > > ... the internal name we use for the struct field etc. The user observing the > message ought to be able to easily associate it back with the DT item. > > Jan
On Wed Apr 16, 2025 at 2:42 PM BST, Daniel P. Smith wrote: > > On 4/10/25 08:08, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> Introduce the `cpus` property, named as such for dom0less compatibility, that >>> represents the maximum number of vpcus to allocate for a domain. In the device ^ | "vcpus" >> >> Nit: vcpus > > I agree with you here, the issue is that it was requested that we keep > this field in line with Arm's DT, and they unfortunately used `cpus` to > specify the vcpu allocation. He meant in the commit message. There's a typo. Cheers, Alejandro
On 16.04.2025 15:42, Daniel P. Smith wrote: > > On 4/10/25 08:08, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> Introduce the `cpus` property, named as such for dom0less compatibility, that >>> represents the maximum number of vpcus to allocate for a domain. In the device >> >> Nit: vcpus > > I agree with you here, the issue is that it was requested that we keep > this field in line with Arm's DT, and they unfortunately used `cpus` to > specify the vcpu allocation. You misunderstood, I think. The comment was on the mis-spelling in the latter of the quoted lines. Jan
On 4/16/25 09:54, Alejandro Vallejo wrote: > On Wed Apr 16, 2025 at 2:42 PM BST, Daniel P. Smith wrote: >> >> On 4/10/25 08:08, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>> >>>> Introduce the `cpus` property, named as such for dom0less compatibility, that >>>> represents the maximum number of vpcus to allocate for a domain. In the device > ^ > | > "vcpus" > >>> >>> Nit: vcpus >> >> I agree with you here, the issue is that it was requested that we keep >> this field in line with Arm's DT, and they unfortunately used `cpus` to >> specify the vcpu allocation. > > He meant in the commit message. There's a typo. Right, so I guess my response should have been further down. He points it out as a nit the second time, and then his final comment led me to believe he was raising an issue with vcpu vs cpu and not just the typo. v/r, dps
On 4/16/25 09:54, Jan Beulich wrote: > On 16.04.2025 15:42, Daniel P. Smith wrote: >> >> On 4/10/25 08:08, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>> >>>> Introduce the `cpus` property, named as such for dom0less compatibility, that >>>> represents the maximum number of vpcus to allocate for a domain. In the device >>> >>> Nit: vcpus >> >> I agree with you here, the issue is that it was requested that we keep >> this field in line with Arm's DT, and they unfortunately used `cpus` to >> specify the vcpu allocation. > > You misunderstood, I think. The comment was on the mis-spelling in the latter > of the quoted lines. > Then your latter comment is that you want the internal field to be renamed to cpu? Wouldn't that create further confusion of a physical cpu assignment vs virtual cpu allocation? v/r, dps
On 16.04.2025 16:19, Daniel P. Smith wrote: > On 4/16/25 09:54, Jan Beulich wrote: >> On 16.04.2025 15:42, Daniel P. Smith wrote: >>> >>> On 4/10/25 08:08, Jan Beulich wrote: >>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>> >>>>> Introduce the `cpus` property, named as such for dom0less compatibility, that >>>>> represents the maximum number of vpcus to allocate for a domain. In the device >>>> >>>> Nit: vcpus >>> >>> I agree with you here, the issue is that it was requested that we keep >>> this field in line with Arm's DT, and they unfortunately used `cpus` to >>> specify the vcpu allocation. >> >> You misunderstood, I think. The comment was on the mis-spelling in the latter >> of the quoted lines. > > Then your latter comment is that you want the internal field to be > renamed to cpu? No? Where are you taking that from? My comment was that a log message refers to "max_vcpus", when no field / property of that name is being processed. Going back to my reply (and seeing that Alejandro understood what I meant, afaict) I see nothing ambiguous there at all. In any event, ftaod, there were three entirely independent comments in my original reply to that patch. Jan > Wouldn't that create further confusion of a physical cpu > assignment vs virtual cpu allocation? > > v/r, > dps >
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 36fb090643..7b3e31a08f 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -635,6 +635,9 @@ int __init construct_dom0(const struct boot_domain *bd) if ( !get_memsize(&dom0_max_size, LONG_MAX) && bd->max_pages ) dom0_size.nr_pages = bd->max_pages; + if ( opt_dom0_max_vcpus_max == UINT_MAX && bd->max_vcpus ) + opt_dom0_max_vcpus_max = bd->max_vcpus; + 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 338b4838c2..5fcb767bdd 100644 --- a/xen/arch/x86/domain-builder/fdt.c +++ b/xen/arch/x86/domain-builder/fdt.c @@ -246,6 +246,17 @@ static int __init process_domain_node( bd->max_pages = PFN_DOWN(kb * SZ_1K); printk(" max memory: %ld kb\n", kb); } + else if ( strncmp(prop_name, "cpus", name_len) == 0 ) + { + uint32_t val = UINT_MAX; + if ( fdt_prop_as_u32(prop, &val) != 0 ) + { + printk(" failed processing max_vcpus for domain %s\n", name); + return -EINVAL; + } + bd->max_vcpus = val; + printk(" max vcpus: %d\n", bd->max_vcpus); + } } 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 fa8ea1cc66..969c02a6ea 100644 --- a/xen/arch/x86/include/asm/boot-domain.h +++ b/xen/arch/x86/include/asm/boot-domain.h @@ -22,6 +22,8 @@ struct boot_domain { unsigned long min_pages; unsigned long max_pages; + unsigned int max_vcpus; + struct boot_module *kernel; struct boot_module *module; const char *cmdline;