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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- 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