Re: [Xen-devel] [PATCH v2] hvmloader, libxl: use the correct ACPI settings depending on device model

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 25, 2017 at 08:55:30PM +0100, Igor Druzhinin wrote:
> We need to choose ACPI tables and ACPI IO port location
> properly depending on the device model version we are running.
> Previously, this decision was made by BIOS type specific
> code in hvmloader, e.g. always load QEMU traditional specific
> tables if it's ROMBIOS and always load QEMU Xen specific
> tables if it's SeaBIOS.
> 
> This change saves this behavior but adds an additional way
> (xenstore key) to specify the correct device model if we
> happen to run a non-default one. Toolstack bit makes use of it.

Should there also be a change to libxl to allow selecting rombios
with qemu-xen or seabios with qemu-trad?

> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Changes in v2:
> * fix insufficient allocation size of localent
> ---
>  tools/firmware/hvmloader/hvmloader.c |  2 --
>  tools/firmware/hvmloader/ovmf.c      |  2 ++
>  tools/firmware/hvmloader/rombios.c   |  2 ++
>  tools/firmware/hvmloader/seabios.c   |  3 +++
>  tools/firmware/hvmloader/util.c      | 24 ++++++++++++++++++++++++
>  tools/libxl/libxl_create.c           |  4 +++-
>  6 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/hvmloader.c 
> b/tools/firmware/hvmloader/hvmloader.c
> index f603f68..db11ab1 100644
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -405,8 +405,6 @@ int main(void)
>          }
>  
>          acpi_enable_sci();
> -
> -        hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
>      }
>  
>      init_vm86_tss();
> diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
> index 4ff7f1d..ebadc64 100644
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -127,6 +127,8 @@ static void ovmf_acpi_build_tables(void)
>          .dsdt_15cpu_len = 0
>      };
>  
> +    hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);

This 1/0 seems very opaque, we should have a proper define for it in
param.h (not that you should fix it).

> +
>      hvmloader_acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS);
>  }
>  
> diff --git a/tools/firmware/hvmloader/rombios.c 
> b/tools/firmware/hvmloader/rombios.c
> index 56b39b7..31a7c65 100644
> --- a/tools/firmware/hvmloader/rombios.c
> +++ b/tools/firmware/hvmloader/rombios.c
> @@ -181,6 +181,8 @@ static void rombios_acpi_build_tables(void)
>          .dsdt_15cpu_len = dsdt_15cpu_len,
>      };
>  
> +    hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 0);
> +
>      hvmloader_acpi_build_tables(&config, ACPI_PHYSICAL_ADDRESS);
>  }
>  
> diff --git a/tools/firmware/hvmloader/seabios.c 
> b/tools/firmware/hvmloader/seabios.c
> index 870576a..5878eff 100644
> --- a/tools/firmware/hvmloader/seabios.c
> +++ b/tools/firmware/hvmloader/seabios.c
> @@ -28,6 +28,7 @@
>  
>  #include <acpi2_0.h>
>  #include <libacpi.h>
> +#include <xen/hvm/params.h>
>  
>  extern unsigned char dsdt_anycpu_qemu_xen[];
>  extern int dsdt_anycpu_qemu_xen_len;
> @@ -99,6 +100,8 @@ static void seabios_acpi_build_tables(void)
>          .dsdt_15cpu_len = 0,
>      };
>  
> +    hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> +
>      hvmloader_acpi_build_tables(&config, rsdp);
>      add_table(rsdp);
>  }
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index db5f240..45b777c 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -31,6 +31,9 @@
>  #include <xen/hvm/hvm_xs_strings.h>
>  #include <xen/hvm/params.h>
>  
> +extern unsigned char dsdt_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[];
> +extern int dsdt_anycpu_qemu_xen_len, dsdt_anycpu_len, dsdt_15cpu_len;

Part of those extern declarations are now present in ovmf.c,
seabios.c, rombios.c and now also util.c, maybe it would make sense to
just declare them in util.h?

>  /*
>   * Check whether there exists overlap in the specified memory range.
>   * Returns true if exists, else returns false.
> @@ -897,6 +900,27 @@ void hvmloader_acpi_build_tables(struct acpi_config 
> *config,
>      /* Allocate and initialise the acpi info area. */
>      mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1);
>  
> +    /* If the device model is specified switch to the corresponding tables */
> +    s = xenstore_read("platform/device-model", "");
> +    if ( !strncmp(s, "qemu_xen_traditional", 21) )
> +    {
> +        config->dsdt_anycpu = dsdt_anycpu;
> +        config->dsdt_anycpu_len = dsdt_anycpu_len;
> +        config->dsdt_15cpu = dsdt_15cpu;
> +        config->dsdt_15cpu_len = dsdt_15cpu_len;
> +
> +        hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 0);
> +    }
> +    else if ( !strncmp(s, "qemu_xen", 9) )
> +    {
> +        config->dsdt_anycpu = dsdt_anycpu_qemu_xen;
> +        config->dsdt_anycpu_len = dsdt_anycpu_qemu_xen_len;
> +        config->dsdt_15cpu = NULL;
> +        config->dsdt_15cpu_len = 0;
> +
> +        hvm_param_set(HVM_PARAM_ACPI_IOPORTS_LOCATION, 1);
> +    }

Does it still make sense to set the tables in
{ovmf/seabios/rombios}_acpi_build_tables?

It seems like it's going to be overwritten here in any case because
the toolstack always writes the "platform/device-model" node.

Maybe it would be better to just panic if the node is not set, and
remove {ovmf/seabios/rombios}_acpi_build_tables.

Roger.

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