Re: [PATCH 01/11] acpi: pc: vga: use AcpiDevAmlIf interface to build VGA device descriptors

[Ani Sinha]

On Mon, Oct 17, 2022 at 3:52 PM Igor Mammedov <imammedo@redhat.com> wrote:
>

I would add a comment that we do not expect any functional change in
any ACPI tables with this change. It's only a refactoring.

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Other than the comment above,
Reviewed-by: Ani Sinha <ani@anisinha.ca>

> ---
>  hw/display/vga_int.h       |  2 ++
>  hw/display/acpi-vga-stub.c |  7 +++++++
>  hw/display/acpi-vga.c      | 26 ++++++++++++++++++++++++++
>  hw/display/meson.build     | 17 +++++++++++++++++
>  hw/display/vga-pci.c       |  4 ++++
>  hw/i386/acpi-build.c       | 26 +-------------------------
>  6 files changed, 57 insertions(+), 25 deletions(-)
>  create mode 100644 hw/display/acpi-vga-stub.c
>  create mode 100644 hw/display/acpi-vga.c
>
> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
> index 305e700014..330406ad9c 100644
> --- a/hw/display/vga_int.h
> +++ b/hw/display/vga_int.h
> @@ -30,6 +30,7 @@
>  #include "ui/console.h"
>
>  #include "hw/display/bochs-vbe.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>
>  #define ST01_V_RETRACE      0x08
>  #define ST01_DISP_ENABLE    0x01
> @@ -195,4 +196,5 @@ void pci_std_vga_mmio_region_init(VGACommonState *s,
>                                    MemoryRegion *subs,
>                                    bool qext, bool edid);
>
> +void build_vga_aml(AcpiDevAmlIf *adev, Aml *scope);
>  #endif
> diff --git a/hw/display/acpi-vga-stub.c b/hw/display/acpi-vga-stub.c
> new file mode 100644
> index 0000000000..a9b0ecf76d
> --- /dev/null
> +++ b/hw/display/acpi-vga-stub.c
> @@ -0,0 +1,7 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/acpi_aml_interface.h"
> +#include "vga_int.h"
> +
> +void build_vga_aml(AcpiDevAmlIf *adev, Aml *scope)
> +{
> +}
> diff --git a/hw/display/acpi-vga.c b/hw/display/acpi-vga.c
> new file mode 100644
> index 0000000000..f0e9ef1fcf
> --- /dev/null
> +++ b/hw/display/acpi-vga.c
> @@ -0,0 +1,26 @@
> +#include "qemu/osdep.h"
> +#include "hw/acpi/acpi_aml_interface.h"
> +#include "hw/pci/pci.h"
> +#include "vga_int.h"
> +
> +void build_vga_aml(AcpiDevAmlIf *adev, Aml *scope)
> +{
> +    int s3d = 0;
> +    Aml *method;
> +
> +    if (object_dynamic_cast(OBJECT(adev), "qxl-vga")) {
> +        s3d = 3;
> +    }
> +
> +    method = aml_method("_S1D", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(0)));
> +    aml_append(scope, method);
> +
> +    method = aml_method("_S2D", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(0)));
> +    aml_append(scope, method);
> +
> +    method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
> +    aml_append(method, aml_return(aml_int(s3d)));
> +    aml_append(scope, method);
> +}
> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index adc53dd8b6..7a725ed80e 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -38,10 +38,21 @@ softmmu_ss.add(when: 'CONFIG_NEXTCUBE', if_true: 
> files('next-fb.c'))
>
>  specific_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
>
> +if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
> +    config_all_devices.has_key('CONFIG_VGA_PCI') or
> +    config_all_devices.has_key('CONFIG_VMWARE_VGA') or
> +    config_all_devices.has_key('CONFIG_ATI_VGA')
> +   )
> +  softmmu_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
> +                                      if_false: files('acpi-vga-stub.c'))
> +endif
> +
>  if config_all_devices.has_key('CONFIG_QXL')
>    qxl_ss = ss.source_set()
>    qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 
> 'qxl-render.c'),
>                                             pixman, spice])
> +  qxl_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
> +                                  if_false: files('acpi-vga-stub.c'))
>    hw_display_modules += {'qxl': qxl_ss}
>  endif
>
> @@ -52,6 +63,7 @@ softmmu_ss.add(when: 'CONFIG_ARTIST', if_true: 
> files('artist.c'))
>
>  softmmu_ss.add(when: [pixman, 'CONFIG_ATI_VGA'], if_true: files('ati.c', 
> 'ati_2d.c', 'ati_dbg.c'))
>
> +
>  if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
>    virtio_gpu_ss = ss.source_set()
>    virtio_gpu_ss.add(when: 'CONFIG_VIRTIO_GPU',
> @@ -87,14 +99,19 @@ if config_all_devices.has_key('CONFIG_VIRTIO_VGA')
>                      if_true: [files('virtio-vga.c'), pixman])
>    virtio_vga_ss.add(when: 'CONFIG_VHOST_USER_VGA',
>                      if_true: files('vhost-user-vga.c'))
> +  virtio_vga_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
> +                                         if_false: files('acpi-vga-stub.c'))
>    hw_display_modules += {'virtio-vga': virtio_vga_ss}
>
>    virtio_vga_gl_ss = ss.source_set()
>    virtio_vga_gl_ss.add(when: ['CONFIG_VIRTIO_VGA', virgl, opengl],
>                         if_true: [files('virtio-vga-gl.c'), pixman])
> +  virtio_vga_gl_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-vga.c'),
> +                                            if_false: 
> files('acpi-vga-stub.c'))
>    hw_display_modules += {'virtio-vga-gl': virtio_vga_gl_ss}
>  endif
>
>  specific_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_lcdc.c'))
>
> +softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('acpi-vga-stub.c'))
>  modules += { 'hw-display': hw_display_modules }
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index 3e5bc259f7..9a91de7ed1 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -35,6 +35,7 @@
>  #include "hw/loader.h"
>  #include "hw/display/edid.h"
>  #include "qom/object.h"
> +#include "hw/acpi/acpi_aml_interface.h"
>
>  enum vga_pci_flags {
>      PCI_VGA_FLAG_ENABLE_MMIO = 1,
> @@ -354,11 +355,13 @@ static void vga_pci_class_init(ObjectClass *klass, void 
> *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
>
>      k->vendor_id = PCI_VENDOR_ID_QEMU;
>      k->device_id = PCI_DEVICE_ID_QEMU_VGA;
>      dc->vmsd = &vmstate_vga_pci;
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
> +    adevc->build_dev_aml = build_vga_aml;
>  }
>
>  static const TypeInfo vga_pci_type_info = {
> @@ -369,6 +372,7 @@ static const TypeInfo vga_pci_type_info = {
>      .class_init = vga_pci_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { TYPE_ACPI_DEV_AML_IF },
>          { },
>      },
>  };
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4f54b61904..26932b4e2c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -430,7 +430,6 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>          bool hotpluggbale_slot = false;
>          bool bridge_in_acpi = false;
>          bool cold_plugged_bridge = false;
> -        bool is_vga = false;
>
>          if (pdev) {
>              pc = PCI_DEVICE_GET_CLASS(pdev);
> @@ -440,8 +439,6 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>                  continue;
>              }
>
> -            is_vga = pc->class_id == PCI_CLASS_DISPLAY_VGA;
> -
>              /*
>               * Cold plugged bridges aren't themselves hot-pluggable.
>               * Hotplugged bridges *are* hot-pluggable.
> @@ -489,28 +486,7 @@ static void build_append_pci_bus_devices(Aml 
> *parent_scope, PCIBus *bus,
>              aml_append(dev, aml_pci_device_dsm());
>          }
>
> -        if (is_vga) {
> -            /* add VGA specific AML methods */
> -            int s3d;
> -
> -            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> -                s3d = 3;
> -            } else {
> -                s3d = 0;
> -            }
> -
> -            method = aml_method("_S1D", 0, AML_NOTSERIALIZED);
> -            aml_append(method, aml_return(aml_int(0)));
> -            aml_append(dev, method);
> -
> -            method = aml_method("_S2D", 0, AML_NOTSERIALIZED);
> -            aml_append(method, aml_return(aml_int(0)));
> -            aml_append(dev, method);
> -
> -            method = aml_method("_S3D", 0, AML_NOTSERIALIZED);
> -            aml_append(method, aml_return(aml_int(s3d)));
> -            aml_append(dev, method);
> -        }
> +        call_dev_aml_func(DEVICE(pdev), dev);
>
>          bridge_in_acpi =  cold_plugged_bridge && pcihp_bridge_en;
>          if (bridge_in_acpi) {
> --
> 2.31.1
>