From: Sunil V L <sunilvl@ventanamicro.com>
To: Bin Meng <bmeng.cn@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
Alistair Francis <alistair.francis@wdc.com>,
Bin Meng <bin.meng@windriver.com>,
qemu-riscv@nongnu.org, qemu-devel@nongnu.org,
Andrew Jones <ajones@ventanamicro.com>,
Anup Patel <apatel@ventanamicro.com>,
Atish Kumar Patra <atishp@rivosinc.com>
Subject: Re: [PATCH 04/10] hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables
Date: Tue, 7 Feb 2023 23:45:25 +0530 [thread overview]
Message-ID: <Y+KVPaVQPcoItBbV@sunil-laptop> (raw)
In-Reply-To: <CAEUhbmU2cofq767-9Lfs2CF+DzpZ6FCHz1ox1TC1siPs5uUdWQ@mail.gmail.com>
On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> > >
> > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > some refactoring work is needed before ACPI support fully lands on
> > > RISC-V.
> > > For example, we can extract the common part among x86/arm/riscv into a
> > > separate file, like hw/acpi/acpi-build.c?
> > >
> >
> > While it appears like there is same code in multiple places, those
> > functions take architecture specific MachineState parameter. Some tables
> > like MADT though with same name, will have different contents for
> > different architectures.
> >
> > Only one function which Daniel also pointed is the wrapper
> > acpi_align_size() which can be made common. I am not
> > sure whether it is worth of refactoring.
> >
>
> It's more than that. For example,
>
> With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> of cpus, so there is no need to pass the architecture specific
> MachineState as the parameter.
>
I would not make this function common. The reason is, an architecture may
choose to attach different ACPI objects under the CPU node.
> Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
>
The issue is, these things are not exactly common across all architectures.
x86 has bit different data in these objects. While today it appears they
are same for riscv and arm, in future things may change for an architecture.
It doesn't look like it is a standard practice to build files under
hw/acpi for specific architectures. Hence, IMO, it is better to keep these
things in architecture specific folders to allow them to do differently in
future.
But I look forward for the feedback from other architecture maintainers on
this topic. My experience in qemu is very limited. So, I need help from
experts.
Thanks!
Sunil
next prev parent reply other threads:[~2023-02-07 18:16 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-02 4:52 [PATCH 00/10] Add basic ACPI support for risc-v virt Sunil V L
2023-02-02 4:52 ` [PATCH 01/10] hw/riscv/virt: Add OEM_ID and OEM_TABLE_ID fields Sunil V L
2023-02-06 5:49 ` Bin Meng
2023-02-09 0:17 ` Alistair Francis
2023-02-02 4:52 ` [PATCH 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI Sunil V L
2023-02-06 9:43 ` Bin Meng
2023-02-06 10:54 ` Andrea Bolognani
2023-02-06 11:18 ` Philippe Mathieu-Daudé
2023-02-06 12:35 ` Andrew Jones
2023-02-06 13:04 ` Sunil V L
2023-02-07 3:57 ` Bin Meng
2023-02-07 5:37 ` Andrew Jones
2023-02-07 12:33 ` Sunil V L
2023-02-06 12:56 ` Gerd Hoffmann
2023-02-07 8:50 ` Philippe Mathieu-Daudé
2023-02-07 10:12 ` Sunil V L
2023-02-07 10:14 ` Andrew Jones
2023-02-06 23:14 ` Bernhard Beschow
2023-02-07 10:23 ` Andrew Jones
2023-02-07 13:56 ` Andrea Bolognani
2023-02-07 14:02 ` Thomas Huth
2023-02-07 14:38 ` Andrea Bolognani
2023-02-07 19:20 ` Andrew Jones
2023-02-08 16:48 ` Andrea Bolognani
2023-02-09 5:15 ` Thomas Huth
2023-02-02 4:52 ` [PATCH 03/10] hw/riscv/virt: Add memmap pointer to RiscVVirtState Sunil V L
2023-02-06 9:50 ` Bin Meng
2023-02-09 0:18 ` Alistair Francis
2023-02-02 4:52 ` [PATCH 04/10] hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables Sunil V L
2023-02-06 10:17 ` Bin Meng
2023-02-06 13:24 ` Sunil V L
2023-02-07 16:10 ` Bin Meng
2023-02-07 18:15 ` Sunil V L [this message]
2023-02-08 1:06 ` Bin Meng
2023-02-08 4:49 ` Sunil V L
2023-02-24 16:14 ` Igor Mammedov
2023-02-02 4:52 ` [PATCH 05/10] hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT Sunil V L
2023-02-09 0:21 ` Alistair Francis
2023-02-02 4:52 ` [PATCH 06/10] hw/riscv/virt: virt-acpi-build.c: Add RHCT Table Sunil V L
2023-02-02 4:52 ` [PATCH 07/10] hw/riscv: meson.build: Build virt-acpi-build.c Sunil V L
2023-02-06 10:26 ` Bin Meng
2023-02-02 4:52 ` [PATCH 08/10] hw/riscv/Kconfig: virt: Enable ACPI config options Sunil V L
2023-02-06 10:29 ` Bin Meng
2023-02-06 14:19 ` Sunil V L
2023-02-02 4:52 ` [PATCH 09/10] hw/riscv/virt.c: Initialize the ACPI tables Sunil V L
2023-02-06 10:32 ` Bin Meng
2023-02-02 4:52 ` [PATCH 10/10] MAINTAINERS: Add entry for RISC-V ACPI Sunil V L
2023-02-06 10:33 ` Bin Meng
2023-02-09 0:23 ` Alistair Francis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y+KVPaVQPcoItBbV@sunil-laptop \
--to=sunilvl@ventanamicro.com \
--cc=ajones@ventanamicro.com \
--cc=alistair.francis@wdc.com \
--cc=apatel@ventanamicro.com \
--cc=atishp@rivosinc.com \
--cc=bin.meng@windriver.com \
--cc=bmeng.cn@gmail.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).