| Message ID | 20250408160802.49870-2-agarciav@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Hyperlaunch device tree for dom0 | expand |
On 08.04.2025 18:07, Alejandro Vallejo wrote: > From: "Daniel P. Smith" <dpsmith@apertussolutions.com> > > To begin moving toward allowing the hypervisor to construct more than one > domain at boot, a container is needed for a domain's build information. > Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial > struct boot_domain that encapsulate the build information for a domain. > > Add a kernel and ramdisk boot module reference along with a struct domain > reference to the new struct boot_domain. This allows a struct boot_domain > reference to be the only parameter necessary to pass down through the domain > construction call chain. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Acked-by: Jan Beulich <jbeulich@suse.com> > --- /dev/null > +++ b/xen/arch/x86/include/asm/boot-domain.h > @@ -0,0 +1,28 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (c) 2024 Apertus Solutions, LLC > + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> > + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> > + */ I wonder if the 2024-s here shouldn't have been amended by 2025 now. (I'm also, more generally, unclear about the purpose of such, especially when the file is really small and simple. It's only going to go stale, as we can see from various other files having such copyright lines.) Jan
On Wed Apr 9, 2025 at 7:24 AM BST, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> To begin moving toward allowing the hypervisor to construct more than one >> domain at boot, a container is needed for a domain's build information. >> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial >> struct boot_domain that encapsulate the build information for a domain. >> >> Add a kernel and ramdisk boot module reference along with a struct domain >> reference to the new struct boot_domain. This allows a struct boot_domain >> reference to be the only parameter necessary to pass down through the domain >> construction call chain. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks > >> --- /dev/null >> +++ b/xen/arch/x86/include/asm/boot-domain.h >> @@ -0,0 +1,28 @@ >> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >> +/* >> + * Copyright (c) 2024 Apertus Solutions, LLC >> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> >> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> >> + */ > > I wonder if the 2024-s here shouldn't have been amended by 2025 now. Maybe. I didn't think about it, tbh. One could argue that Daniel and Christopher's original contribution was indeed in 2024 and the fact they weren't committed until (hopefully!) 2025 doesn't remove the fact they did exist in some form in 2024. I don't particularly care either way, but tend to lean on the "it's fine how it is". Cheers, Alejandro
On 4/9/25 02:24, Jan Beulich wrote: > On 08.04.2025 18:07, Alejandro Vallejo wrote: >> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >> >> To begin moving toward allowing the hypervisor to construct more than one >> domain at boot, a container is needed for a domain's build information. >> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial >> struct boot_domain that encapsulate the build information for a domain. >> >> Add a kernel and ramdisk boot module reference along with a struct domain >> reference to the new struct boot_domain. This allows a struct boot_domain >> reference to be the only parameter necessary to pass down through the domain >> construction call chain. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> I have to object because the meaningless rename is going cause significant pain in the rebase of the follow-on series for no improved code clarity. V/r, DPS
On 4/9/25 06:28, Alejandro Vallejo wrote: > On Wed Apr 9, 2025 at 7:24 AM BST, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> To begin moving toward allowing the hypervisor to construct more than one >>> domain at boot, a container is needed for a domain's build information. >>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial >>> struct boot_domain that encapsulate the build information for a domain. >>> >>> Add a kernel and ramdisk boot module reference along with a struct domain >>> reference to the new struct boot_domain. This allows a struct boot_domain >>> reference to be the only parameter necessary to pass down through the domain >>> construction call chain. >>> >>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >> >> Acked-by: Jan Beulich <jbeulich@suse.com> > > Thanks > >> >>> --- /dev/null >>> +++ b/xen/arch/x86/include/asm/boot-domain.h >>> @@ -0,0 +1,28 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>> +/* >>> + * Copyright (c) 2024 Apertus Solutions, LLC >>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> >>> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> >>> + */ >> >> I wonder if the 2024-s here shouldn't have been amended by 2025 now. > > Maybe. I didn't think about it, tbh. One could argue that Daniel and > Christopher's original contribution was indeed in 2024 and the fact they > weren't committed until (hopefully!) 2025 doesn't remove the fact they > did exist in some form in 2024. > > I don't particularly care either way, but tend to lean on the "it's fine > how it is". Jan is correct, now that we have moved into 2025, they should be updated. Maybe we can actually get it all committed this year. v/r, dps
On 10.04.2025 15:09, Daniel P. Smith wrote: > On 4/9/25 02:24, Jan Beulich wrote: >> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>> >>> To begin moving toward allowing the hypervisor to construct more than one >>> domain at boot, a container is needed for a domain's build information. >>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial >>> struct boot_domain that encapsulate the build information for a domain. >>> >>> Add a kernel and ramdisk boot module reference along with a struct domain >>> reference to the new struct boot_domain. This allows a struct boot_domain >>> reference to be the only parameter necessary to pass down through the domain >>> construction call chain. >>> >>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >> >> Acked-by: Jan Beulich <jbeulich@suse.com> > > I have to object because the meaningless rename is going cause > significant pain in the rebase of the follow-on series for no improved > code clarity. Sorry, then an incremental patch undoing the rename that happened (with appropriate justification) will need proposing - the patch here has gone in already. Jan
On 2025-04-10 11:01, Jan Beulich wrote: > On 10.04.2025 15:09, Daniel P. Smith wrote: >> On 4/9/25 02:24, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>> >>>> To begin moving toward allowing the hypervisor to construct more than one >>>> domain at boot, a container is needed for a domain's build information. >>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the initial >>>> struct boot_domain that encapsulate the build information for a domain. >>>> >>>> Add a kernel and ramdisk boot module reference along with a struct domain >>>> reference to the new struct boot_domain. This allows a struct boot_domain >>>> reference to be the only parameter necessary to pass down through the domain >>>> construction call chain. >>>> >>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>> >>> Acked-by: Jan Beulich <jbeulich@suse.com> >> >> I have to object because the meaningless rename is going cause >> significant pain in the rebase of the follow-on series for no improved >> code clarity. > > Sorry, then an incremental patch undoing the rename that happened (with > appropriate justification) will need proposing - the patch here has gone > in already. Coming from a Linux background, ramdisk seemed more natural to me. But looking at hvm_start_info, the fields are called module there. And since we shouldn't tie this to the Linux naming, the more generic "module" name seemed fine to me. Regards, Jason
On 2025-04-10 09:13, Daniel P. Smith wrote: > On 4/9/25 06:28, Alejandro Vallejo wrote: >> On Wed Apr 9, 2025 at 7:24 AM BST, Jan Beulich wrote: >>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>> --- /dev/null >>>> +++ b/xen/arch/x86/include/asm/boot-domain.h >>>> @@ -0,0 +1,28 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */ >>>> +/* >>>> + * Copyright (c) 2024 Apertus Solutions, LLC >>>> + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> >>>> + * Copyright (c) 2024 Christopher Clark >>>> <christopher.w.clark@gmail.com> >>>> + */ >>> >>> I wonder if the 2024-s here shouldn't have been amended by 2025 now. >> >> Maybe. I didn't think about it, tbh. One could argue that Daniel and >> Christopher's original contribution was indeed in 2024 and the fact they >> weren't committed until (hopefully!) 2025 doesn't remove the fact they >> did exist in some form in 2024. >> >> I don't particularly care either way, but tend to lean on the "it's fine >> how it is". > > Jan is correct, now that we have moved into 2025, they should be > updated. Maybe we can actually get it all committed this year. These were written in 2024. Neither Christopher nor Dan touched them in 2025, so why would the date change? Regards, Jason
On 4/10/25 16:56, Jason Andryuk wrote: > On 2025-04-10 11:01, Jan Beulich wrote: >> On 10.04.2025 15:09, Daniel P. Smith wrote: >>> On 4/9/25 02:24, Jan Beulich wrote: >>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>> >>>>> To begin moving toward allowing the hypervisor to construct more >>>>> than one >>>>> domain at boot, a container is needed for a domain's build >>>>> information. >>>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the >>>>> initial >>>>> struct boot_domain that encapsulate the build information for a >>>>> domain. >>>>> >>>>> Add a kernel and ramdisk boot module reference along with a struct >>>>> domain >>>>> reference to the new struct boot_domain. This allows a struct >>>>> boot_domain >>>>> reference to be the only parameter necessary to pass down through >>>>> the domain >>>>> construction call chain. >>>>> >>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>>> >>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>> >>> I have to object because the meaningless rename is going cause >>> significant pain in the rebase of the follow-on series for no improved >>> code clarity. >> >> Sorry, then an incremental patch undoing the rename that happened (with >> appropriate justification) will need proposing - the patch here has gone >> in already. > > Coming from a Linux background, ramdisk seemed more natural to me. But > looking at hvm_start_info, the fields are called module there. And > since we shouldn't tie this to the Linux naming, the more generic > "module" name seemed fine to me. Again, as I have stated, ramdisk is not a Linux only concept. In fact, as Jan points out, initrd/initramfs are Linux specific implementations of a ramdisk for which Xen doesn't even fully support. I am inclined to ask the inverse of why hvm_start_info uses the name module. But that aside, let's consider the fact that the field is only populated by the device tree when a module type of BOOTMOD_RAMDISK is matched. And all the uses of the field are when its value is stored into a local variable called initrd. Though the biggest irony is that generally obtuse abstraction are routinely blocked unless there is a tangible future case. Yet none was offered in the comment. Thus on that principle alone, a request for a tangible future use should have been requested and provided for the change to be considered. V/r, DPS
On 16.04.2025 15:02, Daniel P. Smith wrote: > On 4/10/25 16:56, Jason Andryuk wrote: >> On 2025-04-10 11:01, Jan Beulich wrote: >>> On 10.04.2025 15:09, Daniel P. Smith wrote: >>>> On 4/9/25 02:24, Jan Beulich wrote: >>>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>>> >>>>>> To begin moving toward allowing the hypervisor to construct more >>>>>> than one >>>>>> domain at boot, a container is needed for a domain's build >>>>>> information. >>>>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the >>>>>> initial >>>>>> struct boot_domain that encapsulate the build information for a >>>>>> domain. >>>>>> >>>>>> Add a kernel and ramdisk boot module reference along with a struct >>>>>> domain >>>>>> reference to the new struct boot_domain. This allows a struct >>>>>> boot_domain >>>>>> reference to be the only parameter necessary to pass down through >>>>>> the domain >>>>>> construction call chain. >>>>>> >>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>>>> >>>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> I have to object because the meaningless rename is going cause >>>> significant pain in the rebase of the follow-on series for no improved >>>> code clarity. >>> >>> Sorry, then an incremental patch undoing the rename that happened (with >>> appropriate justification) will need proposing - the patch here has gone >>> in already. >> >> Coming from a Linux background, ramdisk seemed more natural to me. But >> looking at hvm_start_info, the fields are called module there. And >> since we shouldn't tie this to the Linux naming, the more generic >> "module" name seemed fine to me. > > Again, as I have stated, ramdisk is not a Linux only concept. In fact, > as Jan points out, initrd/initramfs are Linux specific implementations > of a ramdisk for which Xen doesn't even fully support. I am inclined to > ask the inverse of why hvm_start_info uses the name module. But that > aside, let's consider the fact that the field is only populated by the > device tree when a module type of BOOTMOD_RAMDISK is matched. And all > the uses of the field are when its value is stored into a local variable > called initrd. > > Though the biggest irony is that generally obtuse abstraction are > routinely blocked unless there is a tangible future case. Yet none was > offered in the comment. Thus on that principle alone, a request for a > tangible future use should have been requested and provided for the > change to be considered. Does it even need to be a _future_ use here? Aren't you working on abstracting domain creation, suitable (in principle) for all architectures? Isn't therefore a more generic name (as "module" is) preferable over a more specific one? Jan
V/r, Daniel P. Smith Apertus Solutions, LLC On 4/16/25 09:33, Jan Beulich wrote: > On 16.04.2025 15:02, Daniel P. Smith wrote: >> On 4/10/25 16:56, Jason Andryuk wrote: >>> On 2025-04-10 11:01, Jan Beulich wrote: >>>> On 10.04.2025 15:09, Daniel P. Smith wrote: >>>>> On 4/9/25 02:24, Jan Beulich wrote: >>>>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>>>> >>>>>>> To begin moving toward allowing the hypervisor to construct more >>>>>>> than one >>>>>>> domain at boot, a container is needed for a domain's build >>>>>>> information. >>>>>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the >>>>>>> initial >>>>>>> struct boot_domain that encapsulate the build information for a >>>>>>> domain. >>>>>>> >>>>>>> Add a kernel and ramdisk boot module reference along with a struct >>>>>>> domain >>>>>>> reference to the new struct boot_domain. This allows a struct >>>>>>> boot_domain >>>>>>> reference to be the only parameter necessary to pass down through >>>>>>> the domain >>>>>>> construction call chain. >>>>>>> >>>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>>>>> >>>>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>>>> >>>>> I have to object because the meaningless rename is going cause >>>>> significant pain in the rebase of the follow-on series for no improved >>>>> code clarity. >>>> >>>> Sorry, then an incremental patch undoing the rename that happened (with >>>> appropriate justification) will need proposing - the patch here has gone >>>> in already. >>> >>> Coming from a Linux background, ramdisk seemed more natural to me. But >>> looking at hvm_start_info, the fields are called module there. And >>> since we shouldn't tie this to the Linux naming, the more generic >>> "module" name seemed fine to me. >> >> Again, as I have stated, ramdisk is not a Linux only concept. In fact, >> as Jan points out, initrd/initramfs are Linux specific implementations >> of a ramdisk for which Xen doesn't even fully support. I am inclined to >> ask the inverse of why hvm_start_info uses the name module. But that >> aside, let's consider the fact that the field is only populated by the >> device tree when a module type of BOOTMOD_RAMDISK is matched. And all >> the uses of the field are when its value is stored into a local variable >> called initrd. >> >> Though the biggest irony is that generally obtuse abstraction are >> routinely blocked unless there is a tangible future case. Yet none was >> offered in the comment. Thus on that principle alone, a request for a >> tangible future use should have been requested and provided for the >> change to be considered. > > Does it even need to be a _future_ use here? Aren't you working on > abstracting domain creation, suitable (in principle) for all architectures? > Isn't therefore a more generic name (as "module" is) preferable over a more > specific one? Yes we are trying to build a future capability, but my point is let's consider all possible known OS's start up today. What other boot module could potentially be passed in that is exclusive of a ramdisk, thus allowing a multiplex of the field. And the answer is none. The other potential modules that could be passed in will need to be able to be coexist with a ramdisk module being passed. The immediate examples I can point to are, an SELinux policy file or a guest device tree. I'm not too familiar, perhaps a Zephyr guest may only take a guest DT, but a Linux guest may take an initrd and a DT. v/r, dps
On 16.04.2025 16:00, Daniel P. Smith wrote: > > > V/r, > Daniel P. Smith > Apertus Solutions, LLC > > On 4/16/25 09:33, Jan Beulich wrote: >> On 16.04.2025 15:02, Daniel P. Smith wrote: >>> On 4/10/25 16:56, Jason Andryuk wrote: >>>> On 2025-04-10 11:01, Jan Beulich wrote: >>>>> On 10.04.2025 15:09, Daniel P. Smith wrote: >>>>>> On 4/9/25 02:24, Jan Beulich wrote: >>>>>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>>>>> >>>>>>>> To begin moving toward allowing the hypervisor to construct more >>>>>>>> than one >>>>>>>> domain at boot, a container is needed for a domain's build >>>>>>>> information. >>>>>>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the >>>>>>>> initial >>>>>>>> struct boot_domain that encapsulate the build information for a >>>>>>>> domain. >>>>>>>> >>>>>>>> Add a kernel and ramdisk boot module reference along with a struct >>>>>>>> domain >>>>>>>> reference to the new struct boot_domain. This allows a struct >>>>>>>> boot_domain >>>>>>>> reference to be the only parameter necessary to pass down through >>>>>>>> the domain >>>>>>>> construction call chain. >>>>>>>> >>>>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>>>>>> >>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>>>>> >>>>>> I have to object because the meaningless rename is going cause >>>>>> significant pain in the rebase of the follow-on series for no improved >>>>>> code clarity. >>>>> >>>>> Sorry, then an incremental patch undoing the rename that happened (with >>>>> appropriate justification) will need proposing - the patch here has gone >>>>> in already. >>>> >>>> Coming from a Linux background, ramdisk seemed more natural to me. But >>>> looking at hvm_start_info, the fields are called module there. And >>>> since we shouldn't tie this to the Linux naming, the more generic >>>> "module" name seemed fine to me. >>> >>> Again, as I have stated, ramdisk is not a Linux only concept. In fact, >>> as Jan points out, initrd/initramfs are Linux specific implementations >>> of a ramdisk for which Xen doesn't even fully support. I am inclined to >>> ask the inverse of why hvm_start_info uses the name module. But that >>> aside, let's consider the fact that the field is only populated by the >>> device tree when a module type of BOOTMOD_RAMDISK is matched. And all >>> the uses of the field are when its value is stored into a local variable >>> called initrd. >>> >>> Though the biggest irony is that generally obtuse abstraction are >>> routinely blocked unless there is a tangible future case. Yet none was >>> offered in the comment. Thus on that principle alone, a request for a >>> tangible future use should have been requested and provided for the >>> change to be considered. >> >> Does it even need to be a _future_ use here? Aren't you working on >> abstracting domain creation, suitable (in principle) for all architectures? >> Isn't therefore a more generic name (as "module" is) preferable over a more >> specific one? > > Yes we are trying to build a future capability, but my point is let's > consider all possible known OS's start up today. What other boot module > could potentially be passed in that is exclusive of a ramdisk, thus > allowing a multiplex of the field. And the answer is none. Is it? What if you are to start a nested Xen with its own kernel, initrd and perhaps even an XSM policy "module"? Or anything else that is multi- module capable (possibly but not necessarily because of having started out as multiboot)? Jan
V/r, Daniel P. Smith Apertus Solutions, LLC On 4/16/25 10:06, Jan Beulich wrote: > On 16.04.2025 16:00, Daniel P. Smith wrote: >> >> >> V/r, >> Daniel P. Smith >> Apertus Solutions, LLC >> >> On 4/16/25 09:33, Jan Beulich wrote: >>> On 16.04.2025 15:02, Daniel P. Smith wrote: >>>> On 4/10/25 16:56, Jason Andryuk wrote: >>>>> On 2025-04-10 11:01, Jan Beulich wrote: >>>>>> On 10.04.2025 15:09, Daniel P. Smith wrote: >>>>>>> On 4/9/25 02:24, Jan Beulich wrote: >>>>>>>> On 08.04.2025 18:07, Alejandro Vallejo wrote: >>>>>>>>> From: "Daniel P. Smith" <dpsmith@apertussolutions.com> >>>>>>>>> >>>>>>>>> To begin moving toward allowing the hypervisor to construct more >>>>>>>>> than one >>>>>>>>> domain at boot, a container is needed for a domain's build >>>>>>>>> information. >>>>>>>>> Introduce a new header, <xen/asm/bootdomain.h>, that contains the >>>>>>>>> initial >>>>>>>>> struct boot_domain that encapsulate the build information for a >>>>>>>>> domain. >>>>>>>>> >>>>>>>>> Add a kernel and ramdisk boot module reference along with a struct >>>>>>>>> domain >>>>>>>>> reference to the new struct boot_domain. This allows a struct >>>>>>>>> boot_domain >>>>>>>>> reference to be the only parameter necessary to pass down through >>>>>>>>> the domain >>>>>>>>> construction call chain. >>>>>>>>> >>>>>>>>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >>>>>>>>> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> >>>>>>>> >>>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>>>>>> >>>>>>> I have to object because the meaningless rename is going cause >>>>>>> significant pain in the rebase of the follow-on series for no improved >>>>>>> code clarity. >>>>>> >>>>>> Sorry, then an incremental patch undoing the rename that happened (with >>>>>> appropriate justification) will need proposing - the patch here has gone >>>>>> in already. >>>>> >>>>> Coming from a Linux background, ramdisk seemed more natural to me. But >>>>> looking at hvm_start_info, the fields are called module there. And >>>>> since we shouldn't tie this to the Linux naming, the more generic >>>>> "module" name seemed fine to me. >>>> >>>> Again, as I have stated, ramdisk is not a Linux only concept. In fact, >>>> as Jan points out, initrd/initramfs are Linux specific implementations >>>> of a ramdisk for which Xen doesn't even fully support. I am inclined to >>>> ask the inverse of why hvm_start_info uses the name module. But that >>>> aside, let's consider the fact that the field is only populated by the >>>> device tree when a module type of BOOTMOD_RAMDISK is matched. And all >>>> the uses of the field are when its value is stored into a local variable >>>> called initrd. >>>> >>>> Though the biggest irony is that generally obtuse abstraction are >>>> routinely blocked unless there is a tangible future case. Yet none was >>>> offered in the comment. Thus on that principle alone, a request for a >>>> tangible future use should have been requested and provided for the >>>> change to be considered. >>> >>> Does it even need to be a _future_ use here? Aren't you working on >>> abstracting domain creation, suitable (in principle) for all architectures? >>> Isn't therefore a more generic name (as "module" is) preferable over a more >>> specific one? >> >> Yes we are trying to build a future capability, but my point is let's >> consider all possible known OS's start up today. What other boot module >> could potentially be passed in that is exclusive of a ramdisk, thus >> allowing a multiplex of the field. And the answer is none. > > Is it? What if you are to start a nested Xen with its own kernel, initrd > and perhaps even an XSM policy "module"? Or anything else that is multi- > module capable (possibly but not necessarily because of having started > out as multiboot)? As you said on the v2 thread, just one other OS also using such a concept doesn't mean much. With that said, as I am sure you are well aware of, the nested example is a far more complicated situation which there is currently no good abstraction in use. And btw, as far as ramdisk usage goes, I would be remised not to mention that another significantly large OS uses a ramdisk for boot, and that would be Windows. Windows leverages a ramdisk as configuration store between the winlaoder and tcbloader when doing a DRTM boot. v/r, dps
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 8191d90a22..0b467fd4a4 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -13,6 +13,7 @@ #include <xen/softirq.h> #include <asm/amd.h> +#include <asm/bootinfo.h> #include <asm/dom0_build.h> #include <asm/guest.h> #include <asm/hpet.h> @@ -614,9 +615,10 @@ int __init dom0_setup_permissions(struct domain *d) return rc; } -int __init construct_dom0(struct boot_info *bi, struct domain *d) +int __init construct_dom0(const struct boot_domain *bd) { int rc; + const struct domain *d = bd->d; /* Sanity! */ BUG_ON(!pv_shim && d->domain_id != 0); @@ -626,9 +628,9 @@ int __init construct_dom0(struct boot_info *bi, struct domain *d) process_pending_softirqs(); if ( is_hvm_domain(d) ) - rc = dom0_construct_pvh(bi, d); + rc = dom0_construct_pvh(bd); else if ( is_pv_domain(d) ) - rc = dom0_construct_pv(bi, d); + rc = dom0_construct_pv(bd); else panic("Cannot construct Dom0. No guest interface available\n"); diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 9fd68df7b9..2a094b3145 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -644,9 +644,11 @@ static bool __init check_and_adjust_load_address( } static int __init pvh_load_kernel( - struct domain *d, struct boot_module *image, struct boot_module *initrd, - paddr_t *entry, paddr_t *start_info_addr) + const struct boot_domain *bd, paddr_t *entry, paddr_t *start_info_addr) { + struct domain *d = bd->d; + struct boot_module *image = bd->kernel; + struct boot_module *initrd = bd->module; void *image_base = bootstrap_map_bm(image); void *image_start = image_base + image->headroom; unsigned long image_len = image->size; @@ -1328,26 +1330,17 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d) } } -int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d) +int __init dom0_construct_pvh(const struct boot_domain *bd) { paddr_t entry, start_info; - struct boot_module *image; - struct boot_module *initrd = NULL; - unsigned int idx; + struct domain *d = bd->d; int rc; printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id); - idx = first_boot_module_index(bi, BOOTMOD_KERNEL); - if ( idx >= bi->nr_modules ) + if ( bd->kernel == NULL ) panic("Missing kernel boot module for %pd construction\n", d); - image = &bi->mods[idx]; - - idx = first_boot_module_index(bi, BOOTMOD_RAMDISK); - if ( idx < bi->nr_modules ) - initrd = &bi->mods[idx]; - if ( is_hardware_domain(d) ) { /* @@ -1385,7 +1378,7 @@ int __init dom0_construct_pvh(struct boot_info *bi, struct domain *d) return rc; } - rc = pvh_load_kernel(d, image, initrd, &entry, &start_info); + rc = pvh_load_kernel(bd, &entry, &start_info); if ( rc ) { printk("Failed to load Dom0 kernel\n"); diff --git a/xen/arch/x86/include/asm/boot-domain.h b/xen/arch/x86/include/asm/boot-domain.h new file mode 100644 index 0000000000..5e1e1a0b61 --- /dev/null +++ b/xen/arch/x86/include/asm/boot-domain.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2024 Apertus Solutions, LLC + * Author: Daniel P. Smith <dpsmith@apertussolutions.com> + * Copyright (c) 2024 Christopher Clark <christopher.w.clark@gmail.com> + */ + +#ifndef __XEN_X86_BOOTDOMAIN_H__ +#define __XEN_X86_BOOTDOMAIN_H__ + +struct boot_domain { + struct boot_module *kernel; + struct boot_module *module; + + struct domain *d; +}; + +#endif + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h index f8b4229130..3afc214c17 100644 --- a/xen/arch/x86/include/asm/bootinfo.h +++ b/xen/arch/x86/include/asm/bootinfo.h @@ -11,10 +11,14 @@ #include <xen/init.h> #include <xen/multiboot.h> #include <xen/types.h> +#include <asm/boot-domain.h> /* Max number of boot modules a bootloader can provide in addition to Xen */ #define MAX_NR_BOOTMODS 63 +/* Max number of boot domains that Xen can construct */ +#define MAX_NR_BOOTDOMS 1 + /* Boot module binary type / purpose */ enum bootmod_type { BOOTMOD_UNKNOWN, @@ -78,6 +82,7 @@ struct boot_info { unsigned int nr_modules; struct boot_module mods[MAX_NR_BOOTMODS + 1]; + struct boot_domain domains[MAX_NR_BOOTDOMS]; }; /* diff --git a/xen/arch/x86/include/asm/dom0_build.h b/xen/arch/x86/include/asm/dom0_build.h index 2d67b17213..ff021c24af 100644 --- a/xen/arch/x86/include/asm/dom0_build.h +++ b/xen/arch/x86/include/asm/dom0_build.h @@ -13,9 +13,9 @@ unsigned long dom0_compute_nr_pages(struct domain *d, unsigned long initrd_len); int dom0_setup_permissions(struct domain *d); -struct boot_info; -int dom0_construct_pv(struct boot_info *bi, struct domain *d); -int dom0_construct_pvh(struct boot_info *bi, struct domain *d); +struct boot_domain; +int dom0_construct_pv(const struct boot_domain *bd); +int dom0_construct_pvh(const struct boot_domain *bd); unsigned long dom0_paging_pages(const struct domain *d, unsigned long nr_pages); diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index 5c2391a868..ac34c69855 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -26,8 +26,8 @@ void subarch_init_memory(void); void init_IRQ(void); -struct boot_info; -int construct_dom0(struct boot_info *bi, struct domain *d); +struct boot_domain; +int construct_dom0(const struct boot_domain *bd); void setup_io_bitmap(struct domain *d); diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index 96e28c7b6a..b485eea05f 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -355,7 +355,7 @@ static struct page_info * __init alloc_chunk(struct domain *d, return page; } -static int __init dom0_construct(struct boot_info *bi, struct domain *d) +static int __init dom0_construct(const struct boot_domain *bd) { unsigned int i; int rc, order, machine; @@ -371,14 +371,15 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d) struct page_info *page = NULL; unsigned int flush_flags = 0; start_info_t *si; + struct domain *d = bd->d; struct vcpu *v = d->vcpu[0]; - struct boot_module *image; - struct boot_module *initrd = NULL; + struct boot_module *image = bd->kernel; + struct boot_module *initrd = bd->module; void *image_base; unsigned long image_len; void *image_start; - unsigned long initrd_len = 0; + unsigned long initrd_len = initrd ? initrd->size : 0; l4_pgentry_t *l4tab = NULL, *l4start = NULL; l3_pgentry_t *l3tab = NULL, *l3start = NULL; @@ -416,22 +417,13 @@ static int __init dom0_construct(struct boot_info *bi, struct domain *d) printk(XENLOG_INFO "*** Building a PV Dom%d ***\n", d->domain_id); - i = first_boot_module_index(bi, BOOTMOD_KERNEL); - if ( i >= bi->nr_modules ) + if ( !image ) panic("Missing kernel boot module for %pd construction\n", d); - image = &bi->mods[i]; image_base = bootstrap_map_bm(image); image_len = image->size; image_start = image_base + image->headroom; - i = first_boot_module_index(bi, BOOTMOD_RAMDISK); - if ( i < bi->nr_modules ) - { - initrd = &bi->mods[i]; - initrd_len = initrd->size; - } - d->max_pages = ~0U; if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 ) @@ -1078,7 +1070,7 @@ out: return rc; } -int __init dom0_construct_pv(struct boot_info *bi, struct domain *d) +int __init dom0_construct_pv(const struct boot_domain *bd) { unsigned long cr4 = read_cr4(); int rc; @@ -1096,7 +1088,7 @@ int __init dom0_construct_pv(struct boot_info *bi, struct domain *d) write_cr4(cr4 & ~X86_CR4_SMAP); } - rc = dom0_construct(bi, d); + rc = dom0_construct(bd); if ( cr4 & X86_CR4_SMAP ) { diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index d70abb7e0c..ddb10ea03d 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -992,16 +992,9 @@ static struct domain *__init create_dom0(struct boot_info *bi) .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0, }, }; + struct boot_domain *bd = &bi->domains[0]; struct domain *d; domid_t domid; - struct boot_module *image; - unsigned int idx; - - idx = first_boot_module_index(bi, BOOTMOD_KERNEL); - if ( idx >= bi->nr_modules ) - panic("Missing kernel boot module for building domain\n"); - - image = &bi->mods[idx]; if ( opt_dom0_pvh ) { @@ -1028,11 +1021,11 @@ static struct domain *__init create_dom0(struct boot_info *bi) panic("Error creating d%uv0\n", domid); /* Grab the DOM0 command line. */ - if ( image->cmdline_pa || bi->kextra ) + if ( bd->kernel->cmdline_pa || bi->kextra ) { - if ( image->cmdline_pa ) - safe_strcpy( - cmdline, cmdline_cook(__va(image->cmdline_pa), bi->loader)); + if ( bd->kernel->cmdline_pa ) + safe_strcpy(cmdline, + cmdline_cook(__va(bd->kernel->cmdline_pa), bi->loader)); if ( bi->kextra ) /* kextra always includes exactly one leading space. */ @@ -1054,10 +1047,11 @@ static struct domain *__init create_dom0(struct boot_info *bi) safe_strcat(cmdline, acpi_param); } - image->cmdline_pa = __pa(cmdline); + bd->kernel->cmdline_pa = __pa(cmdline); } - if ( construct_dom0(bi, d) != 0 ) + bd->d = d; + if ( construct_dom0(bd) != 0 ) panic("Could not construct domain 0\n"); return d; @@ -1263,6 +1257,7 @@ void asmlinkage __init noreturn __start_xen(void) /* Dom0 kernel is always first */ bi->mods[0].type = BOOTMOD_KERNEL; + bi->domains[0].kernel = &bi->mods[0]; if ( pvh_boot ) { @@ -2129,6 +2124,7 @@ void asmlinkage __init noreturn __start_xen(void) if ( initrdidx < MAX_NR_BOOTMODS ) { bi->mods[initrdidx].type = BOOTMOD_RAMDISK; + bi->domains[0].module = &bi->mods[initrdidx]; if ( first_boot_module_index(bi, BOOTMOD_UNKNOWN) < MAX_NR_BOOTMODS ) printk(XENLOG_WARNING "Multiple initrd candidates, picking module #%u\n",