On Tue, Feb 14, 2023 at 11:43 AM Sunil V L <suni...@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
