[PATCH 0/2] Minor improvement to early boot code

[PATCH 0/2] Minor improvement to early boot code

[Wei Liu]

Wei Liu (2):
  x86: make sure module array is large enough in pvh-boot.c
  xen: make opt_xen_console true before console initialisation

 xen/arch/x86/guest/pvh-boot.c | 9 ++++++++-
 xen/drivers/char/console.c    | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.11.0


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

[PATCH 1/2] x86: make sure module array is large enough in pvh-boot.c

[Wei Liu]

The relocation code in __start_xen requires one extra element in the
module array. By the looks of it the temporary array is already large
enough. Add a BUG_ON to catch any issue in the future.

While at it, turn another ASSERT to BUG_ON as well.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/guest/pvh-boot.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 0e9e5bfdf6..2e103105fa 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -42,7 +42,14 @@ static void __init convert_pvh_info(void)
     module_t *mod;
     unsigned int i;
 
-    ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
+    BUG_ON(pvh_info->magic != XEN_HVM_START_MAGIC_VALUE);
+
+    /*
+     * Temporary module array needs to be at least one element bigger than
+     * required. The extra element is used to aid relocation. See
+     * arch/x86/setup.c:__start_xen().
+     */
+    BUG_ON(ARRAY_SIZE(pvh_mbi_mods) <= pvh_info->nr_modules);
 
     /*
      * Turn hvm_start_info into mbi. Luckily all modules are placed under 4GB
-- 
2.11.0


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

[PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

This helps capture issues before console is initialised. The impact is
minimal because it only affects Xen running in as a guest.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/drivers/char/console.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index e48039dd82..bbec98304e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
 static int __read_mostly sercon_handle = -1;
 
 #ifdef CONFIG_X86
-static bool __read_mostly opt_console_xen; /* console=xen */
+/* Set to true at start of day to catch early boot issues */
+static bool __read_mostly opt_console_xen = true; /* console=xen */
 #endif
 
 static DEFINE_SPINLOCK(console_lock);
@@ -821,6 +822,10 @@ void __init console_init_preirq(void)
 
     serial_init_preirq();
 
+#ifdef CONFIG_X86
+    opt_console_xen = false;
+#endif
+
     /* Where should console output go? */
     for ( p = opt_console; p != NULL; p = strchr(p, ',') )
     {
-- 
2.11.0


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

Re: [PATCH 1/2] x86: make sure module array is large enough in pvh-boot.c

[Jan Beulich]

>>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> The relocation code in __start_xen requires one extra element in the
> module array. By the looks of it the temporary array is already large
> enough. Add a BUG_ON to catch any issue in the future.
> 
> While at it, turn another ASSERT to BUG_ON as well.

Hmm, a little strange - I thought we had agreed on panic() before.
The extra output BUG_ON() produces is unlikely to be helpful here.
With this switched to panic() it certainly can have my ack.

Jan



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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Jan Beulich]

>>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
>  static int __read_mostly sercon_handle = -1;
>  
>  #ifdef CONFIG_X86
> -static bool __read_mostly opt_console_xen; /* console=xen */
> +/* Set to true at start of day to catch early boot issues */
> +static bool __read_mostly opt_console_xen = true; /* console=xen */

When Andrew suggested this, iirc he said to make the variable
tristate. Otherwise ...

> @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
>  
>      serial_init_preirq();
>  
> +#ifdef CONFIG_X86
> +    opt_console_xen = false;
> +#endif

... you possibly override a user specified "true" here.

Jan



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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
> >>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
> >  static int __read_mostly sercon_handle = -1;
> >  
> >  #ifdef CONFIG_X86
> > -static bool __read_mostly opt_console_xen; /* console=xen */
> > +/* Set to true at start of day to catch early boot issues */
> > +static bool __read_mostly opt_console_xen = true; /* console=xen */
> 
> When Andrew suggested this, iirc he said to make the variable
> tristate. Otherwise ...

I figure it doesn't need to be tristate.

> 
> > @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
> >  
> >      serial_init_preirq();
> >  
> > +#ifdef CONFIG_X86
> > +    opt_console_xen = false;
> > +#endif
> 
> ... you possibly override a user specified "true" here.

Do I? This is right before option parsing, during which opt_console_xen
is set.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 1/2] x86: make sure module array is large enough in pvh-boot.c

[Wei Liu]

On Fri, Sep 28, 2018 at 09:06:25AM -0600, Jan Beulich wrote:
> >>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> > The relocation code in __start_xen requires one extra element in the
> > module array. By the looks of it the temporary array is already large
> > enough. Add a BUG_ON to catch any issue in the future.
> > 
> > While at it, turn another ASSERT to BUG_ON as well.
> 
> Hmm, a little strange - I thought we had agreed on panic() before.
> The extra output BUG_ON() produces is unlikely to be helpful here.
> With this switched to panic() it certainly can have my ack.

Oops, I have no idea why I agreed to use panic but wrote BUG_ON instead.
My memory has failed me.

I will resend this patch.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Andrew Cooper]

On 28/09/18 16:12, Wei Liu wrote:
> On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
>>>>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
>>> --- a/xen/drivers/char/console.c
>>> +++ b/xen/drivers/char/console.c
>>> @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
>>>  static int __read_mostly sercon_handle = -1;
>>>  
>>>  #ifdef CONFIG_X86
>>> -static bool __read_mostly opt_console_xen; /* console=xen */
>>> +/* Set to true at start of day to catch early boot issues */
>>> +static bool __read_mostly opt_console_xen = true; /* console=xen */
>> When Andrew suggested this, iirc he said to make the variable
>> tristate. Otherwise ...
> I figure it doesn't need to be tristate.
>
>>> @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
>>>  
>>>      serial_init_preirq();
>>>  
>>> +#ifdef CONFIG_X86
>>> +    opt_console_xen = false;
>>> +#endif
>> ... you possibly override a user specified "true" here.
> Do I? This is right before option parsing, during which opt_console_xen
> is set.

This is fine in principle, but you can't initialise it to true at
compile time, or native boots will try using hypercalls / port 0xe9.

The best bet is to set it to true in the PVH entry asm.

~Andrew

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 04:13:51PM +0100, Andrew Cooper wrote:
> On 28/09/18 16:12, Wei Liu wrote:
> > On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
> >>>>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> >>> --- a/xen/drivers/char/console.c
> >>> +++ b/xen/drivers/char/console.c
> >>> @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
> >>>  static int __read_mostly sercon_handle = -1;
> >>>  
> >>>  #ifdef CONFIG_X86
> >>> -static bool __read_mostly opt_console_xen; /* console=xen */
> >>> +/* Set to true at start of day to catch early boot issues */
> >>> +static bool __read_mostly opt_console_xen = true; /* console=xen */
> >> When Andrew suggested this, iirc he said to make the variable
> >> tristate. Otherwise ...
> > I figure it doesn't need to be tristate.
> >
> >>> @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
> >>>  
> >>>      serial_init_preirq();
> >>>  
> >>> +#ifdef CONFIG_X86
> >>> +    opt_console_xen = false;
> >>> +#endif
> >> ... you possibly override a user specified "true" here.
> > Do I? This is right before option parsing, during which opt_console_xen
> > is set.
> 
> This is fine in principle, but you can't initialise it to true at
> compile time, or native boots will try using hypercalls / port 0xe9.
> 
> The best bet is to set it to true in the PVH entry asm.

Right. I will set it in the PVH entry instead.

Wei.

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Jan Beulich]

>>> On 28.09.18 at 17:12, <wei.liu2@citrix.com> wrote:
> On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
>> >>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
>> > --- a/xen/drivers/char/console.c
>> > +++ b/xen/drivers/char/console.c
>> > @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
>> >  static int __read_mostly sercon_handle = -1;
>> >  
>> >  #ifdef CONFIG_X86
>> > -static bool __read_mostly opt_console_xen; /* console=xen */
>> > +/* Set to true at start of day to catch early boot issues */
>> > +static bool __read_mostly opt_console_xen = true; /* console=xen */
>> 
>> When Andrew suggested this, iirc he said to make the variable
>> tristate. Otherwise ...
> 
> I figure it doesn't need to be tristate.
> 
>> 
>> > @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
>> >  
>> >      serial_init_preirq();
>> >  
>> > +#ifdef CONFIG_X86
>> > +    opt_console_xen = false;
>> > +#endif
>> 
>> ... you possibly override a user specified "true" here.
> 
> Do I? This is right before option parsing, during which opt_console_xen
> is set.

Hmm, I see - I didn't recall we still have this ad hoc parsing in
place, instead of using custom_param(). But isn't this too early then,
as messages issued from the parsing code then would not be visible?

Jan



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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 09:19:56AM -0600, Jan Beulich wrote:
> >>> On 28.09.18 at 17:12, <wei.liu2@citrix.com> wrote:
> > On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
> >> >>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> >> > --- a/xen/drivers/char/console.c
> >> > +++ b/xen/drivers/char/console.c
> >> > @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
> >> >  static int __read_mostly sercon_handle = -1;
> >> >  
> >> >  #ifdef CONFIG_X86
> >> > -static bool __read_mostly opt_console_xen; /* console=xen */
> >> > +/* Set to true at start of day to catch early boot issues */
> >> > +static bool __read_mostly opt_console_xen = true; /* console=xen */
> >> 
> >> When Andrew suggested this, iirc he said to make the variable
> >> tristate. Otherwise ...
> > 
> > I figure it doesn't need to be tristate.
> > 
> >> 
> >> > @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
> >> >  
> >> >      serial_init_preirq();
> >> >  
> >> > +#ifdef CONFIG_X86
> >> > +    opt_console_xen = false;
> >> > +#endif
> >> 
> >> ... you possibly override a user specified "true" here.
> > 
> > Do I? This is right before option parsing, during which opt_console_xen
> > is set.
> 
> Hmm, I see - I didn't recall we still have this ad hoc parsing in
> place, instead of using custom_param(). But isn't this too early then,
> as messages issued from the parsing code then would not be visible?

To me it doesn't make anything worse than before. But I see your point.
I will see if there is an easy way to fix this.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 04:23:24PM +0100, Wei Liu wrote:
> On Fri, Sep 28, 2018 at 09:19:56AM -0600, Jan Beulich wrote:
> > >>> On 28.09.18 at 17:12, <wei.liu2@citrix.com> wrote:
> > > On Fri, Sep 28, 2018 at 09:08:34AM -0600, Jan Beulich wrote:
> > >> >>> On 28.09.18 at 10:24, <wei.liu2@citrix.com> wrote:
> > >> > --- a/xen/drivers/char/console.c
> > >> > +++ b/xen/drivers/char/console.c
> > >> > @@ -91,7 +91,8 @@ static uint32_t conringc, conringp;
> > >> >  static int __read_mostly sercon_handle = -1;
> > >> >  
> > >> >  #ifdef CONFIG_X86
> > >> > -static bool __read_mostly opt_console_xen; /* console=xen */
> > >> > +/* Set to true at start of day to catch early boot issues */
> > >> > +static bool __read_mostly opt_console_xen = true; /* console=xen */
> > >> 
> > >> When Andrew suggested this, iirc he said to make the variable
> > >> tristate. Otherwise ...
> > > 
> > > I figure it doesn't need to be tristate.
> > > 
> > >> 
> > >> > @@ -821,6 +822,10 @@ void __init console_init_preirq(void)
> > >> >  
> > >> >      serial_init_preirq();
> > >> >  
> > >> > +#ifdef CONFIG_X86
> > >> > +    opt_console_xen = false;
> > >> > +#endif
> > >> 
> > >> ... you possibly override a user specified "true" here.
> > > 
> > > Do I? This is right before option parsing, during which opt_console_xen
> > > is set.
> > 
> > Hmm, I see - I didn't recall we still have this ad hoc parsing in
> > place, instead of using custom_param(). But isn't this too early then,
> > as messages issued from the parsing code then would not be visible?
> 
> To me it doesn't make anything worse than before. But I see your point.
> I will see if there is an easy way to fix this.

At the moment the ordering of the options matter, which means if "xen"
is not the first option we can lose output.

I think one easy way of improving is to parse opt_console twice. The
first time solely looks for "xen" and the second time parses the rest
options.


Wei.

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Jan Beulich]

>>> On 28.09.18 at 17:53, <wei.liu2@citrix.com> wrote:
> At the moment the ordering of the options matter, which means if "xen"
> is not the first option we can lose output.
> 
> I think one easy way of improving is to parse opt_console twice. The
> first time solely looks for "xen" and the second time parses the rest
> options.

What's wrong with making the variable a tristate, and move your
set-to-false after the parsing loop? If the user said "false" for the
option, then they apparently don't care much about the output.

But yes, what you suggest is an option as well.

Jan



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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 10:05:21AM -0600, Jan Beulich wrote:
> >>> On 28.09.18 at 17:53, <wei.liu2@citrix.com> wrote:
> > At the moment the ordering of the options matter, which means if "xen"
> > is not the first option we can lose output.
> > 
> > I think one easy way of improving is to parse opt_console twice. The
> > first time solely looks for "xen" and the second time parses the rest
> > options.
> 
> What's wrong with making the variable a tristate, and move your
> set-to-false after the parsing loop? If the user said "false" for the
> option, then they apparently don't care much about the output.

Oh, there is nothing wrong with that. It just I didn't think about that
solution while thinking about this problem.

> 
> But yes, what you suggest is an option as well.

Yeah, I already got code for it.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH 2/2] xen: make opt_xen_console true before console initialisation

[Wei Liu]

On Fri, Sep 28, 2018 at 05:19:50PM +0100, Wei Liu wrote:
> On Fri, Sep 28, 2018 at 10:05:21AM -0600, Jan Beulich wrote:
> > >>> On 28.09.18 at 17:53, <wei.liu2@citrix.com> wrote:
> > > At the moment the ordering of the options matter, which means if "xen"
> > > is not the first option we can lose output.
> > > 
> > > I think one easy way of improving is to parse opt_console twice. The
> > > first time solely looks for "xen" and the second time parses the rest
> > > options.
> > 
> > What's wrong with making the variable a tristate, and move your
> > set-to-false after the parsing loop? If the user said "false" for the
> > option, then they apparently don't care much about the output.
> 
> Oh, there is nothing wrong with that. It just I didn't think about that
> solution while thinking about this problem.
> 
> > 
> > But yes, what you suggest is an option as well.
> 
> Yeah, I already got code for it.

After fixing the bugs in my code it has become too long for my liking so
I will turn to use tristate instead. :p

Wei.

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