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

[Sunil V L]

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