Re: [PULL v4 44/83] acpi: pc: vga: use AcpiDevAmlIf interface to build VGA device descriptors

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 09, 2022 at 06:39:27PM +0100, Laurent Vivier wrote:
> This one breaks something for me:
> 
> [3/65] Compiling C object libhw-display-virtio-vga-gl.a.p/hw_display_acpi-vga.c.o
> FAILED: libhw-display-virtio-vga-gl.a.p/hw_display_acpi-vga.c.o
> clang -m64 -mcx16 -Ilibhw-display-virtio-vga-gl.a.p -I.
> -I../../../Projects/qemu-upstream -Iqapi -Itrace -Iui -Iui/shader
> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include
> -I/usr/include/sysprof-4 -fcolor-diagnostics -Wall -Winvalid-pch -Werror
> -std=gnu11 -O2 -g -isystem
> /home/lvivier/Projects/qemu-upstream/linux-headers -isystem linux-headers
> -iquote . -iquote /home/lvivier/Projects/qemu-upstream -iquote
> /home/lvivier/Projects/qemu-upstream/include -iquote
> /home/lvivier/Projects/qemu-upstream/tcg/i386 -pthread -D_GNU_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes
> -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes
> -fno-strict-aliasing -fno-common -fwrapv -Wold-style-definition
> -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels
> -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs
> -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition
> -Wno-tautological-type-limit-compare -Wno-psabi
> -Wno-gnu-variable-sized-type-not-at-end -fstack-protector-strong
> -fsanitize=address -fPIC -DBUILD_DSO -MD -MQ
> libhw-display-virtio-vga-gl.a.p/hw_display_acpi-vga.c.o -MF
> libhw-display-virtio-vga-gl.a.p/hw_display_acpi-vga.c.o.d -o
> libhw-display-virtio-vga-gl.a.p/hw_display_acpi-vga.c.o -c
> ../../../Projects/qemu-upstream/hw/display/acpi-vga.c
> In file included from ../../../Projects/qemu-upstream/hw/display/acpi-vga.c:4:
> In file included from ../../../Projects/qemu-upstream/hw/display/vga_int.h:30:
> In file included from /home/lvivier/Projects/qemu-upstream/include/ui/console.h:4:
> /home/lvivier/Projects/qemu-upstream/include/ui/qemu-pixman.h:12:10: fatal
> error: 'pixman.h' file not found
> #include <pixman.h>
>          ^~~~~~~~~~
> 1 error generated.
> ninja: build stopped: subcommand failed.
> make: *** [Makefile:165: run-ninja] Error 1
> make: Leaving directory '/home/lvivier/Objects/qemu-upstream/x86_64'
> 
> Any idea?
> 
> my configure is:
> 
> configure' '--disable-docs' '--target-list=x86_64-softmmu'
> '--enable-modules' '--disable-spice' '--enable-docs'
> 
> path to pixman.h is:
> 
> /usr/include/pixman-1/pixman.h
> 
> Thanks,
> Laurent
> 
> On 11/7/22 23:51, Michael S. Tsirkin wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > NB:
> > We do not expect any functional change in any ACPI tables with this
> > change. It's only a refactoring.
> > 
> > NB2:
> > Some targets (or1k) do not support acpi and CONFIG_ACPI is off for them.
> > However, modules are reused between all architectures so CONFIG_ACPI is
> > on.  For those architectures, dummy stub function definitions help to
> > resolve symbols.  This change uses more of these and so it adds a couple
> > of dummy stub definitions so that symbols for those can be resolved.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20221017102146.2254096-2-imammedo@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > CC: Bernhard Beschow <shentey@gmail.com>
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <20221107152744.868434-1-ani@anisinha.ca>
> > ---
> >   hw/display/vga_int.h       |  2 ++
> >   hw/acpi/aml-build-stub.c   | 10 ++++++++++
> >   hw/display/acpi-vga-stub.c |  7 +++++++
> >   hw/display/acpi-vga.c      | 26 ++++++++++++++++++++++++++
> >   hw/display/vga-pci.c       |  4 ++++
> >   hw/i386/acpi-build.c       | 26 +-------------------------
> >   hw/display/meson.build     | 17 +++++++++++++++++
> >   7 files changed, 67 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/acpi/aml-build-stub.c b/hw/acpi/aml-build-stub.c
> > index 8d8ad1a314..89a8fec4af 100644
> > --- a/hw/acpi/aml-build-stub.c
> > +++ b/hw/acpi/aml-build-stub.c
> > @@ -26,6 +26,16 @@ void aml_append(Aml *parent_ctx, Aml *child)
> >   {
> >   }
> > +Aml *aml_return(Aml *val)
> > +{
> > +    return NULL;
> > +}
> > +
> > +Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag)
> > +{
> > +    return NULL;
> > +}
> > +
> >   Aml *aml_resource_template(void)
> >   {
> >       return NULL;
> > 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/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) {
> > 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

Igor what does  CONFIG_ACPI mean? It depends on the target but this
library is target independent. Is this just always built then?

> > +
> >   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 }




Does the following help? It seems like a worthwile cleanup anyway.


diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 330406ad9c..7cf0d11201 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -27,7 +27,6 @@
 
 #include "exec/ioport.h"
 #include "exec/memory.h"
-#include "ui/console.h"
 
 #include "hw/display/bochs-vbe.h"
 #include "hw/acpi/acpi_aml_interface.h"
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 6d4e6d9708..688408e048 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -132,6 +132,8 @@ typedef struct Visitor Visitor;
 typedef struct VMChangeStateEntry VMChangeStateEntry;
 typedef struct VMStateDescription VMStateDescription;
 typedef struct DumpState DumpState;
+typedef struct GraphicHwOps GraphicHwOps;
+typedef struct QEMUCursor QEMUCursor;
 
 /*
  * Pointer types
diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
index 692bec91de..7d786653e8 100644
--- a/hw/display/ati_2d.c
+++ b/hw/display/ati_2d.c
@@ -12,6 +12,7 @@
 #include "ati_regs.h"
 #include "qemu/log.h"
 #include "ui/pixel_ops.h"
+#include "ui/console.h"
 
 /*
  * NOTE:
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 2577005d03..4cc3567c69 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -45,6 +45,7 @@
 #include "ui/pixel_ops.h"
 #include "cirrus_vga_internal.h"
 #include "qom/object.h"
+#include "ui/console.h"
 
 /*
  * TODO:
diff --git a/hw/display/cirrus_vga_isa.c b/hw/display/cirrus_vga_isa.c
index 96144bd690..84be51670e 100644
--- a/hw/display/cirrus_vga_isa.c
+++ b/hw/display/cirrus_vga_isa.c
@@ -31,6 +31,7 @@
 #include "hw/isa/isa.h"
 #include "cirrus_vga_internal.h"
 #include "qom/object.h"
+#include "ui/console.h"
 
 #define TYPE_ISA_CIRRUS_VGA "isa-cirrus-vga"
 OBJECT_DECLARE_SIMPLE_TYPE(ISACirrusVGAState, ISA_CIRRUS_VGA)
diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c
index 46abbc5653..2a5437d803 100644
--- a/hw/display/vga-isa.c
+++ b/hw/display/vga-isa.c
@@ -32,6 +32,7 @@
 #include "qemu/timer.h"
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
+#include "ui/console.h"
 #include "qom/object.h"
 
 #define TYPE_ISA_VGA "isa-vga"
diff --git a/hw/display/vga-mmio.c b/hw/display/vga-mmio.c
index 75dfcedea5..cd2c46776d 100644
--- a/hw/display/vga-mmio.c
+++ b/hw/display/vga-mmio.c
@@ -27,6 +27,7 @@
 #include "hw/sysbus.h"
 #include "hw/display/vga.h"
 #include "hw/qdev-properties.h"
+#include "ui/console.h"
 #include "vga_int.h"
 
 /*
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 9a91de7ed1..df23dbf3a0 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -30,6 +30,7 @@
 #include "migration/vmstate.h"
 #include "vga_int.h"
 #include "ui/pixel_ops.h"
+#include "ui/console.h"
 #include "qemu/module.h"
 #include "qemu/timer.h"
 #include "hw/loader.h"
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 50ecb1ad02..0cb26a791b 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -31,6 +31,7 @@
 #include "vga_int.h"
 #include "vga_regs.h"
 #include "ui/pixel_ops.h"
+#include "ui/console.h"
 #include "qemu/timer.h"
 #include "hw/xen/xen.h"
 #include "migration/vmstate.h"
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index cedbbde522..53949d2539 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -33,6 +33,7 @@
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qom/object.h"
+#include "ui/console.h"
 
 #undef VERBOSE
 #define HW_RECT_ACCEL