Re: [Xen-devel] [RFC PATCH 04/12] hvmloader: add ACPI enabling for Q35

[Alexey G <x1917x@xxxxxxxxx>]

On Mon, 19 Mar 2018 13:01:58 +0000
Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

>On Tue, Mar 13, 2018 at 04:33:49AM +1000, Alexey Gerasimenko wrote:
>> In order to turn on ACPI for OS, we need to write a chipset-specific
>> value to SMI_CMD register (sort of imitation of the APM->ACPI switch
>> on real systems). Modify acpi_enable_sci() function to support both
>> i440 and Q35 emulation.
>> 
>> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
>> ---
>>  tools/firmware/hvmloader/hvmloader.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/firmware/hvmloader/hvmloader.c
>> b/tools/firmware/hvmloader/hvmloader.c index f603f68ded..070698440e
>> 100644 --- a/tools/firmware/hvmloader/hvmloader.c
>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> @@ -257,9 +257,16 @@ static const struct bios_config
>> *detect_bios(void) static void acpi_enable_sci(void)
>>  {
>>      uint8_t pm1a_cnt_val;
>> +    uint8_t acpi_enable_val;
>>  
>> -#define PIIX4_SMI_CMD_IOPORT 0xb2
>> +#define SMI_CMD_IOPORT       0xb2
>>  #define PIIX4_ACPI_ENABLE    0xf1
>> +#define ICH9_ACPI_ENABLE     0x02
>> +
>> +    if (get_pc_machine_type() == MACHINE_TYPE_Q35)
>> +        acpi_enable_val = ICH9_ACPI_ENABLE;
>> +    else
>> +        acpi_enable_val = PIIX4_ACPI_ENABLE;  
>
>Coding style, but I would rather:
>
>switch ( get_pc_machine_type() )
>{
>case MACHINE_TYPE_Q35:
>...
>case MACHINE_TYPE_I440:
>...
>default:
>BUG();
>}

Agree, a better code maintainability.

>I think storing the machine type in a global variable is better than
>calling get_pc_machine_type each time.

OK, will switch to it.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel