Re: [PATCH V2 05/10] hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:33PM +0530, Sunil V L wrote:
> Add Multiple APIC Description Table (MADT) with the
> INTC structure for each cpu.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt-acpi-build.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 3c4da6c385..f54e3fb731 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -134,6 +134,43 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
> RISCVVirtState *s)
>      free_aml_allocator();
>  }
>  
> +/* MADT */
> +static void
> +build_madt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> +{
> +    MachineState *mc = MACHINE(s);

Please use 'ms' for MachineState and 'mc' for MachineClass

> +    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(mc); socket++) {
> +        base_hartid = riscv_socket_first_hartid(mc, 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);   /* hartid   */

The spec calls this field "Hart ID of the hart" (which is redundant), but
we should at least use "Hart ID" for the comment here. We want the text
in the comments to be directly searchable in the specs.

> +            build_append_int_noprefix(table_data, cpu_id, 4);  /* ACPI ID  */

This one should be "ACPI Processor UID"

> +            cpu_id++;
> +        }
> +    }
> +
> +    acpi_table_end(linker, &table);
> +}
> +
>  static void
>  virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>  {
> -- 
> 2.34.1
>

Thanks,
drew