[PATCH 0/2] mini-os: repair stubdom build

[PATCH 0/2] mini-os: repair stubdom build

[Juergen Gross]

The patch series adding HVMlite support for Mini-OS unfortunately
broke building Xen's stubdoms. This small series repairs the broken
bits again.

Please note that there is still some action required in Xen:
ioemu and grub stubdom minios.cfg files need CONFIG_KEEP_STARTINFO=y
to be added. I'm sending out a patch for Xen to do this in parallel.

Juergen Gross (2):
  mini-os: support keeping start_info structure conditionally
  mini-os: don't get xenbus parameters if xenbus is disabled

 Config.mk            |  2 ++
 arch/x86/setup.c     | 12 ++++++++++++
 include/hypervisor.h | 13 +++++++++++++
 include/xenbus.h     |  5 ++++-
 4 files changed, 31 insertions(+), 1 deletion(-)

-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Juergen Gross]

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(+)

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);
-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Wei Liu]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Minios-devel] [PATCH 1/2] mini-os: support keeping start_info structure conditionally

[Wei Liu]

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
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] mini-os: don't get xenbus parameters if xenbus is disabled

[Juergen Gross]

get_xenbus() should be called only if CONFIG_XENBUS is set.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 include/xenbus.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/xenbus.h b/include/xenbus.h
index 5646a25..c254652 100644
--- a/include/xenbus.h
+++ b/include/xenbus.h
@@ -9,10 +9,14 @@ typedef unsigned long xenbus_transaction_t;
 #ifdef CONFIG_XENBUS
 /* Initialize the XenBus system. */
 void init_xenbus(void);
+void get_xenbus(void *p);
 #else
 static inline void init_xenbus(void)
 {
 }
+static inline void get_xenbus(void *p)
+{
+}
 #endif
 
 /* Read the value associated with a path.  Returns a malloc'd error
@@ -31,7 +35,6 @@ typedef struct xenbus_event *xenbus_event_queue;
 
 extern uint32_t xenbus_evtchn;
 
-void get_xenbus(void *p);
 char *xenbus_watch_path_token(xenbus_transaction_t xbt, const char *path, const char *token, xenbus_event_queue *events);
 char *xenbus_unwatch_path_token(xenbus_transaction_t xbt, const char *path, const char *token);
 extern struct wait_queue_head xenbus_watch_queue;
-- 
2.6.6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] mini-os: don't get xenbus parameters if xenbus is disabled

[Wei Liu]

On Mon, Aug 29, 2016 at 01:01:09PM +0200, Juergen Gross wrote:
> get_xenbus() should be called only if CONFIG_XENBUS is set.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] mini-os: don't get xenbus parameters if xenbus is disabled

[Samuel Thibault]

Wei Liu, on Mon 29 Aug 2016 12:11:21 +0100, wrote:
> On Mon, Aug 29, 2016 at 01:01:09PM +0200, Juergen Gross wrote:
> > get_xenbus() should be called only if CONFIG_XENBUS is set.
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel