[5/6] acpi: serial: don't use _STA method
diff mbox series

Message ID 20200327121111.1530-6-kraxel@redhat.com
State New
Headers show
Series
  • acpi: i386 tweaks
Related show

Commit Message

Gerd Hoffmann March 27, 2020, 12:11 p.m. UTC
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>
---
 hw/i386/acpi-build-pc.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

Comments

Igor Mammedov March 27, 2020, 2:33 p.m. UTC | #1
On Fri, 27 Mar 2020 13:11:10 +0100
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>
> ---
>  hw/i386/acpi-build-pc.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/acpi-build-pc.c b/hw/i386/acpi-build-pc.c
> index 18ca2fd46961..97af5eac3d79 100644
> --- a/hw/i386/acpi-build-pc.c
> +++ b/hw/i386/acpi-build-pc.c
> @@ -1204,50 +1204,34 @@ static Aml *build_lpt_device_aml(void)
>      return dev;
>  }
>  
> -static Aml *build_com_device_aml(uint8_t uid)
> +static void build_com_device_aml(Aml *scope, uint8_t uid)
>  {
>      Aml *dev;
>      Aml *crs;
> -    Aml *method;
> -    Aml *if_ctx;
> -    Aml *else_ctx;
> -    Aml *zero = aml_int(0);
> -    Aml *is_present = aml_local(0);
> -    const char *enabled_field = "CAEN";
>      uint8_t irq = 4;
>      uint16_t io_port = 0x03F8;
>  
>      assert(uid == 1 || uid == 2);
>      if (uid == 2) {
> -        enabled_field = "CBEN";
>          irq = 3;
>          io_port = 0x02F8;
>      }
> +    if (!memory_region_present(get_system_io(), io_port)) {
                                  ^^^^^^
even though acpi_setup() is a part of board code, usually it's not recommended to 
use get_system_foo() outside of machine_init()

how about fishing out present serial ports from isa device in a helper
like acpi_get_misc_info(), and then generalize AML like
   build_com_device_aml(Aml *scope, uint8_t uid, io_port, irq)
   

> +        return;
> +    }
>  
>      dev = aml_device("COM%d", uid);
>      aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0501")));
>      aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
>  
> -    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_store(aml_name("%s", enabled_field), 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, io_port, io_port, 0x00, 0x08));
>      aml_append(crs, aml_irq_no_flags(irq));
>      aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -    return dev;
> +    aml_append(scope, dev);
>  }
>  
>  static void build_isa_devices_aml(Aml *table)
> @@ -1265,8 +1249,8 @@ static void build_isa_devices_aml(Aml *table)
>          aml_append(scope, build_fdc_device_aml(fdc));
>      }
>      aml_append(scope, build_lpt_device_aml());
> -    aml_append(scope, build_com_device_aml(1));
> -    aml_append(scope, build_com_device_aml(2));
> +    build_com_device_aml(scope, 1);
> +    build_com_device_aml(scope, 2);
>  
>      if (ambiguous) {
>          error_report("Multiple ISA busses, unable to define IPMI ACPI data");
Gerd Hoffmann March 31, 2020, 3:23 p.m. UTC | #2
> > -static Aml *build_com_device_aml(uint8_t uid)
> > +static void build_com_device_aml(Aml *scope, uint8_t uid)
> >  {
> >      Aml *dev;
> >      Aml *crs;
> > -    Aml *method;
> > -    Aml *if_ctx;
> > -    Aml *else_ctx;
> > -    Aml *zero = aml_int(0);
> > -    Aml *is_present = aml_local(0);
> > -    const char *enabled_field = "CAEN";
> >      uint8_t irq = 4;
> >      uint16_t io_port = 0x03F8;
> >  
> >      assert(uid == 1 || uid == 2);
> >      if (uid == 2) {
> > -        enabled_field = "CBEN";
> >          irq = 3;
> >          io_port = 0x02F8;
> >      }
> > +    if (!memory_region_present(get_system_io(), io_port)) {
>                                   ^^^^^^
> even though acpi_setup() is a part of board code, usually it's not recommended to 
> use get_system_foo() outside of machine_init()
> 
> how about fishing out present serial ports from isa device in a helper
> like acpi_get_misc_info(), and then generalize AML like
>    build_com_device_aml(Aml *scope, uint8_t uid, io_port, irq)

Hmm, I'm wondering whenever it would be useful to have ...

   ISADeviceClass->build_aml(Aml *scope, ISADevice *dev);

... then just walk all isa devices and call the handler
(if present).  Maybe the same for sysbus.

cheers,
  Gerd
Igor Mammedov March 31, 2020, 7:53 p.m. UTC | #3
On Tue, 31 Mar 2020 17:23:42 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> > > -static Aml *build_com_device_aml(uint8_t uid)
> > > +static void build_com_device_aml(Aml *scope, uint8_t uid)
> > >  {
> > >      Aml *dev;
> > >      Aml *crs;
> > > -    Aml *method;
> > > -    Aml *if_ctx;
> > > -    Aml *else_ctx;
> > > -    Aml *zero = aml_int(0);
> > > -    Aml *is_present = aml_local(0);
> > > -    const char *enabled_field = "CAEN";
> > >      uint8_t irq = 4;
> > >      uint16_t io_port = 0x03F8;
> > >  
> > >      assert(uid == 1 || uid == 2);
> > >      if (uid == 2) {
> > > -        enabled_field = "CBEN";
> > >          irq = 3;
> > >          io_port = 0x02F8;
> > >      }
> > > +    if (!memory_region_present(get_system_io(), io_port)) {  
> >                                   ^^^^^^
> > even though acpi_setup() is a part of board code, usually it's not recommended to 
> > use get_system_foo() outside of machine_init()
> > 
> > how about fishing out present serial ports from isa device in a helper
> > like acpi_get_misc_info(), and then generalize AML like
> >    build_com_device_aml(Aml *scope, uint8_t uid, io_port, irq)  
> 
> Hmm, I'm wondering whenever it would be useful to have ...
> 
>    ISADeviceClass->build_aml(Aml *scope, ISADevice *dev);

in relation to iqr, you said earlier that device doesn't know to which irq it's mapped.
that might be a problem in this case, likewise for (MM)IO

> ... then just walk all isa devices and call the handler
> (if present).  Maybe the same for sysbus.

There was already such idea (Paolo or Michael), i.e. to move AML code
generation related to specific devices inside of device model (not
only ISA or sysbus), so one would just have to enumerate present
devices in generic way and ask them to provide AML descriptors and be
done with building DSDT.

Not sure if it's doable in generic way though, especially when it comes to
orchestrating _CRS between various devices.
(while linux might still boot with resource conflicts, Windows 
BSODs occasionally instead). Maybe there are other complications
I don't recall.

So it might take awhile to come up with approach which would work nice.

Simplest way to get job done in case of microvm is to make board
fill in assigned resources in some helper data structure and pass that
to acpi code. (another approach - arm/virt uses static 'registry' to distribute
address space/irq and then acpi code just fetches values from there if
device is present + a bunch of shared PCI code to make up dynamic PCI
description)

> cheers,
>   Gerd
>
Gerd Hoffmann April 1, 2020, 5:51 a.m. UTC | #4
Hi,

> > Hmm, I'm wondering whenever it would be useful to have ...
> > 
> >    ISADeviceClass->build_aml(Aml *scope, ISADevice *dev);
> 
> in relation to iqr, you said earlier that device doesn't know to which irq it's mapped.
> that might be a problem in this case, likewise for (MM)IO

Right, this is a problem for sysbus, isa seems to not have this problem
though.

> > ... then just walk all isa devices and call the handler
> > (if present).  Maybe the same for sysbus.
> 
> There was already such idea (Paolo or Michael), i.e. to move AML code
> generation related to specific devices inside of device model (not
> only ISA or sysbus), so one would just have to enumerate present
> devices in generic way and ask them to provide AML descriptors and be
> done with building DSDT.
> 
> Not sure if it's doable in generic way though, especially when it comes to
> orchestrating _CRS between various devices.

I suspect fully generic is tricky, also because you have to get the
hierarchy right.  For isa I think it is doable without too much trouble
because all isa devices are within the same scope.

sysbus is more tricky I suspect.

> So it might take awhile to come up with approach which would work nice.
> 
> Simplest way to get job done in case of microvm is to make board
> fill in assigned resources in some helper data structure and pass that
> to acpi code.

Well, I'd try to avoid the helper data structure indirection ...

> (another approach - arm/virt uses static 'registry' to distribute
> address space/irq and then acpi code just fetches values from there if
> device is present + a bunch of shared PCI code to make up dynamic PCI
> description)

I suspect the reason for the registry on arm is that you can generate
both acpi and device tree from it.  But maybe that makes sense for
sysbus devices (fw_cfg + virtio-mmio for microvm).

cheers,
  Gerd

Patch
diff mbox series

diff --git a/hw/i386/acpi-build-pc.c b/hw/i386/acpi-build-pc.c
index 18ca2fd46961..97af5eac3d79 100644
--- a/hw/i386/acpi-build-pc.c
+++ b/hw/i386/acpi-build-pc.c
@@ -1204,50 +1204,34 @@  static Aml *build_lpt_device_aml(void)
     return dev;
 }
 
-static Aml *build_com_device_aml(uint8_t uid)
+static void build_com_device_aml(Aml *scope, uint8_t uid)
 {
     Aml *dev;
     Aml *crs;
-    Aml *method;
-    Aml *if_ctx;
-    Aml *else_ctx;
-    Aml *zero = aml_int(0);
-    Aml *is_present = aml_local(0);
-    const char *enabled_field = "CAEN";
     uint8_t irq = 4;
     uint16_t io_port = 0x03F8;
 
     assert(uid == 1 || uid == 2);
     if (uid == 2) {
-        enabled_field = "CBEN";
         irq = 3;
         io_port = 0x02F8;
     }
+    if (!memory_region_present(get_system_io(), io_port)) {
+        return;
+    }
 
     dev = aml_device("COM%d", uid);
     aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0501")));
     aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
 
-    method = aml_method("_STA", 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_store(aml_name("%s", enabled_field), 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, io_port, io_port, 0x00, 0x08));
     aml_append(crs, aml_irq_no_flags(irq));
     aml_append(dev, aml_name_decl("_CRS", crs));
 
-    return dev;
+    aml_append(scope, dev);
 }
 
 static void build_isa_devices_aml(Aml *table)
@@ -1265,8 +1249,8 @@  static void build_isa_devices_aml(Aml *table)
         aml_append(scope, build_fdc_device_aml(fdc));
     }
     aml_append(scope, build_lpt_device_aml());
-    aml_append(scope, build_com_device_aml(1));
-    aml_append(scope, build_com_device_aml(2));
+    build_com_device_aml(scope, 1);
+    build_com_device_aml(scope, 2);
 
     if (ambiguous) {
         error_report("Multiple ISA busses, unable to define IPMI ACPI data");