| Message ID | 1472468469-11560-2-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: > grub stubdom needs the start_info structure. Keep a copy of it in > pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should > default to "n" in order to have it enabled only when really needed. > I'm not sure I understand the rationale for this. Shouldn't start_info always be kept when mini-os is PV? Under what condition should it not be kept? Wei.
On 29/08/16 13:09, Wei Liu wrote: > On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: >> grub stubdom needs the start_info structure. Keep a copy of it in >> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should >> default to "n" in order to have it enabled only when really needed. >> > > I'm not sure I understand the rationale for this. > > Shouldn't start_info always be kept when mini-os is PV? Under what > condition should it not be kept? The application on top of Mini-OS shouldn't depend on Mini-OS being paravirtualized or not in the "normal" case. grub-stubdom is a special case, as it needs to kexec to a loaded kernel which in turn needs the start_info, of course. ioemu-stubdom OTOH should not need start_info as it could work on a HVMlite Mini-OS, too. The idea of removing start_info in my HVMlite series was driven by that thought: any application using start_info should break as it clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. pv-grub was an oversight here. I'm planning to modify ioemu-stubdom in the future to not use start_info and then let it run in HVMlite mode, too. Juergen
On 29/08/2016 12:17, Juergen Gross wrote: > On 29/08/16 13:09, Wei Liu wrote: >> On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: >>> grub stubdom needs the start_info structure. Keep a copy of it in >>> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should >>> default to "n" in order to have it enabled only when really needed. >>> >> I'm not sure I understand the rationale for this. >> >> Shouldn't start_info always be kept when mini-os is PV? Under what >> condition should it not be kept? > The application on top of Mini-OS shouldn't depend on Mini-OS being > paravirtualized or not in the "normal" case. grub-stubdom is a > special case, as it needs to kexec to a loaded kernel which in turn > needs the start_info, of course. > > ioemu-stubdom OTOH should not need start_info as it could work on > a HVMlite Mini-OS, too. > > The idea of removing start_info in my HVMlite series was driven by > that thought: any application using start_info should break as it > clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. > pv-grub was an oversight here. > > I'm planning to modify ioemu-stubdom in the future to not use > start_info and then let it run in HVMlite mode, too. There is an issue here between MiniOS itself, and "the stubdom application". There should be no circumstance where the stubdom application needs access (pv-grub being a special case, but maybe the kexec details can be hidden as well). However, while there is still relevant information in start_info, the low level PV bits should still have access. Moreover, it is necessary to always have access when doing suspend/resume. ~Andrew
On Mon, Aug 29, 2016 at 01:17:56PM +0200, Juergen Gross wrote: > On 29/08/16 13:09, Wei Liu wrote: > > On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: > >> grub stubdom needs the start_info structure. Keep a copy of it in > >> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should > >> default to "n" in order to have it enabled only when really needed. > >> > > > > I'm not sure I understand the rationale for this. > > > > Shouldn't start_info always be kept when mini-os is PV? Under what > > condition should it not be kept? > > The application on top of Mini-OS shouldn't depend on Mini-OS being > paravirtualized or not in the "normal" case. grub-stubdom is a > special case, as it needs to kexec to a loaded kernel which in turn > needs the start_info, of course. > > ioemu-stubdom OTOH should not need start_info as it could work on > a HVMlite Mini-OS, too. > > The idea of removing start_info in my HVMlite series was driven by > that thought: any application using start_info should break as it > clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. > pv-grub was an oversight here. > > I'm planning to modify ioemu-stubdom in the future to not use > start_info and then let it run in HVMlite mode, too. > > Right. I think we're on the same page regarding how apps should be like. Would it be sufficient that pv-grub has a hard dependency on PV mini-os? That should avoid yet another config option. And the semantics seems rather strange. But in the end I don't think I would argue strongly one way or the other because the config option your introduced defaults to n. Wei. > Juergen >
On 29/08/16 13:47, Andrew Cooper wrote: > On 29/08/2016 12:17, Juergen Gross wrote: >> On 29/08/16 13:09, Wei Liu wrote: >>> On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: >>>> grub stubdom needs the start_info structure. Keep a copy of it in >>>> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should >>>> default to "n" in order to have it enabled only when really needed. >>>> >>> I'm not sure I understand the rationale for this. >>> >>> Shouldn't start_info always be kept when mini-os is PV? Under what >>> condition should it not be kept? >> The application on top of Mini-OS shouldn't depend on Mini-OS being >> paravirtualized or not in the "normal" case. grub-stubdom is a >> special case, as it needs to kexec to a loaded kernel which in turn >> needs the start_info, of course. >> >> ioemu-stubdom OTOH should not need start_info as it could work on >> a HVMlite Mini-OS, too. >> >> The idea of removing start_info in my HVMlite series was driven by >> that thought: any application using start_info should break as it >> clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. >> pv-grub was an oversight here. >> >> I'm planning to modify ioemu-stubdom in the future to not use >> start_info and then let it run in HVMlite mode, too. > > There is an issue here between MiniOS itself, and "the stubdom application". Correct. > There should be no circumstance where the stubdom application needs > access (pv-grub being a special case, but maybe the kexec details can be > hidden as well). Indeed. I'll have a look if this is possible. In case I find a clean solution I'll send patches including one removing CONFIG_KEEP_STARTINFO again. > However, while there is still relevant information in start_info, the > low level PV bits should still have access. Moreover, it is necessary > to always have access when doing suspend/resume. The information from start_info is available inside Mini-OS. So this should be no problem. Juergen
On 29/08/2016 13:28, Juergen Gross wrote: > On 29/08/16 13:47, Andrew Cooper wrote: >> On 29/08/2016 12:17, Juergen Gross wrote: >>> On 29/08/16 13:09, Wei Liu wrote: >>>> On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: >>>>> grub stubdom needs the start_info structure. Keep a copy of it in >>>>> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should >>>>> default to "n" in order to have it enabled only when really needed. >>>>> >>>> I'm not sure I understand the rationale for this. >>>> >>>> Shouldn't start_info always be kept when mini-os is PV? Under what >>>> condition should it not be kept? >>> The application on top of Mini-OS shouldn't depend on Mini-OS being >>> paravirtualized or not in the "normal" case. grub-stubdom is a >>> special case, as it needs to kexec to a loaded kernel which in turn >>> needs the start_info, of course. >>> >>> ioemu-stubdom OTOH should not need start_info as it could work on >>> a HVMlite Mini-OS, too. >>> >>> The idea of removing start_info in my HVMlite series was driven by >>> that thought: any application using start_info should break as it >>> clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. >>> pv-grub was an oversight here. >>> >>> I'm planning to modify ioemu-stubdom in the future to not use >>> start_info and then let it run in HVMlite mode, too. >> There is an issue here between MiniOS itself, and "the stubdom application". > Correct. > >> There should be no circumstance where the stubdom application needs >> access (pv-grub being a special case, but maybe the kexec details can be >> hidden as well). > Indeed. I'll have a look if this is possible. In case I find a clean > solution I'll send patches including one removing CONFIG_KEEP_STARTINFO > again. > >> However, while there is still relevant information in start_info, the >> low level PV bits should still have access. Moreover, it is necessary >> to always have access when doing suspend/resume. > The information from start_info is available inside Mini-OS. So this > should be no problem. I have never understood MiniOS's decision to memcpy() it elsewhere. It is just a plain page of RAM set up by the domain builder; copying it elsewhere is just a waste of space. OTOH, you must pass a pointer to it in the suspend() hypercall, so the resume logic can re-modify part of it. ~Andrew
On 29/08/16 14:32, Andrew Cooper wrote: > On 29/08/2016 13:28, Juergen Gross wrote: >> On 29/08/16 13:47, Andrew Cooper wrote: >>> On 29/08/2016 12:17, Juergen Gross wrote: >>>> On 29/08/16 13:09, Wei Liu wrote: >>>>> On Mon, Aug 29, 2016 at 01:01:08PM +0200, Juergen Gross wrote: >>>>>> grub stubdom needs the start_info structure. Keep a copy of it in >>>>>> pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should >>>>>> default to "n" in order to have it enabled only when really needed. >>>>>> >>>>> I'm not sure I understand the rationale for this. >>>>> >>>>> Shouldn't start_info always be kept when mini-os is PV? Under what >>>>> condition should it not be kept? >>>> The application on top of Mini-OS shouldn't depend on Mini-OS being >>>> paravirtualized or not in the "normal" case. grub-stubdom is a >>>> special case, as it needs to kexec to a loaded kernel which in turn >>>> needs the start_info, of course. >>>> >>>> ioemu-stubdom OTOH should not need start_info as it could work on >>>> a HVMlite Mini-OS, too. >>>> >>>> The idea of removing start_info in my HVMlite series was driven by >>>> that thought: any application using start_info should break as it >>>> clearly wouldn't be agnostic of the mode (pv or HVMlite) of Mini-OS. >>>> pv-grub was an oversight here. >>>> >>>> I'm planning to modify ioemu-stubdom in the future to not use >>>> start_info and then let it run in HVMlite mode, too. >>> There is an issue here between MiniOS itself, and "the stubdom application". >> Correct. >> >>> There should be no circumstance where the stubdom application needs >>> access (pv-grub being a special case, but maybe the kexec details can be >>> hidden as well). >> Indeed. I'll have a look if this is possible. In case I find a clean >> solution I'll send patches including one removing CONFIG_KEEP_STARTINFO >> again. >> >>> However, while there is still relevant information in start_info, the >>> low level PV bits should still have access. Moreover, it is necessary >>> to always have access when doing suspend/resume. >> The information from start_info is available inside Mini-OS. So this >> should be no problem. > > I have never understood MiniOS's decision to memcpy() it elsewhere. It > is just a plain page of RAM set up by the domain builder; copying it > elsewhere is just a waste of space. > > OTOH, you must pass a pointer to it in the suspend() hypercall, so the > resume logic can re-modify part of it. Hmm, interesting detail. It took me some time to locate the code where the start_info parameter of the suspend call is being used. So the correct way to deal with start_info is to save the pointer to it and mark this page as being in use. I think I'll modify my patch to drop the new CONFIG parameter. Later I'll modify ioemu to no longer rely on start_info and pv-grub if possible, too. Then I can handle the start_info page the way it should be done. Juergen
diff --git a/Config.mk b/Config.mk index aa36761..ae18ffd 100644 --- a/Config.mk +++ b/Config.mk @@ -175,6 +175,7 @@ CONFIG_XENBUS ?= y CONFIG_XC ?=y CONFIG_LWIP ?= $(lwip) CONFIG_BALLOON ?= n +CONFIG_KEEP_STARTINFO ?= n # Export config items as compiler directives DEFINES-$(CONFIG_PARAVIRT) += -DCONFIG_PARAVIRT @@ -192,6 +193,7 @@ DEFINES-$(CONFIG_FBFRONT) += -DCONFIG_FBFRONT DEFINES-$(CONFIG_CONSFRONT) += -DCONFIG_CONSFRONT DEFINES-$(CONFIG_XENBUS) += -DCONFIG_XENBUS DEFINES-$(CONFIG_BALLOON) += -DCONFIG_BALLOON +DEFINES-$(CONFIG_KEEP_STARTINFO) += -DCONFIG_KEEP_STARTINFO # Override settings for this OS PTHREAD_LIBS = diff --git a/arch/x86/setup.c b/arch/x86/setup.c index 86955cf..1761b81 100644 --- a/arch/x86/setup.c +++ b/arch/x86/setup.c @@ -33,6 +33,14 @@ #include <xen/arch-x86/cpuid.h> #include <xen/arch-x86/hvm/start_info.h> +#ifdef CONFIG_KEEP_STARTINFO +/* + * This structure contains start-of-day info, such as pagetable base pointer, + * address of the shared_info structure, and things like that. + */ +union start_info_union start_info_union; +#endif + /* * Shared page for communicating with the hypervisor. * Events flags go here, for example. @@ -189,6 +197,10 @@ arch_init(void *par) /* print out some useful information */ print_start_of_day(par); +#ifdef CONFIG_KEEP_STARTINFO + memcpy(&start_info, par, sizeof(start_info)); +#endif + start_kernel(); } diff --git a/include/hypervisor.h b/include/hypervisor.h index 3073a8a..5475867 100644 --- a/include/hypervisor.h +++ b/include/hypervisor.h @@ -27,6 +27,19 @@ #include <mini-os/traps.h> /* hypervisor.c */ +#ifdef CONFIG_KEEP_STARTINFO +/* + * a placeholder for the start of day information passed up from the hypervisor + */ +union start_info_union +{ + start_info_t start_info; + char padding[512]; +}; +extern union start_info_union start_info_union; +#define start_info (start_info_union.start_info) +#endif + #ifndef CONFIG_PARAVIRT int hvm_get_parameter(int idx, uint64_t *value); int hvm_set_parameter(int idx, uint64_t value);
grub stubdom needs the start_info structure. Keep a copy of it in pv mini-os if configured via "CONFIG_KEEP_STARTINFO". This should default to "n" in order to have it enabled only when really needed. Signed-off-by: Juergen Gross <jgross@suse.com> --- Config.mk | 2 ++ arch/x86/setup.c | 12 ++++++++++++ include/hypervisor.h | 13 +++++++++++++ 3 files changed, 27 insertions(+)