[PATCH v3 10/15] acpi: parallel: don't use _STA method

Gerd Hoffmann posted 15 patches 5 years, 9 months ago
Maintainers: John Snow <jsnow@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Kevin Wolf <kwolf@redhat.com>, Richard Henderson <rth@twiddle.net>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <lvivier@redhat.com>, Thomas Huth <thuth@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Igor Mammedov <imammedo@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
[PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Gerd Hoffmann 5 years, 9 months ago
The _STA method dates back to the days where we had a static DSDT.  The
device is listed in the DSDT table unconditionally and the _STA method
checks a bit in the isa bridge pci config space to figure whenever a
given is isa device is present or not, then evaluates to 0x0f (present)
or 0x00 (absent).

These days the DSDT is generated by qemu anyway, so if a device is not
present we can simply drop it from the DSDT instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index fea83352e6ab..e01afbd011d9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1167,39 +1167,26 @@ static Aml *build_mouse_device_aml(void)
     return dev;
 }
 
-static Aml *build_lpt_device_aml(void)
+static void build_lpt_device_aml(Aml *scope)
 {
     Aml *dev;
     Aml *crs;
-    Aml *method;
-    Aml *if_ctx;
-    Aml *else_ctx;
-    Aml *zero = aml_int(0);
-    Aml *is_present = aml_local(0);
+
+    if (!memory_region_present(get_system_io(), 0x0378)) {
+        return;
+    }
 
     dev = aml_device("LPT");
     aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
 
-    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_store(aml_name("LPEN"), is_present));
-    if_ctx = aml_if(aml_equal(is_present, zero));
-    {
-        aml_append(if_ctx, aml_return(aml_int(0x00)));
-    }
-    aml_append(method, if_ctx);
-    else_ctx = aml_else();
-    {
-        aml_append(else_ctx, aml_return(aml_int(0x0f)));
-    }
-    aml_append(method, else_ctx);
-    aml_append(dev, method);
+    aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
 
     crs = aml_resource_template();
     aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
     aml_append(crs, aml_irq_no_flags(7));
     aml_append(dev, aml_name_decl("_CRS", crs));
 
-    return dev;
+    aml_append(scope, dev);
 }
 
 static void build_isa_devices_aml(Aml *table)
@@ -1215,7 +1202,7 @@ static void build_isa_devices_aml(Aml *table)
     if (fdc) {
         aml_append(scope, build_fdc_device_aml(fdc));
     }
-    aml_append(scope, build_lpt_device_aml());
+    build_lpt_device_aml(scope);
 
     if (ambiguous) {
         error_report("Multiple ISA busses, unable to define IPMI ACPI data");
-- 
2.18.2


Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
On 4/29/20 3:59 PM, Gerd Hoffmann wrote:
> The _STA method dates back to the days where we had a static DSDT.  The
> device is listed in the DSDT table unconditionally and the _STA method
> checks a bit in the isa bridge pci config space to figure whenever a
> given is isa device is present or not, then evaluates to 0x0f (present)
> or 0x00 (absent).
> 
> These days the DSDT is generated by qemu anyway, so if a device is not
> present we can simply drop it from the DSDT instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   hw/i386/acpi-build.c | 29 ++++++++---------------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fea83352e6ab..e01afbd011d9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1167,39 +1167,26 @@ static Aml *build_mouse_device_aml(void)
>       return dev;
>   }
>   
> -static Aml *build_lpt_device_aml(void)
> +static void build_lpt_device_aml(Aml *scope)
>   {
>       Aml *dev;
>       Aml *crs;
> -    Aml *method;
> -    Aml *if_ctx;
> -    Aml *else_ctx;
> -    Aml *zero = aml_int(0);
> -    Aml *is_present = aml_local(0);
> +
> +    if (!memory_region_present(get_system_io(), 0x0378)) {
> +        return;
> +    }

Please move this check in a separate patch.

Also, it may be worth a ISA_PARALLEL_IOBASE 0x378 definition cleanup 
like you did with the RTC.

>   
>       dev = aml_device("LPT");
>       aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
>   
> -    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_store(aml_name("LPEN"), is_present));
> -    if_ctx = aml_if(aml_equal(is_present, zero));
> -    {
> -        aml_append(if_ctx, aml_return(aml_int(0x00)));
> -    }
> -    aml_append(method, if_ctx);
> -    else_ctx = aml_else();
> -    {
> -        aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -    }
> -    aml_append(method, else_ctx);
> -    aml_append(dev, method);
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>   
>       crs = aml_resource_template();
>       aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
>       aml_append(crs, aml_irq_no_flags(7));
>       aml_append(dev, aml_name_decl("_CRS", crs));
>   
> -    return dev;
> +    aml_append(scope, dev);
>   }
>   
>   static void build_isa_devices_aml(Aml *table)
> @@ -1215,7 +1202,7 @@ static void build_isa_devices_aml(Aml *table)
>       if (fdc) {
>           aml_append(scope, build_fdc_device_aml(fdc));
>       }
> -    aml_append(scope, build_lpt_device_aml());
> +    build_lpt_device_aml(scope);
>   
>       if (ambiguous) {
>           error_report("Multiple ISA busses, unable to define IPMI ACPI data");
> 


Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Gerd Hoffmann 5 years, 9 months ago
  Hi,

> Also, it may be worth a ISA_PARALLEL_IOBASE 0x378 definition cleanup like
> you did with the RTC.

Next patch in series deals with that.

take care,
  Gerd


Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Igor Mammedov 5 years, 9 months ago
On Wed, 29 Apr 2020 15:59:58 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> The _STA method dates back to the days where we had a static DSDT.  The
> device is listed in the DSDT table unconditionally and the _STA method
> checks a bit in the isa bridge pci config space to figure whenever a
> given is isa device is present or not, then evaluates to 0x0f (present)
> or 0x00 (absent).
> 
> These days the DSDT is generated by qemu anyway, so if a device is not
> present we can simply drop it from the DSDT instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

looking more at it, we should also cleanup no longer used LPEN field
the same applies to similar fields for serial and ... 


> ---
>  hw/i386/acpi-build.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fea83352e6ab..e01afbd011d9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1167,39 +1167,26 @@ static Aml *build_mouse_device_aml(void)
>      return dev;
>  }
>  
> -static Aml *build_lpt_device_aml(void)
> +static void build_lpt_device_aml(Aml *scope)
>  {
>      Aml *dev;
>      Aml *crs;
> -    Aml *method;
> -    Aml *if_ctx;
> -    Aml *else_ctx;
> -    Aml *zero = aml_int(0);
> -    Aml *is_present = aml_local(0);
> +
> +    if (!memory_region_present(get_system_io(), 0x0378)) {
> +        return;
> +    }
>  
>      dev = aml_device("LPT");
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
>  
> -    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_store(aml_name("LPEN"), is_present));
> -    if_ctx = aml_if(aml_equal(is_present, zero));
> -    {
> -        aml_append(if_ctx, aml_return(aml_int(0x00)));
> -    }
> -    aml_append(method, if_ctx);
> -    else_ctx = aml_else();
> -    {
> -        aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -    }
> -    aml_append(method, else_ctx);
> -    aml_append(dev, method);
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>      crs = aml_resource_template();
>      aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
>      aml_append(crs, aml_irq_no_flags(7));
>      aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -    return dev;
> +    aml_append(scope, dev);
>  }
>  
>  static void build_isa_devices_aml(Aml *table)
> @@ -1215,7 +1202,7 @@ static void build_isa_devices_aml(Aml *table)
>      if (fdc) {
>          aml_append(scope, build_fdc_device_aml(fdc));
>      }
> -    aml_append(scope, build_lpt_device_aml());
> +    build_lpt_device_aml(scope);
>  
>      if (ambiguous) {
>          error_report("Multiple ISA busses, unable to define IPMI ACPI data");


Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Gerd Hoffmann 5 years, 9 months ago
On Thu, Apr 30, 2020 at 06:25:24PM +0200, Igor Mammedov wrote:
> On Wed, 29 Apr 2020 15:59:58 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > The _STA method dates back to the days where we had a static DSDT.  The
> > device is listed in the DSDT table unconditionally and the _STA method
> > checks a bit in the isa bridge pci config space to figure whenever a
> > given is isa device is present or not, then evaluates to 0x0f (present)
> > or 0x00 (absent).
> > 
> > These days the DSDT is generated by qemu anyway, so if a device is not
> > present we can simply drop it from the DSDT instead.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> looking more at it, we should also cleanup no longer used LPEN field
> the same applies to similar fields for serial and ... 

IIRC these bits are in the chipset specs so we should not remove them
from pci config space.

Removing from DSDT should be possible indeed (I guess this is what you
mean?)

take care,
  Gerd


Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method
Posted by Igor Mammedov 5 years, 9 months ago
On Mon, 4 May 2020 15:25:16 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Thu, Apr 30, 2020 at 06:25:24PM +0200, Igor Mammedov wrote:
> > On Wed, 29 Apr 2020 15:59:58 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > The _STA method dates back to the days where we had a static DSDT.  The
> > > device is listed in the DSDT table unconditionally and the _STA method
> > > checks a bit in the isa bridge pci config space to figure whenever a
> > > given is isa device is present or not, then evaluates to 0x0f (present)
> > > or 0x00 (absent).
> > > 
> > > These days the DSDT is generated by qemu anyway, so if a device is not
> > > present we can simply drop it from the DSDT instead.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > looking more at it, we should also cleanup no longer used LPEN field
> > the same applies to similar fields for serial and ...   
> 
> IIRC these bits are in the chipset specs so we should not remove them
> from pci config space.
we also can't touch device model due to migration resons
(ACPI code that checks STA bits is in the wild, and us removing ACPI part
of it won't change that)

> 
> Removing from DSDT should be possible indeed (I guess this is what you
> mean?)
yep

> 
> take care,
>   Gerd
>