| Message ID | 20200327121111.1530-6-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: i386 tweaks | expand |
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");
> > -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
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 >
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
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");
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(-)