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

2018-03-19 Thread Roger Pau Monné
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 
> ---
>  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_ENABLE0xf1
> +#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();
}

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

Thanks, Roger.

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

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

2018-03-13 Thread Wei Liu
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 
> ---
>  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_ENABLE0xf1
> +#define ICH9_ACPI_ENABLE 0x02
> +
> +if (get_pc_machine_type() == MACHINE_TYPE_Q35)

Coding style.

And the previous patch can be folded into this one.

Wei.

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