[PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Philip Wernersbach]

[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]

xen: [v2] Pass the location of the ACPI RSDP to DOM0.

Some machines, such as recent IBM servers, only allow the OS to get the
ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
cannot get the RSDP on these machines, leading to all sorts of
functionality reductions.

Signed-off-by: Philip Wernersbach <philip.wernersbach@gmail.com>

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b49256d..8c307c9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1378,6 +1378,25 @@ void __init __start_xen(unsigned long mbi_p)
             safe_strcat(dom0_cmdline, " acpi=");
             safe_strcat(dom0_cmdline, acpi_param);
         }
+        if ( !strstr(dom0_cmdline, "acpi_rsdp=") )
+        {
+            char acpi_os_root_pointer_string[MAX_RSDP_ADDRESS_STRING];
+            int acpi_os_root_pointer_string_size =
snprintf(acpi_os_root_pointer_string,
+
MAX_RSDP_ADDRESS_STRING, "%08lX",
+
acpi_os_get_root_pointer());
+
+            if ( (acpi_os_root_pointer_string_size > 0) &&
(acpi_os_root_pointer_string_size <= MAX_RSDP_ADDRESS_STRING) )
+            {
+                safe_strcat(dom0_cmdline, " acpi_rsdp=0x");
+                safe_strcat(dom0_cmdline, acpi_os_root_pointer_string);
+            }
+            else
+            {
+                printk(XENLOG_WARNING "RSDP Address String Size Check
Failed!\n");
+                printk(XENLOG_WARNING "Not passing acpi_rsdp to Dom0!\n");
+                printk(XENLOG_WARNING "(Expected size 1-%d, got
%d)\n", MAX_RSDP_ADDRESS_STRING, acpi_os_root_pointer_string_size);
+            }
+        }

         cmdline = dom0_cmdline;
     }
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 8c5697e..a7c3905 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -750,6 +750,7 @@ struct start_info {
                                 /*  SIF_MOD_START_PFN set in flags).      */
     unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
 #define MAX_GUEST_CMDLINE 1024
+#define MAX_RSDP_ADDRESS_STRING 21
     int8_t cmd_line[MAX_GUEST_CMDLINE];
     /* The pfn range here covers both page table and p->m table frames.   */
     unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */

[-- Attachment #2: xen-master-pass-acpi-rsdp.patch --]
[-- Type: text/x-patch, Size: 2397 bytes --]

xen: [v2] Pass the location of the ACPI RSDP to DOM0.
 
Some machines, such as recent IBM servers, only allow the OS to get the
ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
cannot get the RSDP on these machines, leading to all sorts of
functionality reductions.
 
Signed-off-by: Philip Wernersbach <philip.wernersbach@gmail.com>

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b49256d..8c307c9 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1378,6 +1378,25 @@ void __init __start_xen(unsigned long mbi_p)
             safe_strcat(dom0_cmdline, " acpi=");
             safe_strcat(dom0_cmdline, acpi_param);
         }
+        if ( !strstr(dom0_cmdline, "acpi_rsdp=") )
+        {
+            char acpi_os_root_pointer_string[MAX_RSDP_ADDRESS_STRING];
+            int acpi_os_root_pointer_string_size = snprintf(acpi_os_root_pointer_string, 
+                                                            MAX_RSDP_ADDRESS_STRING, "%08lX", 
+                                                            acpi_os_get_root_pointer());
+
+            if ( (acpi_os_root_pointer_string_size > 0) && (acpi_os_root_pointer_string_size <= MAX_RSDP_ADDRESS_STRING) )
+            {   
+                safe_strcat(dom0_cmdline, " acpi_rsdp=0x");
+                safe_strcat(dom0_cmdline, acpi_os_root_pointer_string);
+            } 
+            else 
+            {
+                printk(XENLOG_WARNING "RSDP Address String Size Check Failed!\n");
+                printk(XENLOG_WARNING "Not passing acpi_rsdp to Dom0!\n");
+                printk(XENLOG_WARNING "(Expected size 1-%d, got %d)\n", MAX_RSDP_ADDRESS_STRING, acpi_os_root_pointer_string_size);
+            }
+        }
 
         cmdline = dom0_cmdline;
     }
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 8c5697e..a7c3905 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -750,6 +750,7 @@ struct start_info {
                                 /*  SIF_MOD_START_PFN set in flags).      */
     unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
 #define MAX_GUEST_CMDLINE 1024
+#define MAX_RSDP_ADDRESS_STRING 21
     int8_t cmd_line[MAX_GUEST_CMDLINE];
     /* The pfn range here covers both page table and p->m table frames.   */
     unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3467 bytes --]

On 20/01/2014 22:08, Philip Wernersbach wrote:
> xen: [v2] Pass the location of the ACPI RSDP to DOM0.
>
> Some machines, such as recent IBM servers, only allow the OS to get the
> ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
> cannot get the RSDP on these machines, leading to all sorts of
> functionality reductions.
>
> Signed-off-by: Philip Wernersbach <philip.wernersbach@gmail.com>
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b49256d..8c307c9 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1378,6 +1378,25 @@ void __init __start_xen(unsigned long mbi_p)
>              safe_strcat(dom0_cmdline, " acpi=");
>              safe_strcat(dom0_cmdline, acpi_param);
>          }

Please read the CODING_STYLE document in the SCM root.

In particular, there should be a newline here, and lines should (where
possible) be wrapped at ~72-80 chars

> +        if ( !strstr(dom0_cmdline, "acpi_rsdp=") )
> +        {
> +            char acpi_os_root_pointer_string[MAX_RSDP_ADDRESS_STRING];

char rp_str[sizeof(unsigned long)*2 + 1] would suffice with a much
shorter name, and do without an extra define.

> +            int acpi_os_root_pointer_string_size =

This return value from snprintf is bogus when used in the if() condition
later; It is unconditionally within the provided bounds.  Furthermore,
snprintf() is not going to fail for a single hex long into an
appropriately sized buffer.

Furthermore, to correctly check for a failure of
acpi_os_get_root_pointer(), you would be much better with something such as:

acpi_physical_address rsdp = acpi_os_get_root_pointer();

if ( rsdp )
{
  ... append
}
else
{
  ... error
}

> snprintf(acpi_os_root_pointer_string,
> +
> MAX_RSDP_ADDRESS_STRING, "%08lX",
> +
> acpi_os_get_root_pointer());
> +
> +            if ( (acpi_os_root_pointer_string_size > 0) &&
> (acpi_os_root_pointer_string_size <= MAX_RSDP_ADDRESS_STRING) )
> +            {
> +                safe_strcat(dom0_cmdline, " acpi_rsdp=0x");
> +                safe_strcat(dom0_cmdline, acpi_os_root_pointer_string);
> +            }
> +            else
> +            {
> +                printk(XENLOG_WARNING "RSDP Address String Size Check
> Failed!\n");
> +                printk(XENLOG_WARNING "Not passing acpi_rsdp to Dom0!\n");
> +                printk(XENLOG_WARNING "(Expected size 1-%d, got
> %d)\n", MAX_RSDP_ADDRESS_STRING, acpi_os_root_pointer_string_size);

And talking of errors, this is overkill.  A simple:

printk(XENLOG_WARNING "Failed to get acpi_rsdp to pass to dom0\n");

should suffice.

~Andrew

> +            }
> +        }
>
>          cmdline = dom0_cmdline;
>      }
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 8c5697e..a7c3905 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -750,6 +750,7 @@ struct start_info {
>                                  /*  SIF_MOD_START_PFN set in flags).      */
>      unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
>  #define MAX_GUEST_CMDLINE 1024
> +#define MAX_RSDP_ADDRESS_STRING 21
>      int8_t cmd_line[MAX_GUEST_CMDLINE];
>      /* The pfn range here covers both page table and p->m table frames.   */
>      unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 5037 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Jan Beulich]

>>> On 20.01.14 at 23:08, Philip Wernersbach <philip.wernersbach@gmail.com> wrote:
> xen: [v2] Pass the location of the ACPI RSDP to DOM0.
> 
> Some machines, such as recent IBM servers, only allow the OS to get the
> ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0

This reads as if this was a bug in Xen, which it isn't. Dom0's
lack of EFI support when running on top of Xen is the issue.

> cannot get the RSDP on these machines, leading to all sorts of
> functionality reductions.
> 
> Signed-off-by: Philip Wernersbach <philip.wernersbach@gmail.com>
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b49256d..8c307c9 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1378,6 +1378,25 @@ void __init __start_xen(unsigned long mbi_p)
>              safe_strcat(dom0_cmdline, " acpi=");
>              safe_strcat(dom0_cmdline, acpi_param);
>          }
> +        if ( !strstr(dom0_cmdline, "acpi_rsdp=") )

I think I had indicated my opposition to this sort of hack on v1
already; I'm not sure I asked which OSes usable as Dom0 but
other than Linux recognize this option. Or which versions of
Linux actually do (I'm pretty sure older ones don't).

Bottom line - I continue to think that the issue should be fixed
in Linux.

> +        {
> +            char acpi_os_root_pointer_string[MAX_RSDP_ADDRESS_STRING];
> +            int acpi_os_root_pointer_string_size =
> snprintf(acpi_os_root_pointer_string,
> +
> MAX_RSDP_ADDRESS_STRING, "%08lX",
> +
> acpi_os_get_root_pointer());
> +
> +            if ( (acpi_os_root_pointer_string_size > 0) &&
> (acpi_os_root_pointer_string_size <= MAX_RSDP_ADDRESS_STRING) )
> +            {
> +                safe_strcat(dom0_cmdline, " acpi_rsdp=0x");
> +                safe_strcat(dom0_cmdline, acpi_os_root_pointer_string);
> +            }
> +            else
> +            {
> +                printk(XENLOG_WARNING "RSDP Address String Size Check
> Failed!\n");
> +                printk(XENLOG_WARNING "Not passing acpi_rsdp to Dom0!\n");
> +                printk(XENLOG_WARNING "(Expected size 1-%d, got
> %d)\n", MAX_RSDP_ADDRESS_STRING, acpi_os_root_pointer_string_size);
> +            }
> +        }
> 
>          cmdline = dom0_cmdline;
>      }
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index 8c5697e..a7c3905 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -750,6 +750,7 @@ struct start_info {
>                                  /*  SIF_MOD_START_PFN set in flags).      
> */
>      unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
>  #define MAX_GUEST_CMDLINE 1024
> +#define MAX_RSDP_ADDRESS_STRING 21

This clearly doesn't belong here (public header). In fact it doesn't
belong into any header at all, as it's being used only in a single
source file.

Jan

>      int8_t cmd_line[MAX_GUEST_CMDLINE];
>      /* The pfn range here covers both page table and p->m table frames.   */
>      unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Philip Wernersbach]

On Tue, Jan 21, 2014 at 4:51 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 20.01.14 at 23:08, Philip Wernersbach <philip.wernersbach@gmail.com> wrote:
>> xen: [v2] Pass the location of the ACPI RSDP to DOM0.
>>
>> Some machines, such as recent IBM servers, only allow the OS to get the
>> ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
>
> This reads as if this was a bug in Xen, which it isn't. Dom0's
> lack of EFI support when running on top of Xen is the issue.

It all depends on how you look at it, as there's two ways to fix this.
Linux currently supports EFI just fine. Xen nukes DOM0's ability to
access EFI using the current methods, which causes Linux's existing
EFI support to fail. This would be Xen's fault. If Xen currently
exposes EFI to DOM0 in some other way that Linux is not aware of, then
this would be Linux's fault. Either way, we can either choose to fix
Xen or fix Linux.

> I think I had indicated my opposition to this sort of hack on v1
> already; I'm not sure I asked which OSes usable as Dom0 but
> other than Linux recognize this option. Or which versions of
> Linux actually do (I'm pretty sure older ones don't).
>
> Bottom line - I continue to think that the issue should be fixed
> in Linux.

The method that this patch uses is a valid way to fix this. In the
best case, the DOM0 OS recognizes this option and uses it. In the
worst case, the DOM0 OS won't recognize the option and will ignore it,
so we're no worse off.

I agree with you that this is more of a stop gap than a long term fix.
The final solution is to fully expose EFI services to the DOM0 in some
way. However, getting there will take some time. The reason this patch
came about is that the company I work for bought new IBM servers in
the hope of migrating our existing Xen server farm to the new IBM
servers. But we soon found out that DOM0 couldn't find the ACPI RSDP
pointer on the new IBM servers, which means that Xen is dead in the
water on these machines for now. The final solution of exposing EFI to
the DOM0 is the ultimate goal, but for now businesses need an
immediate solution. This provides an acceptable solution until DOM0
EFI services are implemented at a later date. There is no reason why
this can't be merged now and then removed when DOM0 EFI service
support arrives.

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Konrad Rzeszutek Wilk]

On Tue, Jan 21, 2014 at 04:19:40PM -0500, Philip Wernersbach wrote:
> On Tue, Jan 21, 2014 at 4:51 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>> On 20.01.14 at 23:08, Philip Wernersbach <philip.wernersbach@gmail.com> wrote:
> >> xen: [v2] Pass the location of the ACPI RSDP to DOM0.
> >>
> >> Some machines, such as recent IBM servers, only allow the OS to get the
> >> ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
> >
> > This reads as if this was a bug in Xen, which it isn't. Dom0's
> > lack of EFI support when running on top of Xen is the issue.
> 
> It all depends on how you look at it, as there's two ways to fix this.
> Linux currently supports EFI just fine. Xen nukes DOM0's ability to
> access EFI using the current methods, which causes Linux's existing
> EFI support to fail. This would be Xen's fault. If Xen currently
> exposes EFI to DOM0 in some other way that Linux is not aware of, then
> this would be Linux's fault. Either way, we can either choose to fix
> Xen or fix Linux.
> 
> > I think I had indicated my opposition to this sort of hack on v1
> > already; I'm not sure I asked which OSes usable as Dom0 but
> > other than Linux recognize this option. Or which versions of
> > Linux actually do (I'm pretty sure older ones don't).
> >
> > Bottom line - I continue to think that the issue should be fixed
> > in Linux.
> 
> The method that this patch uses is a valid way to fix this. In the
> best case, the DOM0 OS recognizes this option and uses it. In the
> worst case, the DOM0 OS won't recognize the option and will ignore it,
> so we're no worse off.
> 
> I agree with you that this is more of a stop gap than a long term fix.
> The final solution is to fully expose EFI services to the DOM0 in some
> way. However, getting there will take some time. The reason this patch
> came about is that the company I work for bought new IBM servers in
> the hope of migrating our existing Xen server farm to the new IBM
> servers. But we soon found out that DOM0 couldn't find the ACPI RSDP
> pointer on the new IBM servers, which means that Xen is dead in the
> water on these machines for now. The final solution of exposing EFI to
> the DOM0 is the ultimate goal, but for now businesses need an
> immediate solution. This provides an acceptable solution until DOM0
> EFI services are implemented at a later date. There is no reason why
> this can't be merged now and then removed when DOM0 EFI service
> support arrives.

Could you use the Linux patches that use Xen's EFI services? Ie,
the DOM0 EFI ones?

They were posted on the mailing list some time ago (last year, November-ish?)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Philip Wernersbach]

On Tue, Jan 21, 2014 at 4:31 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> Could you use the Linux patches that use Xen's EFI services? Ie,
> the DOM0 EFI ones?
>
> They were posted on the mailing list some time ago (last year, November-ish?)

That is an option, but I can't find the patches (a thorough search of
Google yields nothing). The other issue is that unless those were
merged into the kernel by version 3.12.8, we would have to roll our
own kernel (3.12.8 is the newest kernel that Debian supplies).

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Konrad Rzeszutek Wilk]

On Tue, Jan 21, 2014 at 05:09:10PM -0500, Philip Wernersbach wrote:
> On Tue, Jan 21, 2014 at 4:31 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > Could you use the Linux patches that use Xen's EFI services? Ie,
> > the DOM0 EFI ones?
> >
> > They were posted on the mailing list some time ago (last year, November-ish?)
> 
> That is an option, but I can't find the patches (a thorough search of
> Google yields nothing). The other issue is that unless those were

http://www.gossamer-threads.com/lists/xen/devel/294362

is my response.

> merged into the kernel by version 3.12.8, we would have to roll our
> own kernel (3.12.8 is the newest kernel that Debian supplies).

Right, but you are rolling your own hypervisor. With this you
wouldn't need to roll your hypervisor but instead can just do
the kernel.

Re: [PATCH][v2] xen: Pass the location of the ACPI RSDP to DOM0.

[Jan Beulich]

>>> On 21.01.14 at 22:19, Philip Wernersbach <philip.wernersbach@gmail.com> wrote:
> On Tue, Jan 21, 2014 at 4:51 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 20.01.14 at 23:08, Philip Wernersbach <philip.wernersbach@gmail.com> wrote:
>>> xen: [v2] Pass the location of the ACPI RSDP to DOM0.
>>>
>>> Some machines, such as recent IBM servers, only allow the OS to get the
>>> ACPI RSDP from EFI. Since Xen nukes DOM0's ability to access EFI, DOM0
>>
>> This reads as if this was a bug in Xen, which it isn't. Dom0's
>> lack of EFI support when running on top of Xen is the issue.
> 
> It all depends on how you look at it, as there's two ways to fix this.
> Linux currently supports EFI just fine. Xen nukes DOM0's ability to
> access EFI using the current methods, which causes Linux's existing
> EFI support to fail. This would be Xen's fault. If Xen currently
> exposes EFI to DOM0 in some other way that Linux is not aware of, then
> this would be Linux's fault. Either way, we can either choose to fix
> Xen or fix Linux.

Which implies you don't understand the fundamental concepts of
Xen PV guests and/or EFI and/or the implications thereof on the
interaction of the two: PV guests (including Dom0) aren't fully
privileged (they don't run in ring 0), and hence _can't_ be allowed
direct access to EFI services and data. Such accesses _have to_
go through the hypervisor.

>> I think I had indicated my opposition to this sort of hack on v1
>> already; I'm not sure I asked which OSes usable as Dom0 but
>> other than Linux recognize this option. Or which versions of
>> Linux actually do (I'm pretty sure older ones don't).
>>
>> Bottom line - I continue to think that the issue should be fixed
>> in Linux.
> 
> The method that this patch uses is a valid way to fix this. In the
> best case, the DOM0 OS recognizes this option and uses it. In the
> worst case, the DOM0 OS won't recognize the option and will ignore it,
> so we're no worse off.

OSes may also choose to not ignore but choke on unknown options
passed to them.

> I agree with you that this is more of a stop gap than a long term fix.
> The final solution is to fully expose EFI services to the DOM0 in some
> way. However, getting there will take some time. The reason this patch
> came about is that the company I work for bought new IBM servers in
> the hope of migrating our existing Xen server farm to the new IBM
> servers. But we soon found out that DOM0 couldn't find the ACPI RSDP
> pointer on the new IBM servers, which means that Xen is dead in the
> water on these machines for now. The final solution of exposing EFI to
> the DOM0 is the ultimate goal, but for now businesses need an
> immediate solution. This provides an acceptable solution until DOM0
> EFI services are implemented at a later date. There is no reason why
> this can't be merged now and then removed when DOM0 EFI service
> support arrives.

One of the reasons I'm not in agreement with this view is that this
change papers over one of the issues but completely ignores others
(like the DMI tables likely also not being found on such systems by
the Dom0 OS, or like there not necessarily being an RTC available;
there are more things here - just go check what data can be
retrieved from Xen, and think about how the OS would get at that
data without proper EFI support implemented). I hope you agree
that it's unreasonable to work around all of these via command line
options. And I hope you understand that by not working around
them, you may end up with an apparently working system that in
the end breaks long after boot rather than right at boot (likely
leading to data loss, which I personally view as much worse than
not being able to boot on a certain system).

Jan