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

["Sebastian Herbszt" <herbszt@gmx.de>]

Gleb Natapov wrote:
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>

Looks good.

> diff --git a/bios/rombios32.c b/bios/rombios32.c
> index 3269be5..3b8085f 100644
> --- a/bios/rombios32.c
> +++ b/bios/rombios32.c
> @@ -457,6 +457,8 @@ void wrmsr_smp(uint32_t index, uint64_t val)
> #define QEMU_CFG_SIGNATURE  0x00
> #define QEMU_CFG_ID         0x01
> #define QEMU_CFG_UUID       0x02
> +#define QEMU_CFG_ARCH_LOCAL     0x8000
> +#define QEMU_CFG_ACPI_TABLES  (QEMU_CFG_ARCH_LOCAL + 0)
> 
> int qemu_cfg_port;
> 
> @@ -484,6 +486,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(QEMU_CFG_ACPI_TABLES);
> +    qemu_cfg_read((uint8_t*)&cnt, sizeof(cnt));
> +
> +    return cnt;
> +}

Change "int" to "uint16_t", e.g. "static uint16_t acpi_additional_tables(void)" ?

> +
> +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 uuid_probe(void)
> @@ -1534,8 +1557,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;
> +    uint16_t i, external_tables;
> 
>     /* reserve memory space for tables */
> #ifdef BX_USE_EBDA_TABLES
> @@ -1548,10 +1571,17 @@ void acpi_bios_init(void)
>     bios_table_cur_addr += sizeof(*rsdp);
> #endif
> 
> +#ifdef BX_QEMU
> +    external_tables = acpi_additional_tables();
> +#else
> +    external_tables = 0;
> +#endif
> +
>     addr = base_addr = ram_size - ACPI_DATA_SIZE;
>     rsdt_addr = addr;
>     rsdt = (void *)(addr);
> -    addr += sizeof(*rsdt);
> +    rsdt_size = sizeof(*rsdt) + external_tables * 4;
> +    addr += rsdt_size;
> 
>     fadt_addr = addr;
>     fadt = (void *)(addr);
> @@ -1590,12 +1620,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);
> @@ -1607,17 +1631,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);
> @@ -1692,6 +1705,7 @@ void acpi_bios_init(void)
>                                 "APIC", madt_size, 1);
>     }
> 
> +    memset(rsdt, 0, rsdt_size);

I missed that sizeof(rsdt_descriptor_rev1) did not cover the whole rsdt.
Moving memset() up here and using rsdt_size seems reasonable / less confusing.

> #ifdef BX_QEMU
>     /* HPET */
>     memset(hpet, 0, sizeof(*hpet));
> @@ -1702,7 +1716,34 @@ void acpi_bios_init(void)
>     hpet->addr.address = cpu_to_le32(ACPI_HPET_ADDRESS);
>     acpi_build_table_header((struct  acpi_table_header *)hpet,
>                              "HPET", sizeof(*hpet), 1);
> +
> +    acpi_additional_tables(); /* resets cfg to required entry */
> +    for(i = 0; i < external_tables; i++) {
> +        uint16_t len;
> +        if(addr >= ram_size)
> +            BX_PANIC("ACPI table overflow\n");
> +        if(acpi_load_table(i, addr, &len) < 0)
> +            BX_PANIC("Failed to load ACPI table from QEMU\n");
> +        rsdt->table_offset_entry[i+4] = cpu_to_le32(addr);
> +        addr += len;
> +    }

Add another check for "addr >= ram_size" here? Or move the check after
acpi_load_table()? Since its not possible to check for "addr + len >= ram_size" before
acpi_load_table() this can still overflow and won't be checked for.

> +#endif
> +
> +    /* 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", 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);
> 
> }

- Sebastian