Re: [Xen-devel] [PATCH][v4] xen: Pass the location of the ACPI RSDP to DOM0.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.14 at 18:15, Philip Wernersbach <philip.wernersbach@xxxxxxxxx> 
>>> wrote:
> xen: [v4] 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.

As said before - the description reads as if Xen did something wrong
here. I think I explained in enough detail why this _has_ to be that
way. Hence this description is at least misleading, and thus not
acceptable.

> --- a/xen/arch/x86/acpi/boot.c
> +++ b/xen/arch/x86/acpi/boot.c
> @@ -57,6 +57,7 @@ bool_t __initdata acpi_lapic;
>  bool_t __initdata acpi_ioapic;
> 
>  bool_t acpi_skip_timer_override __initdata;
> +bool_t acpi_rsdp_passthrough    __initdata;

I see absolutely no reason why this option can't be contained to
setup.c - (static, not declaration in any header).

And you surely don't need the multiple successive blanks.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -75,6 +75,11 @@ custom_param("acpi", parse_acpi_param);
>  boolean_param("acpi_skip_timer_override", acpi_skip_timer_override);
> 
>  /* **** Linux config option: propagated to domain0. */
> +/* acpi_rsdp_passthrough: Explicitly pass the ACPI RSDP pointer to */
> +/*                        domain0 via the acpi_rsdp option.        */
> +boolean_param("acpi_rsdp_passthrough", acpi_rsdp_passthrough);
> +
> +/* **** Linux config option: propagated to domain0. */
>  /* noapic: Disable IOAPIC setup. */
>  boolean_param("noapic", skip_ioapic_setup);
> 
> @@ -1378,6 +1383,26 @@ void __init __start_xen(unsigned long mbi_p)
>              safe_strcat(dom0_cmdline, " acpi=");
>              safe_strcat(dom0_cmdline, acpi_param);
>          }
> +        if ( efi_enabled && acpi_rsdp_passthrough &&
> +             !strstr(dom0_cmdline, "acpi_rsdp=") )
> +        {
> +            acpi_physical_address rp = acpi_os_get_root_pointer();
> +            char rp_str[sizeof(acpi_physical_address)*2 + 3];
> +
> +            if ( rp )
> +            {

If you already restrict the scopes of the newly added variables
(which I appreciate), please move the declaration of rp_str here.

> +                snprintf(rp_str, sizeof(acpi_physical_address)*2 + 3,
> +                         "%p", (void *)rp);

Both the use of %p and the cast to void * seem pretty bogus. I
don't recall earlier reviews having requested you to do so...

Jan


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