| Message ID | 20230224083701.2657063-6-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add basic ACPI support for risc-v virt | expand |
On Fri, 24 Feb 2023 14:06:58 +0530 Sunil V L <sunilvl@ventanamicro.com> wrote: > Add Multiple APIC Description Table (MADT) with the > RINTC structure for each cpu. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > --- > hw/riscv/virt-acpi-build.c | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > index 3a5e2e6d53..8b85b34c55 100644 > --- a/hw/riscv/virt-acpi-build.c > +++ b/hw/riscv/virt-acpi-build.c > @@ -32,6 +32,7 @@ > #include "sysemu/reset.h" > #include "migration/vmstate.h" > #include "hw/riscv/virt.h" > +#include "hw/riscv/numa.h" > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, > free_aml_allocator(); > } > > +/* MADT */ see build_srat() how this comment must look like > +static void build_madt(GArray *table_data, > + BIOSLinker *linker, > + RISCVVirtState *s) > +{ > + MachineState *ms = MACHINE(s); > + int socket; > + uint16_t base_hartid = 0; > + uint32_t cpu_id = 0; > + > + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, > + .oem_table_id = s->oem_table_id }; > + > + acpi_table_begin(&table, table_data); > + /* Local Interrupt Controller Address */ > + build_append_int_noprefix(table_data, 0, 4); > + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ > + > + /* RISC-V Local INTC structures per HART */ > + for (socket = 0; socket < riscv_socket_count(ms); socket++) { > + base_hartid = riscv_socket_first_hartid(ms, socket); > + > + for (int i = 0; i < s->soc[socket].num_harts; i++) { > + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ > + build_append_int_noprefix(table_data, 20, 1); /* Length */ > + build_append_int_noprefix(table_data, 1, 1); /* Version */ > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > + build_append_int_noprefix(table_data, 1, 4); /* Flags */ > + build_append_int_noprefix(table_data, > + (base_hartid + i), 8); /* Hart ID */ > + > + /* ACPI Processor UID */ > + build_append_int_noprefix(table_data, cpu_id, 4); cpu_id here seems to be unrelated to one in DSDT. Could you explain how socket/hartid and cpu_id are related to each other? > + cpu_id++; > + } > + } > + > + acpi_table_end(linker, &table); > +} > + > static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables) > { > GArray *table_offsets; > @@ -153,6 +194,9 @@ static void 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); > + > /* XSDT is pointed to by RSDP */ > xsdt = tables_blob->len; > build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
Hi Igor, On Fri, Feb 24, 2023 at 01:53:43PM +0100, Igor Mammedov wrote: > On Fri, 24 Feb 2023 14:06:58 +0530 > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > Add Multiple APIC Description Table (MADT) with the > > RINTC structure for each cpu. > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > Acked-by: Alistair Francis <alistair.francis@wdc.com> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > hw/riscv/virt-acpi-build.c | 44 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > > index 3a5e2e6d53..8b85b34c55 100644 > > --- a/hw/riscv/virt-acpi-build.c > > +++ b/hw/riscv/virt-acpi-build.c > > @@ -32,6 +32,7 @@ > > #include "sysemu/reset.h" > > #include "migration/vmstate.h" > > #include "hw/riscv/virt.h" > > +#include "hw/riscv/numa.h" > > > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > > > @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, > > free_aml_allocator(); > > } > > > > +/* MADT */ > > see build_srat() how this comment must look like > Currently, even though ECRs are approved, the ACPI spec is not released for these MADT structures. I can add the spec version for the generic MADT but not for the RINTC. Same issue with a new table RHCT. What is the recommendation in such case? > > +static void build_madt(GArray *table_data, > > + BIOSLinker *linker, > > + RISCVVirtState *s) > > +{ > > + MachineState *ms = MACHINE(s); > > + int socket; > > + uint16_t base_hartid = 0; > > + uint32_t cpu_id = 0; > > + > > + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, > > + .oem_table_id = s->oem_table_id }; > > + > > + acpi_table_begin(&table, table_data); > > + /* Local Interrupt Controller Address */ > > + build_append_int_noprefix(table_data, 0, 4); > > + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ > > + > > + /* RISC-V Local INTC structures per HART */ > > + for (socket = 0; socket < riscv_socket_count(ms); socket++) { > > + base_hartid = riscv_socket_first_hartid(ms, socket); > > + > > + for (int i = 0; i < s->soc[socket].num_harts; i++) { > > + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ > > + build_append_int_noprefix(table_data, 20, 1); /* Length */ > > + build_append_int_noprefix(table_data, 1, 1); /* Version */ > > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > + build_append_int_noprefix(table_data, 1, 4); /* Flags */ > > + build_append_int_noprefix(table_data, > > + (base_hartid + i), 8); /* Hart ID */ > > + > > + /* ACPI Processor UID */ > > + build_append_int_noprefix(table_data, cpu_id, 4); > > cpu_id here seems to be unrelated to one in DSDT. > Could you explain how socket/hartid and cpu_id are related to each other? > cpu_id should match the _UID. I needed two loops here to get the base_hartid of the socket. hart_id is the unique ID for each hart similar to MPIDR / APIC ID. I understand your point. Let me make DSDT also created using two loops so that both match. Thank you very much for your review! Sunil
On Fri, 24 Feb 2023 19:56:58 +0530 Sunil V L <sunilvl@ventanamicro.com> wrote: > Hi Igor, > > On Fri, Feb 24, 2023 at 01:53:43PM +0100, Igor Mammedov wrote: > > On Fri, 24 Feb 2023 14:06:58 +0530 > > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > > > Add Multiple APIC Description Table (MADT) with the > > > RINTC structure for each cpu. > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > Acked-by: Alistair Francis <alistair.francis@wdc.com> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > hw/riscv/virt-acpi-build.c | 44 ++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 44 insertions(+) > > > > > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > > > index 3a5e2e6d53..8b85b34c55 100644 > > > --- a/hw/riscv/virt-acpi-build.c > > > +++ b/hw/riscv/virt-acpi-build.c > > > @@ -32,6 +32,7 @@ > > > #include "sysemu/reset.h" > > > #include "migration/vmstate.h" > > > #include "hw/riscv/virt.h" > > > +#include "hw/riscv/numa.h" > > > > > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > > > > > @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, > > > free_aml_allocator(); > > > } > > > > > > +/* MADT */ > > > > see build_srat() how this comment must look like > > > Currently, even though ECRs are approved, the ACPI spec is not released > for these MADT structures. I can add the spec version for the generic > MADT but not for the RINTC. Same issue with a new table RHCT. > What is the recommendation in such case? ther must be some draft variant of spec or a ticket where it was approved or a reference impl. somewhere. > > > > +static void build_madt(GArray *table_data, > > > + BIOSLinker *linker, > > > + RISCVVirtState *s) > > > +{ > > > + MachineState *ms = MACHINE(s); > > > + int socket; > > > + uint16_t base_hartid = 0; > > > + uint32_t cpu_id = 0; > > > + > > > + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, > > > + .oem_table_id = s->oem_table_id }; > > > + > > > + acpi_table_begin(&table, table_data); > > > + /* Local Interrupt Controller Address */ > > > + build_append_int_noprefix(table_data, 0, 4); > > > + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ > > > + > > > + /* RISC-V Local INTC structures per HART */ > > > + for (socket = 0; socket < riscv_socket_count(ms); socket++) { > > > + base_hartid = riscv_socket_first_hartid(ms, socket); > > > + > > > + for (int i = 0; i < s->soc[socket].num_harts; i++) { > > > + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ > > > + build_append_int_noprefix(table_data, 20, 1); /* Length */ > > > + build_append_int_noprefix(table_data, 1, 1); /* Version */ > > > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > > + build_append_int_noprefix(table_data, 1, 4); /* Flags */ > > > + build_append_int_noprefix(table_data, > > > + (base_hartid + i), 8); /* Hart ID */ > > > + > > > + /* ACPI Processor UID */ > > > + build_append_int_noprefix(table_data, cpu_id, 4); > > > > cpu_id here seems to be unrelated to one in DSDT. > > Could you explain how socket/hartid and cpu_id are related to each other? > > > cpu_id should match the _UID. I needed two loops here to get the > base_hartid of the socket. hart_id is the unique ID for each hart > similar to MPIDR / APIC ID. I understand your point. Let me make DSDT > also created using two loops so that both match. Why not reuse possible CPUs to describe topology there and then use ids from it to build ACPI tables instead of inventing your own cpu topo all over the place? PS: look for possible_cpus and how it's used (virt-arm already uses it although partially). And I'd like to avoid adding new ad-hoc ways to describe CPU topology is current possible_cpu ca do the job. > Thank you very much for your review! > Sunil >
On Mon, Feb 27, 2023 at 04:41:21PM +0100, Igor Mammedov wrote: > On Fri, 24 Feb 2023 19:56:58 +0530 > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > Hi Igor, > > > > On Fri, Feb 24, 2023 at 01:53:43PM +0100, Igor Mammedov wrote: > > > On Fri, 24 Feb 2023 14:06:58 +0530 > > > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > > > > > Add Multiple APIC Description Table (MADT) with the > > > > RINTC structure for each cpu. > > > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > > Acked-by: Alistair Francis <alistair.francis@wdc.com> > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > > --- > > > > hw/riscv/virt-acpi-build.c | 44 ++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 44 insertions(+) > > > > > > > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > > > > index 3a5e2e6d53..8b85b34c55 100644 > > > > --- a/hw/riscv/virt-acpi-build.c > > > > +++ b/hw/riscv/virt-acpi-build.c > > > > @@ -32,6 +32,7 @@ > > > > #include "sysemu/reset.h" > > > > #include "migration/vmstate.h" > > > > #include "hw/riscv/virt.h" > > > > +#include "hw/riscv/numa.h" > > > > > > > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > > > > > > > @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, > > > > free_aml_allocator(); > > > > } > > > > > > > > +/* MADT */ > > > > > > see build_srat() how this comment must look like > > > > > Currently, even though ECRs are approved, the ACPI spec is not released > > for these MADT structures. I can add the spec version for the generic > > MADT but not for the RINTC. Same issue with a new table RHCT. > > What is the recommendation in such case? > > ther must be some draft variant of spec or a ticket where it was approved > or a reference impl. somewhere. > Sure, I can add the ticket ID. Thanks! > > > > > > +static void build_madt(GArray *table_data, > > > > + BIOSLinker *linker, > > > > + RISCVVirtState *s) > > > > +{ > > > > + MachineState *ms = MACHINE(s); > > > > + int socket; > > > > + uint16_t base_hartid = 0; > > > > + uint32_t cpu_id = 0; > > > > + > > > > + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, > > > > + .oem_table_id = s->oem_table_id }; > > > > + > > > > + acpi_table_begin(&table, table_data); > > > > + /* Local Interrupt Controller Address */ > > > > + build_append_int_noprefix(table_data, 0, 4); > > > > + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ > > > > + > > > > + /* RISC-V Local INTC structures per HART */ > > > > + for (socket = 0; socket < riscv_socket_count(ms); socket++) { > > > > + base_hartid = riscv_socket_first_hartid(ms, socket); > > > > + > > > > + for (int i = 0; i < s->soc[socket].num_harts; i++) { > > > > + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ > > > > + build_append_int_noprefix(table_data, 20, 1); /* Length */ > > > > + build_append_int_noprefix(table_data, 1, 1); /* Version */ > > > > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > > > + build_append_int_noprefix(table_data, 1, 4); /* Flags */ > > > > + build_append_int_noprefix(table_data, > > > > + (base_hartid + i), 8); /* Hart ID */ > > > > + > > > > + /* ACPI Processor UID */ > > > > + build_append_int_noprefix(table_data, cpu_id, 4); > > > > > > cpu_id here seems to be unrelated to one in DSDT. > > > Could you explain how socket/hartid and cpu_id are related to each other? > > > > > cpu_id should match the _UID. I needed two loops here to get the > > base_hartid of the socket. hart_id is the unique ID for each hart > > similar to MPIDR / APIC ID. I understand your point. Let me make DSDT > > also created using two loops so that both match. > > Why not reuse possible CPUs to describe topology there and then > use ids from it to build ACPI tables instead of inventing your own > cpu topo all over the place? > > PS: look for possible_cpus and how it's used (virt-arm already uses it > although partially). And I'd like to avoid adding new ad-hoc ways > to describe CPU topology is current possible_cpu ca do the job. Okay, sure. Let me take a look at possible_cpus and use in cpu topology. Thanks!
On Tue, 28 Feb 2023 13:04:36 +0530 Sunil V L <sunilvl@ventanamicro.com> wrote: > On Mon, Feb 27, 2023 at 04:41:21PM +0100, Igor Mammedov wrote: > > On Fri, 24 Feb 2023 19:56:58 +0530 > > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > > > Hi Igor, > > > > > > On Fri, Feb 24, 2023 at 01:53:43PM +0100, Igor Mammedov wrote: > > > > On Fri, 24 Feb 2023 14:06:58 +0530 > > > > Sunil V L <sunilvl@ventanamicro.com> wrote: > > > > > > > > > Add Multiple APIC Description Table (MADT) with the > > > > > RINTC structure for each cpu. > > > > > > > > > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > > > > > Acked-by: Alistair Francis <alistair.francis@wdc.com> > > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > > > --- > > > > > hw/riscv/virt-acpi-build.c | 44 ++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 44 insertions(+) > > > > > > > > > > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c > > > > > index 3a5e2e6d53..8b85b34c55 100644 > > > > > --- a/hw/riscv/virt-acpi-build.c > > > > > +++ b/hw/riscv/virt-acpi-build.c > > > > > @@ -32,6 +32,7 @@ > > > > > #include "sysemu/reset.h" > > > > > #include "migration/vmstate.h" > > > > > #include "hw/riscv/virt.h" > > > > > +#include "hw/riscv/numa.h" > > > > > > > > > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > > > > > > > > > @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, > > > > > free_aml_allocator(); > > > > > } > > > > > > > > > > +/* MADT */ > > > > > > > > see build_srat() how this comment must look like > > > > > > > Currently, even though ECRs are approved, the ACPI spec is not released > > > for these MADT structures. I can add the spec version for the generic > > > MADT but not for the RINTC. Same issue with a new table RHCT. > > > What is the recommendation in such case? > > > > ther must be some draft variant of spec or a ticket where it was approved > > or a reference impl. somewhere. > > > Sure, I can add the ticket ID. Thanks! and a link, later when new spec is published we can update it to rev/chapter format. > > > > > > > > > +static void build_madt(GArray *table_data, > > > > > + BIOSLinker *linker, > > > > > + RISCVVirtState *s) > > > > > +{ > > > > > + MachineState *ms = MACHINE(s); > > > > > + int socket; > > > > > + uint16_t base_hartid = 0; > > > > > + uint32_t cpu_id = 0; > > > > > + > > > > > + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, > > > > > + .oem_table_id = s->oem_table_id }; > > > > > + > > > > > + acpi_table_begin(&table, table_data); > > > > > + /* Local Interrupt Controller Address */ > > > > > + build_append_int_noprefix(table_data, 0, 4); > > > > > + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ > > > > > + > > > > > + /* RISC-V Local INTC structures per HART */ > > > > > + for (socket = 0; socket < riscv_socket_count(ms); socket++) { > > > > > + base_hartid = riscv_socket_first_hartid(ms, socket); > > > > > + > > > > > + for (int i = 0; i < s->soc[socket].num_harts; i++) { > > > > > + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ > > > > > + build_append_int_noprefix(table_data, 20, 1); /* Length */ > > > > > + build_append_int_noprefix(table_data, 1, 1); /* Version */ > > > > > + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ > > > > > + build_append_int_noprefix(table_data, 1, 4); /* Flags */ > > > > > + build_append_int_noprefix(table_data, > > > > > + (base_hartid + i), 8); /* Hart ID */ > > > > > + > > > > > + /* ACPI Processor UID */ > > > > > + build_append_int_noprefix(table_data, cpu_id, 4); > > > > > > > > cpu_id here seems to be unrelated to one in DSDT. > > > > Could you explain how socket/hartid and cpu_id are related to each other? > > > > > > > cpu_id should match the _UID. I needed two loops here to get the > > > base_hartid of the socket. hart_id is the unique ID for each hart > > > similar to MPIDR / APIC ID. I understand your point. Let me make DSDT > > > also created using two loops so that both match. > > > > Why not reuse possible CPUs to describe topology there and then > > use ids from it to build ACPI tables instead of inventing your own > > cpu topo all over the place? > > > > PS: look for possible_cpus and how it's used (virt-arm already uses it > > although partially). And I'd like to avoid adding new ad-hoc ways > > to describe CPU topology is current possible_cpu ca do the job. > > Okay, sure. Let me take a look at possible_cpus and use in cpu topology. > Thanks! maybe following could be of help: hw/i386/acpi-common.c:acpi_build_madt and see how madt_cpu is used.
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c index 3a5e2e6d53..8b85b34c55 100644 --- a/hw/riscv/virt-acpi-build.c +++ b/hw/riscv/virt-acpi-build.c @@ -32,6 +32,7 @@ #include "sysemu/reset.h" #include "migration/vmstate.h" #include "hw/riscv/virt.h" +#include "hw/riscv/numa.h" #define ACPI_BUILD_TABLE_SIZE 0x20000 @@ -132,6 +133,46 @@ static void build_dsdt(GArray *table_data, free_aml_allocator(); } +/* MADT */ +static void build_madt(GArray *table_data, + BIOSLinker *linker, + RISCVVirtState *s) +{ + MachineState *ms = MACHINE(s); + int socket; + uint16_t base_hartid = 0; + uint32_t cpu_id = 0; + + AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id, + .oem_table_id = s->oem_table_id }; + + acpi_table_begin(&table, table_data); + /* Local Interrupt Controller Address */ + build_append_int_noprefix(table_data, 0, 4); + build_append_int_noprefix(table_data, 0, 4); /* MADT Flags */ + + /* RISC-V Local INTC structures per HART */ + for (socket = 0; socket < riscv_socket_count(ms); socket++) { + base_hartid = riscv_socket_first_hartid(ms, socket); + + for (int i = 0; i < s->soc[socket].num_harts; i++) { + build_append_int_noprefix(table_data, 0x18, 1); /* Type */ + build_append_int_noprefix(table_data, 20, 1); /* Length */ + build_append_int_noprefix(table_data, 1, 1); /* Version */ + build_append_int_noprefix(table_data, 0, 1); /* Reserved */ + build_append_int_noprefix(table_data, 1, 4); /* Flags */ + build_append_int_noprefix(table_data, + (base_hartid + i), 8); /* Hart ID */ + + /* ACPI Processor UID */ + build_append_int_noprefix(table_data, cpu_id, 4); + cpu_id++; + } + } + + acpi_table_end(linker, &table); +} + static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables) { GArray *table_offsets; @@ -153,6 +194,9 @@ static void 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); + /* XSDT is pointed to by RSDP */ xsdt = tables_blob->len; build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,