[Qemu-devel] Re: [Bochs-developers] [PATCH] Read additional ACPI tables from a VM

[Sebastian Herbszt]

Gleb Natapov wrote:

Just a quick review.

> diff --git a/bios/rombios32.c b/bios/rombios32.c
> index f6ce225..29fd40a 100644
> --- a/bios/rombios32.c
> +++ b/bios/rombios32.c
> @@ -455,6 +455,8 @@ unsigned long bios_table_end_addr;
> #define QEMU_CFG_SIGNATURE  0x00
> #define QEMU_CFG_ID         0x01
> #define QEMU_CFG_UUID       0x02
> +#define FW_CFG_ARCH_LOCAL     0x8000
> +#define FW_CFG_ACPI_TABLES  (FW_CFG_ARCH_LOCAL + 0)

No more QEMU_CFG prefix? Or is this different?

> int qemu_cfg_port;
>
> @@ -482,6 +484,27 @@ void qemu_cfg_read(uint8_t *buf, int len)
>     while (len--)
>         *(buf++) = inb(QEMU_CFG_DATA_PORT);
> }
> +
> +static int acpi_additional_tables(void)
> +{
> +    uint16_t cnt;
> +
> +    qemu_cfg_select(FW_CFG_ACPI_TABLES);
> +    qemu_cfg_read((uint8_t*)&cnt, sizeof(cnt));
> +
> +    return cnt;
> +}
> +
> +static int acpi_load_table(int i, uint32_t addr, uint16_t *len)
> +{
> +    qemu_cfg_read((uint8_t*)len, sizeof(*len));
> +
> +    if (!*len)
> +        return -1;
> +
> +    qemu_cfg_read((uint8_t*)addr, *len);
> +    return 0;
> +}
> #endif
>
> void init_smp_msrs(void)

Can you please diff against bochs cvs?

> @@ -1545,8 +1568,8 @@ void acpi_bios_init(void)
>     uint32_t hpet_addr;
> #endif
>     uint32_t base_addr, rsdt_addr, fadt_addr, addr, facs_addr, dsdt_addr, 
> ssdt_addr;
> -    uint32_t acpi_tables_size, madt_addr, madt_size;
> -    int i;
> +    uint32_t acpi_tables_size, madt_addr, madt_size, rsdt_size;
> +    int i, ext_tbl;

Might change to uint32_t or uint16_t and ext_tbl to something little more 
descriptive like external_tables.

>     /* reserve memory space for tables */
> #ifdef BX_USE_EBDA_TABLES
> @@ -1559,10 +1582,17 @@ void acpi_bios_init(void)
>     bios_table_cur_addr += sizeof(*rsdp);
> #endif
>
> +#ifdef BX_QEMU
> +    ext_tbl = acpi_additional_tables();
> +#else
> +    ext_tbl = 0;
> +#endif
> +
>     addr = base_addr = ram_size - ACPI_DATA_SIZE;
>     rsdt_addr = addr;
>     rsdt = (void *)(addr);
> -    addr += sizeof(*rsdt);
> +    rsdt_size = sizeof(*rsdt) + ext_tbl * sizeof(rsdt->table_offset_entry[0]);

Can use 4 instead of the sizeof() since its defined as a 32-bit address.

> +    addr += rsdt_size;
>
>     fadt_addr = addr;
>     fadt = (void *)(addr);
> @@ -1601,12 +1631,6 @@ void acpi_bios_init(void)
>     addr += sizeof(*hpet);
> #endif
>
> -    acpi_tables_size = addr - base_addr;
> -
> -    BX_INFO("ACPI tables: RSDP addr=0x%08lx ACPI DATA addr=0x%08lx 
> size=0x%x\n",
> -            (unsigned long)rsdp,
> -            (unsigned long)rsdt, acpi_tables_size);
> -
>     /* RSDP */
>     memset(rsdp, 0, sizeof(*rsdp));
>     memcpy(rsdp->signature, "RSD PTR ", 8);
> @@ -1618,17 +1642,6 @@ void acpi_bios_init(void)
>     rsdp->rsdt_physical_address = cpu_to_le32(rsdt_addr);
>     rsdp->checksum = acpi_checksum((void *)rsdp, 20);
>
> -    /* RSDT */
> -    memset(rsdt, 0, sizeof(*rsdt));
> -    rsdt->table_offset_entry[0] = cpu_to_le32(fadt_addr);
> -    rsdt->table_offset_entry[1] = cpu_to_le32(madt_addr);
> -    rsdt->table_offset_entry[2] = cpu_to_le32(ssdt_addr);
> -#ifdef BX_QEMU
> -    rsdt->table_offset_entry[3] = cpu_to_le32(hpet_addr);
> -#endif
> -    acpi_build_table_header((struct acpi_table_header *)rsdt,
> -                            "RSDT", sizeof(*rsdt), 1);
> -
>     /* FADT */
>     memset(fadt, 0, sizeof(*fadt));
>     fadt->firmware_ctrl = cpu_to_le32(facs_addr);
> @@ -1715,6 +1728,37 @@ void acpi_bios_init(void)
>                              "HPET", sizeof(*hpet), 1);
> #endif
>
> +

Stray empty line; might also unite it into one #ifdef #endif pair.

> +#ifdef BX_QEMU
> +    acpi_additional_tables(); /* resets cfg to required entry */
> +    for(i = 0; i < ext_tbl; i++) {
> +        uint16_t len;
> +        if(addr >= ram_size)
> +            BX_PANIC("ACPI table overflow\n");
> +        if(acpi_load_table(i, addr, &len) < 0)
> +            BX_PANIC("Fail to load ACPI table from QEMU\n");

Failed?

> +        rsdt->table_offset_entry[i+4] = cpu_to_le32(addr);
> +        addr += len;
> +    }
> +#endif
> +
> +    /* RSDT */
> +    memset(rsdt, 0, sizeof(*rsdt));

This overwrites the "rsdt->table_offset_entry[i+4] = cpu_to_le32(addr);" 
assignment from above?

> +    rsdt->table_offset_entry[0] = cpu_to_le32(fadt_addr);
> +    rsdt->table_offset_entry[1] = cpu_to_le32(madt_addr);
> +    rsdt->table_offset_entry[2] = cpu_to_le32(ssdt_addr);
> +#ifdef BX_QEMU
> +    rsdt->table_offset_entry[3] = cpu_to_le32(hpet_addr);
> +#endif
> +    acpi_build_table_header((struct acpi_table_header *)rsdt,
> +                            "RSDT", rsdt_size, 1);
> +
> +    acpi_tables_size = addr - base_addr;
> +
> +    BX_INFO("ACPI tables: RSDP addr=0x%08lx ACPI DATA addr=0x%08lx 
> size=0x%x\n",
> +            (unsigned long)rsdp,
> +            (unsigned long)rsdt, acpi_tables_size);
> +
> }
>
> /* SMBIOS entry point -- must be written to a 16-bit aligned address
> --

- Sebastian