Re: [PATCH V2 04/10] hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables

[Bin Meng]

On Tue, Feb 14, 2023 at 11:43 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> > Sunil,
> >
> > This patch is a bit confusing to me. You're using functions that doesn't 
> > exist
> > in the code base yet (build_madt and build_rhct) because they are introduced
> > in later patches. This also means that this patch is not being compiled 
> > tested,
> > because otherwise it would throw a compile error. And the build of the file 
> > only
> > happens after patch 8.
> >
> My intention was to add the caller also in the same patch where the
> function is added. I think I missed it when I split. Thanks!
>
> > Now, there is no hard rule in QEMU that dictates that every patch must be 
> > compile
> > tested, but nevertheless this is a good rule to follow that makes our lives 
> > easier
> > when bisecting and cherry-pick individual patches.
> >
> > My suggestion for this patch is:
> >
> > - squash both patches 7 and 8 into this patch to allow the file to be built;
> >
> Sure.
>
> > - remove the code that is referring to stuff that you haven't add yet:
> >
> > $ git diff
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > index 3c4da6c385..eb17029b64 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables 
> > *tables)
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> > -    acpi_add_table(table_offsets, tables_blob);
> > -    build_madt(tables_blob, tables->linker, s);
> > -
> > -    acpi_add_table(table_offsets, tables_blob);
> > -    build_rhct(tables_blob, tables->linker, s);
> > -
> >      /* XSDT is pointed to by RSDP */
> >      xsdt = tables_blob->len;
> >      build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
> >
> >
> > - in patch 5, add back the lines in virt_acpi_build() that uses 
> > build_madt();
> >
> > - in patch 6, add back the lines in virt_acpi_build() that uses 
> > build_rhct();
> >
> >
> > This will make this patch to be compiled and built right away without 
> > interfering with
> > the end result of the series.
> >
> Thanks!
>
> >
> > One more suggestion:
> >
> >
> > On 2/13/23 11:40, Sunil V L wrote:
> > > Add few basic ACPI tables and DSDT with few devices in a
> > > new file virt-acpi-build.c.
> > >
> > > These are mostly leveraged from arm64.
> >
> > Quick rant that you've already heard: I don't really understand why there 
> > is so
> > much ACPI code duplication everywhere. I really don't. E.g. 
> > acpi_align_size() is
> > copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
> > hw/loongarch/acpi-build.c. I don't see why we can't have a common file in 
> > hw/acpi
> > with helpers and so on that every ACPI architecture can use, and then the
> > individual drivers for each arch can have its own magic sauce.
> >

Yes, I had the same concern with Daniel when looking at the v1 series.

I am hoping the ACPI maintainers could make a decision, whether we
allow another architecture to do the duplication in their arch tree,
or we spend some time refactoring the ACPI table generation stuff.

+Michael and Igor.

> I completely agree that we better avoid duplication But I am bit
> hesitant in this case because,
> 1) Low level functions which help in creating the namespace/static
> tables are already common (ex: aml_append)
>
> 2) Using these basic routines, individual platforms can create the
> namespace with appropriate devices and the methods.
>
> ACPI name space is tightly coupled with a platform. While there may be
> common devices like processors, uart etc, there can be difference in the
> ACPI methods for each of those devices. For ex: CPU objects for one
> platform may support _LPI method. uart may support _DSD for one platform
> and other may use totally different UART. If we have to create common 
> routines,
> we will have to decide on all parameters the common function would need for
> different platforms. Same concern with fw_cfg/virtio etc which look same
> now but in future one platform can add a new method for these devices.
>
> IMHO, even though it looks like we have the same function in each architecture
> currently, this model allows us to have platforms with different devices with
> different methods/features. Creating common routines now would probably make
> them difficult to use in future.
>
> acpi_align_size() is a simple wrapper. We don't need it if we directly
> use the common function.
>
> Since I see insistence let me try moving few functions like fw_cfg (virtio, 
> pci in
> future) to a common file in hw/acpi.
>
> > All this said, instead of mentioning "this is mostly leveraged from arm64", 
> > you
> > can make a brief summary of the changes you've done from the arm64 version. 
> > This
> > will help guide the review into focusing on the novel code you're adding and
> > ignore the copied bits.
> >
> Sure.

Regards,
Bin