Re: [PATCH 01/10] hw/arm/virt-acpi-build.c: Move fw_cfg and virtio to common location

[Daniel Henrique Barboza]

On 7/26/23 05:25, Igor Mammedov wrote:
> On Tue, 25 Jul 2023 22:20:36 +0530
> Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> > On Mon, Jul 24, 2023 at 05:18:59PM +0200, Igor Mammedov wrote:
> > > On Wed, 12 Jul 2023 22:09:34 +0530
> > > Sunil V L <sunilvl@ventanamicro.com> wrote:
> > >    
> > > > The functions which add fw_cfg and virtio to DSDT are same for ARM
> > > > and RISC-V. So, instead of duplicating in RISC-V, move them from
> > > > hw/arm/virt-acpi-build.c to common aml-build.c.
> > > >
> > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > > ---
> > > >   hw/acpi/aml-build.c         | 41 ++++++++++++++++++++++++++++++++++++
> > > >   hw/arm/virt-acpi-build.c    | 42 -------------------------------------
> > > >   hw/riscv/virt-acpi-build.c  | 16 --------------
> > > >   include/hw/acpi/aml-build.h |  6 ++++++
> > > >   4 files changed, 47 insertions(+), 58 deletions(-)
> > > >
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > >
> > > patch looks fine modulo,
> > > I'd put these into respective device files instead of generic
> > > aml-build.c which was intended for basic AML primitives
> > > (it 's got polluted over time with device specific functions
> > > but that's not the reason to continue doing that).
> > >
> > > Also having those functions along with devices models
> > > goes along with self enumerating ACPI devices (currently
> > > it works for x86 PCI/ISA device but there is no reason
> > > that it can't work with other types as well when
> > > I get there)
> > >    
> > Thanks!, Igor. Let me add them to device specific files as per your
> > recommendation.
> just be careful and build test other targets (while disabling the rest)
> at least no to regress them due to build deps. (I'd pick 2 with ACPI
> support that use and not uses affected code) and 1 that  uses device
> model but doesn't use ACPI at all (if such exists)

Sunil is already aware of it but I'll also mention here since it seems relevant
to Igor's point.


This patch breaks i386-softmmu build:


FAILED: libqemu-i386-softmmu.fa.p/hw_i386_acpi-microvm.c.o
cc -m64 -mcx16 -Ilibqemu-i386-softmmu.fa.p -I. -I.. -Itarget/i386 -I../target/i386 -Iqapi -Itrace 
-Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include 
-I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g 
-fstack-protector-strong -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wundef -Wwrite-strings 
-Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration 
-Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self 
-Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined 
-Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs 
-Wno-shift-negative-value -Wno-psabi -isystem /home/danielhb/work/qemu/linux-headers -isystem 
linux-headers -iquote . -iquote /home/danielhb/work/qemu -iquote /home/danielhb/work/qemu/include 
-iquote /home/danielhb/work/qemu/host/include/x86_64 -iquote 
/home/danielhb/work/qemu/host/include/generic -iquote /home/danielhb/work/qemu/tcg/i386 -pthread 
-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv 
-fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H 
'-DCONFIG_TARGET="i386-softmmu-config-target.h"' 
'-DCONFIG_DEVICES="i386-softmmu-config-devices.h"' -MD -MQ 
libqemu-i386-softmmu.fa.p/hw_i386_acpi-microvm.c.o -MF 
libqemu-i386-softmmu.fa.p/hw_i386_acpi-microvm.c.o.d -o 
libqemu-i386-softmmu.fa.p/hw_i386_acpi-microvm.c.o -c ../hw/i386/acpi-microvm.c
../hw/i386/acpi-microvm.c:48:13: error: conflicting types for 
‘acpi_dsdt_add_virtio’; have ‘void(Aml *, MicrovmMachineState *)’
   48 | static void acpi_dsdt_add_virtio(Aml *scope,
      |             ^~~~~~~~~~~~~~~~~~~~
In file included from 
/home/danielhb/work/qemu/include/hw/acpi/acpi_aml_interface.h:5,
                 from ../hw/i386/acpi-microvm.c:29:
/home/danielhb/work/qemu/include/hw/acpi/aml-build.h:503:6: note: previous 
declaration of ‘acpi_dsdt_add_virtio’ with type ‘void(Aml *, const MemMapEntry 
*, uint32_t,  int)’ {aka ‘void(Aml *, const MemMapEntry *, unsigned int,  int)’}
  503 | void acpi_dsdt_add_virtio(Aml *scope, const MemMapEntry 
*virtio_mmio_memmap,
      |      ^~~~~~~~~~~~~~~~~~~~
[5/714] Compiling C object libqemu-i386-softmmu.fa.p/hw_i386_kvm_clock.c.o

This happens because the common 'acpi_dsdt_add_virtio' function matches a local
function with the same name in hw/i386/acpi-microvm.c. We would need to either
rename the shared helper or rename the local acpi-microvm function or do 
something
like Igor mentioned to avoid this name collision.


Thanks,

Daniel








> > Thanks!
> > Sunil