Re: [PATCH V4 5/8] hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT

[Igor Mammedov]

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
>