[Qemu-devel] [RFC] acpi: add reset register to fadt

[Qemu-devel] [RFC] acpi: add reset register to fadt

[Reza Jelveh]

Some operating systems such as FreeBSD and Mac OSX need the reset_register
section of the FADT filled to know which port to write to for a system reset.

What is the right way to set the reset_val and the reset addr in this case?
---
 hw/i386/acpi-build.c | 5 +++++
 hw/i386/acpi-defs.h  | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d0a5c85..21c1453 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -361,6 +361,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
                               (1 << ACPI_FADT_F_SLP_BUTTON) |
                               (1 << ACPI_FADT_F_RTC_S4));
     fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
+    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
+    fadt->reset_val = 0xf;
+    fadt->reset_reg.address_space_id   = aml_system_io;
+    fadt->reset_reg.register_bit_width = 8;
+    fadt->reset_reg.address            = ICH9_RST_CNT_IOPORT;
     /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
      * For more than 8 CPUs, "Clustered Logical" mode has to be used
      */
diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
index c4468f8..960c833 100644
--- a/hw/i386/acpi-defs.h
+++ b/hw/i386/acpi-defs.h
@@ -132,6 +132,8 @@ struct AcpiFadtDescriptorRev1
     uint8_t  reserved4a;             /* Reserved */
     uint8_t  reserved4b;             /* Reserved */
     uint32_t flags;
+    Acpi20GenericAddress reset_reg;
+    uint8_t reset_val;
 } QEMU_PACKED;
 typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
 
-- 
2.3.4

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Marcel Apfelbaum]

On 03/28/2015 05:46 PM, Reza Jelveh wrote:
> Some operating systems such as FreeBSD and Mac OSX need the reset_register
> section of the FADT filled to know which port to write to for a system reset.
>
> What is the right way to set the reset_val and the reset addr in this case?
> ---
>   hw/i386/acpi-build.c | 5 +++++
>   hw/i386/acpi-defs.h  | 2 ++
>   2 files changed, 7 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d0a5c85..21c1453 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -361,6 +361,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
>                                 (1 << ACPI_FADT_F_SLP_BUTTON) |
>                                 (1 << ACPI_FADT_F_RTC_S4));
>       fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> +    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> +    fadt->reset_val = 0xf;
> +    fadt->reset_reg.address_space_id   = aml_system_io;
> +    fadt->reset_reg.register_bit_width = 8;
> +    fadt->reset_reg.address            = ICH9_RST_CNT_IOPORT;
Hi,

Is this for both PC and Q35? Because I don't think PC has ICH9_RST_CNT_IOPORT.

Thanks,
Marcel

>       /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>        * For more than 8 CPUs, "Clustered Logical" mode has to be used
>        */
> diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
> index c4468f8..960c833 100644
> --- a/hw/i386/acpi-defs.h
> +++ b/hw/i386/acpi-defs.h
> @@ -132,6 +132,8 @@ struct AcpiFadtDescriptorRev1
>       uint8_t  reserved4a;             /* Reserved */
>       uint8_t  reserved4b;             /* Reserved */
>       uint32_t flags;
> +    Acpi20GenericAddress reset_reg;
> +    uint8_t reset_val;
>   } QEMU_PACKED;
>   typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
>
>

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Igor Mammedov]

On Sat, 28 Mar 2015 15:46:53 +0100
Reza Jelveh <fishman@saucelabs.com> wrote:

> Some operating systems such as FreeBSD and Mac OSX need the reset_register
> section of the FADT filled to know which port to write to for a system reset.
> 
> What is the right way to set the reset_val and the reset addr in this case?
> ---
>  hw/i386/acpi-build.c | 5 +++++
>  hw/i386/acpi-defs.h  | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d0a5c85..21c1453 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -361,6 +361,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
>                                (1 << ACPI_FADT_F_SLP_BUTTON) |
>                                (1 << ACPI_FADT_F_RTC_S4));
>      fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> +    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> +    fadt->reset_val = 0xf;
> +    fadt->reset_reg.address_space_id   = aml_system_io;
> +    fadt->reset_reg.register_bit_width = 8;
> +    fadt->reset_reg.address            = ICH9_RST_CNT_IOPORT;
>      /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
>       * For more than 8 CPUs, "Clustered Logical" mode has to be used
>       */
> diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
> index c4468f8..960c833 100644
> --- a/hw/i386/acpi-defs.h
> +++ b/hw/i386/acpi-defs.h
> @@ -132,6 +132,8 @@ struct AcpiFadtDescriptorRev1
>      uint8_t  reserved4a;             /* Reserved */
>      uint8_t  reserved4b;             /* Reserved */
>      uint32_t flags;
> +    Acpi20GenericAddress reset_reg;
> +    uint8_t reset_val;
you are extending structure beyond of what specified by ACPI 1.0b spec,
that might break guests.
We probably can't change revision since Windows ACPI implementation
is mostly 1.0b based so we are stuck with it.
Patch needs to be tested with Windows guests starting with XP.

related issue,
adding fields without changing major version to a corresponding
one doesn't look correct.

>  } QEMU_PACKED;
>  typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
>  

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Reza Jelveh]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

On Mon, Mar 30, 2015 at 2:36 PM, Igor Mammedov <imammedo@redhat.com> wrote:

> you are extending structure beyond of what specified by ACPI 1.0b spec,
> that might break guests.
> We probably can't change revision since Windows ACPI implementation
> is mostly 1.0b based so we are stuck with it.
> Patch needs to be tested with Windows guests starting with XP.
>

Works fine with XP as long as you don't use q35, but i guess that's
unsupported by xp by default.


> related issue,
> adding fields without changing major version to a corresponding
> one doesn't look correct.
>

What do you suggest? saying we support 2.0a just because of introducing
reset_reg is not really valid is it?

[-- Attachment #2: Type: text/html, Size: 1278 bytes --]

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Igor Mammedov]

On Mon, 30 Mar 2015 15:27:26 +0200
Reza Jelveh <fishman@saucelabs.com> wrote:

> On Mon, Mar 30, 2015 at 2:36 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > you are extending structure beyond of what specified by ACPI 1.0b spec,
> > that might break guests.
> > We probably can't change revision since Windows ACPI implementation
> > is mostly 1.0b based so we are stuck with it.
> > Patch needs to be tested with Windows guests starting with XP.
> >
> 
> Works fine with XP as long as you don't use q35, but i guess that's
> unsupported by xp by default.
what about other windows versions?

> 
> 
> > related issue,
> > adding fields without changing major version to a corresponding
> > one doesn't look correct.
> >
> 
> What do you suggest? saying we support 2.0a just because of introducing
> reset_reg is not really valid is it?
Try to check whether windows works fine with a newer revision,
if it is, we can create revX FADT structure and use in with new machine types.
In that case we could consider implementing rev5 table so it could be reused
in ARM target.

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Paolo Bonzini]

On 29/03/2015 19:38, Marcel Apfelbaum wrote:
> On 03/28/2015 05:46 PM, Reza Jelveh wrote:
>> Some operating systems such as FreeBSD and Mac OSX need the
>> reset_register
>> section of the FADT filled to know which port to write to for a system
>> reset.
>>
>> What is the right way to set the reset_val and the reset addr in this
>> case?
>> ---
>>   hw/i386/acpi-build.c | 5 +++++
>>   hw/i386/acpi-defs.h  | 2 ++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index d0a5c85..21c1453 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -361,6 +361,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1
>> *fadt, AcpiPmInfo *pm)
>>                                 (1 << ACPI_FADT_F_SLP_BUTTON) |
>>                                 (1 << ACPI_FADT_F_RTC_S4));
>>       fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
>> +    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
>> +    fadt->reset_val = 0xf;
>> +    fadt->reset_reg.address_space_id   = aml_system_io;
>> +    fadt->reset_reg.register_bit_width = 8;
>> +    fadt->reset_reg.address            = ICH9_RST_CNT_IOPORT;
> Hi,
> 
> Is this for both PC and Q35? Because I don't think PC has
> ICH9_RST_CNT_IOPORT.

It does (see RCR_IOPORT).

Paolo

> Thanks,
> Marcel
> 
>>       /* APIC destination mode ("Flat Logical") has an upper limit of
>> 8 CPUs
>>        * For more than 8 CPUs, "Clustered Logical" mode has to be used
>>        */
>> diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
>> index c4468f8..960c833 100644
>> --- a/hw/i386/acpi-defs.h
>> +++ b/hw/i386/acpi-defs.h
>> @@ -132,6 +132,8 @@ struct AcpiFadtDescriptorRev1
>>       uint8_t  reserved4a;             /* Reserved */
>>       uint8_t  reserved4b;             /* Reserved */
>>       uint32_t flags;
>> +    Acpi20GenericAddress reset_reg;
>> +    uint8_t reset_val;
>>   } QEMU_PACKED;
>>   typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
>>
>>
> 
> 
> 

Re: [Qemu-devel] [RFC] acpi: add reset register to fadt

[Paolo Bonzini]

On 30/03/2015 15:27, Reza Jelveh wrote:
>     you are extending structure beyond of what specified by ACPI 1.0b spec,
>     that might break guests.
>     We probably can't change revision since Windows ACPI implementation
>     is mostly 1.0b based so we are stuck with it.
>     Patch needs to be tested with Windows guests starting with XP.
> 
> Works fine with XP as long as you don't use q35, but i guess that's
> unsupported by xp by default. 

Isn't that just because XP doesn't have an AHCI driver?  You should
still be able to use XP with q35 and virtio, by using a driver disk.  Or
add a PATA controller:

   -device piix4-ide,id=pata \
   -drive if=none,id=hd,file=... \
   -device ide-hd,drive=hd,bus=pata.0

Paolo