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

[Sunil V L]

On Wed, Aug 16, 2023 at 03:51:58PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> 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.
> 
Thanks! Daniel. Yes, I am aware of the issue and will fix it along with
Igor's suggestion. I need to fix this irrespective of the approach.

Thanks,
Sunil
> 
> 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
> > > 
> >