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

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 26/07/17 08:31, Roger Pau Monné wrote:
> 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?
> 

It's already there (see libxl__domain_build_info_setdefault()).

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

Makes sense.

>>  /*
>>   * 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.
> 

This is intentional - I want to preserve the original behavior for
compatibility with other toolstacks.

Igor

> Roger.
> 

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