[PATCH] x86: assert MBI is large enough in pvh-boot.c

[PATCH] x86: assert MBI is large enough in pvh-boot.c

[Wei Liu]

The relocation code in __start_xen requires one extra element in the
MBI structure. By the looks of it the temporary MBI array is already
large enough. Add an assertion to catch any issue in the future.

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

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 0e9e5bfdf6..7e13e7604b 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -44,6 +44,13 @@ static void __init convert_pvh_info(void)
 
     ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
 
+    /*
+     * Temporary MBI 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().
+     */
+    ASSERT(ARRAY_SIZE(pvh_mbi_mods) > pvh_info->nr_modules);
+
     /*
      * Turn hvm_start_info into mbi. Luckily all modules are placed under 4GB
      * boundary on x86.
-- 
2.11.0


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

Re: [PATCH] x86: assert MBI is large enough in pvh-boot.c

[Andrew Cooper]

On 26/09/18 12:00, Wei Liu wrote:
> The relocation code in __start_xen requires one extra element in the
> MBI structure. By the looks of it the temporary MBI array is already
> large enough. Add an assertion to catch any issue in the future.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

While this is all well and good, the ASSERT() never actually gets out
onto the console.  This is because, when a failure occurs, the console
hasn't been configured yet.

For development purposes I use:

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index e48039d..64febfa 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -91,7 +91,7 @@ static uint32_t conringc, conringp;
 static int __read_mostly sercon_handle = -1;
 
 #ifdef CONFIG_X86
-static bool __read_mostly opt_console_xen; /* console=xen */
+static bool __read_mostly opt_console_xen = true; /* console=xen */
 #endif
 
 static DEFINE_SPINLOCK(console_lock);

which gets the details out into the L0 log.

As an addendum, might it be worth having opt_console_xen be tristate,
and enabled by the start of the PVH path, so log messages before the
command line is parsed end up being emitted?

~Andrew

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

Re: [PATCH] x86: assert MBI is large enough in pvh-boot.c

[Jan Beulich]

>>> On 26.09.18 at 13:00, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/guest/pvh-boot.c
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -44,6 +44,13 @@ static void __init convert_pvh_info(void)
>  
>      ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
>  
> +    /*
> +     * Temporary MBI 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().
> +     */
> +    ASSERT(ARRAY_SIZE(pvh_mbi_mods) > pvh_info->nr_modules);

Are ASSERT()s (also the other one in context) actually the right thing
here? I think we'd better panic(): That'll also cover release builds and
is imo more appropriate for data coming from the outside.

Jan



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

Re: [PATCH] x86: assert MBI is large enough in pvh-boot.c

[Wei Liu]

On Wed, Sep 26, 2018 at 12:05:05PM +0100, Andrew Cooper wrote:
> On 26/09/18 12:00, Wei Liu wrote:
> > The relocation code in __start_xen requires one extra element in the
> > MBI structure. By the looks of it the temporary MBI array is already
> > large enough. Add an assertion to catch any issue in the future.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> While this is all well and good, the ASSERT() never actually gets out
> onto the console.  This is because, when a failure occurs, the console
> hasn't been configured yet.
> 
> For development purposes I use:
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index e48039d..64febfa 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -91,7 +91,7 @@ static uint32_t conringc, conringp;
>  static int __read_mostly sercon_handle = -1;
>  
>  #ifdef CONFIG_X86
> -static bool __read_mostly opt_console_xen; /* console=xen */
> +static bool __read_mostly opt_console_xen = true; /* console=xen */
>  #endif
>  
>  static DEFINE_SPINLOCK(console_lock);
> 
> which gets the details out into the L0 log.
> 
> As an addendum, might it be worth having opt_console_xen be tristate,
> and enabled by the start of the PVH path, so log messages before the
> command line is parsed end up being emitted?

This is fine by me.

Wei.

> 
> ~Andrew

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

Re: [PATCH] x86: assert MBI is large enough in pvh-boot.c

[Wei Liu]

On Wed, Sep 26, 2018 at 06:07:30AM -0600, Jan Beulich wrote:
> >>> On 26.09.18 at 13:00, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/guest/pvh-boot.c
> > +++ b/xen/arch/x86/guest/pvh-boot.c
> > @@ -44,6 +44,13 @@ static void __init convert_pvh_info(void)
> >  
> >      ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
> >  
> > +    /*
> > +     * Temporary MBI 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().
> > +     */
> > +    ASSERT(ARRAY_SIZE(pvh_mbi_mods) > pvh_info->nr_modules);
> 
> Are ASSERT()s (also the other one in context) actually the right thing
> here? I think we'd better panic(): That'll also cover release builds and
> is imo more appropriate for data coming from the outside.

Okay.

Wei.

> 
> Jan
> 
> 

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