[Qemu-devel] [PATCH] pc: acpi: keep pvpanic backward compatible

[Qemu-devel] [PATCH] pc: acpi: keep pvpanic backward compatible

[Radim Krčmář]

In the old times, we always had pvpanic in ACPI and a _STA function told
the guest not to use it.  Now, we only include pvpanic in ACPI if it is
enabled, so the _STA function is useless from qemu's point of view, but
guests still want to query availability.  Make them happy.

This patch extends
  e65bef695441 pc: acpi: fix pvpanic regression

The original _STA method was
  Method (_STA, 0, NotSerialized) {
      Store (PEST, Local0)
      If (LEqual (Local0, Zero)) {
          Return (Zero) }
      Else {
          Return (0x0F) }}

hence we return only 0xf now.
(Why Linux does '& 0xb' on this result is completely beyond me.)

Also, the device used to be PEVT.  (PEVT as in "panic event"?)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/acpi-build.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 73259e729b9f..4141c110a8e1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -733,7 +733,7 @@ build_ssdt(GArray *table_data, GArray *linker,
     if (misc->pvpanic_port) {
         scope = aml_scope("\\_SB.PCI0.ISA");
 
-        dev = aml_device("PEVR");
+        dev = aml_device("PEVT");
         aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
 
         crs = aml_resource_template();
@@ -748,6 +748,10 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(field, aml_named_field("PEPT", 8));
         aml_append(dev, field);
 
+        method = aml_method("_STA", 0);
+        aml_append(method, aml_return(aml_int(0xf)));
+        aml_append(dev, method);
+
         method = aml_method("RDPT", 0);
         aml_append(method, aml_store(aml_name("PEPT"), aml_local(0)));
         aml_append(method, aml_return(aml_local(0)));
-- 
2.4.1

Re: [Qemu-devel] [PATCH] pc: acpi: keep pvpanic backward compatible

[Igor Mammedov]

On Wed, 27 May 2015 18:29:17 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> In the old times, we always had pvpanic in ACPI and a _STA function told
> the guest not to use it.  Now, we only include pvpanic in ACPI if it is
> enabled, so the _STA function is useless from qemu's point of view, but
> guests still want to query availability.  Make them happy.
maybe also fix guest's driver not to do since absence of _STA
implies that device is present according to ACPI6.0 6.3.7 _STA

or only fix driver and drop this patch so that QEMU wouldn't carry
useless junk.

> 
> This patch extends
>   e65bef695441 pc: acpi: fix pvpanic regression
> 
> The original _STA method was
>   Method (_STA, 0, NotSerialized) {
>       Store (PEST, Local0)
>       If (LEqual (Local0, Zero)) {
>           Return (Zero) }
>       Else {
>           Return (0x0F) }}
> 
> hence we return only 0xf now.
> (Why Linux does '& 0xb' on this result is completely beyond me.)
check _STA related comment in SMC

> 
> Also, the device used to be PEVT.  (PEVT as in "panic event"?)
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/i386/acpi-build.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 73259e729b9f..4141c110a8e1 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -733,7 +733,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>      if (misc->pvpanic_port) {
>          scope = aml_scope("\\_SB.PCI0.ISA");
>  
> -        dev = aml_device("PEVR");
> +        dev = aml_device("PEVT");
>          aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
>  
>          crs = aml_resource_template();
> @@ -748,6 +748,10 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(field, aml_named_field("PEPT", 8));
>          aml_append(dev, field);
>  
> +        method = aml_method("_STA", 0);
> +        aml_append(method, aml_return(aml_int(0xf)));
> +        aml_append(dev, method);
doesn't have to be method,

make it like this:
        /* device present, functioning, decoding, not shown in UI */
        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
>          method = aml_method("RDPT", 0);
>          aml_append(method, aml_store(aml_name("PEPT"), aml_local(0)));
>          aml_append(method, aml_return(aml_local(0)));

Re: [Qemu-devel] [PATCH] pc: acpi: keep pvpanic backward compatible

[Radim Krčmář]

2015-05-29 10:51+0200, Igor Mammedov:
> On Wed, 27 May 2015 18:29:17 +0200
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> > In the old times, we always had pvpanic in ACPI and a _STA function told
> > the guest not to use it.  Now, we only include pvpanic in ACPI if it is
> > enabled, so the _STA function is useless from qemu's point of view, but
> > guests still want to query availability.  Make them happy.
> maybe also fix guest's driver not to do since absence of _STA
> implies that device is present according to ACPI6.0 6.3.7 _STA

Will do.

> or only fix driver and drop this patch so that QEMU wouldn't carry
> useless junk.

I think we still want to run old guests on new versions of QEMU.
We could have kept the incremented _HID otherwise, it would be better.

> > (Why Linux does '& 0xb' on this result is completely beyond me.)
> check _STA related comment in SMC

(Time to actually open the spec ...)

> > +        method = aml_method("_STA", 0);
> > +        aml_append(method, aml_return(aml_int(0xf)));
> > +        aml_append(dev, method);
> doesn't have to be method,
> 
> make it like this:
>         /* device present, functioning, decoding, not shown in UI */
>         aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));

Nice, I didn't expect that much from the evaluator.

Thank you for all the information.
(v2 coming after I read few hundred pages.)