diff mbox series

[XTF,4/4] setup: Setup PV console for HVM guests on xen >4.2

Message ID 20200416094141.65120-5-wipawel@amazon.de (mailing list archive)
State Superseded
Headers show
Series Small fixes and improvements | expand

Commit Message

Wieczorkiewicz, Pawel April 16, 2020, 9:41 a.m. UTC 
From: Paul Semel <phentex@amazon.de>

Xen 4.2 requires a workaround that does not setup PV console
for HVM guests. However, newer Xen versions do not have that
limitation and should always have the PV console setup.

In arch_setup() detects Xen version by issuing xen_version hypercall
and optionally passes the version to main_xtf().

Signed-off-by: Paul Semel <phentex@amazon.de>
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
---
 arch/x86/setup.c        | 20 ++++++++++++++++++--
 common/setup.c          |  6 +++++-
 include/xtf/framework.h |  2 +-
 3 files changed, 24 insertions(+), 4 deletions(-)

Comments

Andrew Cooper April 16, 2020, 10:36 a.m. UTC | #1 
On 16/04/2020 10:41, Pawel Wieczorkiewicz wrote:
> @@ -272,9 +274,23 @@ void arch_setup(void)
>  
>      init_hypercalls();
>  
> -    if ( !is_initdomain() )
> +    xen_version = hypercall_xen_version(XENVER_version, NULL);
> +    if ( version )
> +        *version = xen_version;
> +
> +    /*
> +     * The setup_pv_console function registers a writing function
> +     * that makes hvm guests crash on Xen 4.2

This comment in particular is rather concerning.  Even if there is a
configuration issue in 4.2 stopping the PV console from being wired up
properly, nothing involved in transmitting on the console should crash
the guest.

~Andrew
Wieczorkiewicz, Pawel April 16, 2020, 11:51 a.m. UTC | #2 
> On 16. Apr 2020, at 12:36, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 16/04/2020 10:41, Pawel Wieczorkiewicz wrote:
>> @@ -272,9 +274,23 @@ void arch_setup(void)
>> 
>>     init_hypercalls();
>> 
>> -    if ( !is_initdomain() )
>> +    xen_version = hypercall_xen_version(XENVER_version, NULL);
>> +    if ( version )
>> +        *version = xen_version;
>> +
>> +    /*
>> +     * The setup_pv_console function registers a writing function
>> +     * that makes hvm guests crash on Xen 4.2
> 
> This comment in particular is rather concerning.  Even if there is a
> configuration issue in 4.2 stopping the PV console from being wired up
> properly, nothing involved in transmitting on the console should crash
> the guest.
> 

I am again a little short on details here. Maybe Paul could chime in.
But, I vagualy remember it was something about the init_pv_console()’s:

    if ( port >= (sizeof(shared_info.evtchn_pending) * CHAR_BIT) )
        panic("evtchn %u out of evtchn_pending[] range\n", port);

> ~Andrew


Best Regards,
Pawel Wieczorkiewicz
wipawel@amazon.com



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
Andrew Cooper April 20, 2020, 1:43 p.m. UTC | #3 
On 16/04/2020 12:51, Wieczorkiewicz, Pawel wrote:
>> On 16. Apr 2020, at 12:36, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 16/04/2020 10:41, Pawel Wieczorkiewicz wrote:
>>> @@ -272,9 +274,23 @@ void arch_setup(void)
>>>
>>>     init_hypercalls();
>>>
>>> -    if ( !is_initdomain() )
>>> +    xen_version = hypercall_xen_version(XENVER_version, NULL);
>>> +    if ( version )
>>> +        *version = xen_version;
>>> +
>>> +    /*
>>> +     * The setup_pv_console function registers a writing function
>>> +     * that makes hvm guests crash on Xen 4.2
>> This comment in particular is rather concerning.  Even if there is a
>> configuration issue in 4.2 stopping the PV console from being wired up
>> properly, nothing involved in transmitting on the console should crash
>> the guest.
>>
> I am again a little short on details here. Maybe Paul could chime in.
> But, I vagualy remember it was something about the init_pv_console()’s:
>
>     if ( port >= (sizeof(shared_info.evtchn_pending) * CHAR_BIT) )
>         panic("evtchn %u out of evtchn_pending[] range\n", port);

This is a sanity check about not overrunning the evtchn_pending array. 
However, the check is still correct AFAICT.

port will either be 0 (if not configured by the toolstack), or strictly
less than 8 if configured properly.

What value are you seeing here?

~Andrew
diff mbox series

Patch

diff --git a/arch/x86/setup.c b/arch/x86/setup.c
index f6fa4df..e3f74e6 100644
--- a/arch/x86/setup.c
+++ b/arch/x86/setup.c
@@ -250,8 +250,10 @@  static void xen_console_write(const char *buf, size_t len)
     hypercall_console_write(buf, len);
 }
 
-void arch_setup(void)
+void arch_setup(int *version)
 {
+    int xen_version;
+
     if ( IS_DEFINED(CONFIG_HVM) && !pvh_start_info )
     {
         register_console_callback(qemu_console_write);
@@ -272,9 +274,23 @@  void arch_setup(void)
 
     init_hypercalls();
 
-    if ( !is_initdomain() )
+    xen_version = hypercall_xen_version(XENVER_version, NULL);
+    if ( version )
+        *version = xen_version;
+
+    /*
+     * The setup_pv_console function registers a writing function
+     * that makes hvm guests crash on Xen 4.2
+     */
+    if ( (!IS_DEFINED(CONFIG_HVM) ||
+         (XEN_MAJOR(xen_version) >= 4 && XEN_MINOR(xen_version) > 2)) &&
+         !is_initdomain() )
     {
         setup_pv_console();
+    }
+
+    if ( !is_initdomain() )
+    {
         setup_xenbus();
     }
 
diff --git a/common/setup.c b/common/setup.c
index 932fc09..1d3da15 100644
--- a/common/setup.c
+++ b/common/setup.c
@@ -19,9 +19,13 @@ 
  */
 void __noreturn xtf_main(void)
 {
-    arch_setup();
+    int xen_version;
+
+    arch_setup(&xen_version);
 
     printk("--- Xen Test Framework ---\n");
+    printk("Found Xen: %d.%d\n", XEN_MAJOR(xen_version),
+           XEN_MINOR(xen_version));
     printk("Environment: %s\n", environment_description);
     printk("%s\n", test_title);
 
diff --git a/include/xtf/framework.h b/include/xtf/framework.h
index a71bf39..6664733 100644
--- a/include/xtf/framework.h
+++ b/include/xtf/framework.h
@@ -2,7 +2,7 @@ 
 #define XTF_FRAMEWORK_H
 
 /* To be implemented by each arch */
-void arch_setup(void);
+void arch_setup(int *);
 void test_setup(void);
 
 /* Single line summary of execution environment. */