Re: [PATCH 2/2] xen/x86/pvh: copy ACPI tables to Dom0 instead of mapping

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 12, 2023 at 06:17:20PM -0700, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> Mapping the ACPI tables to Dom0 PVH 1:1 leads to memory corruptions of
> the tables in the guest. Instead, copy the tables to Dom0.
> 
> This is a workaround.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
> As mentioned in the cover letter, this is a RFC workaround as I don't
> know the cause of the underlying problem. I do know that this patch
> solves what would be otherwise a hang at boot when Dom0 PVH attempts to
> parse ACPI tables.

I'm unsure how safe this is for native systems, as it's possible for
firmware to modify the data in the tables, so copying them would
break that functionality.

I think we need to get to the root cause that triggers this behavior
on QEMU.  Is it the table checksum that fail, or something else?  Is
there an error from Linux you could reference?

I've got some feedback below, but I'm unsure copying is the correct
approach.

> ---
>  xen/arch/x86/hvm/dom0_build.c | 107 +++++++++-------------------------
>  1 file changed, 27 insertions(+), 80 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 5fde769863..a6037fc6ed 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -73,32 +73,6 @@ static void __init print_order_stats(const struct domain 
> *d)
>              printk("order %2u allocations: %u\n", i, order_stats[i]);
>  }
>  
> -static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
> -                                       unsigned long nr_pages, const bool 
> map)
> -{
> -    int rc;
> -
> -    for ( ; ; )
> -    {
> -        rc = map ?   map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
> -                 : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
> -        if ( rc == 0 )
> -            break;
> -        if ( rc < 0 )
> -        {
> -            printk(XENLOG_WARNING
> -                   "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n",
> -                   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> -            break;
> -        }
> -        nr_pages -= rc;
> -        pfn += rc;
> -        process_pending_softirqs();
> -    }
> -
> -    return rc;
> -}
> -
>  /* Populate a HVM memory range using the biggest possible order. */
>  static int __init pvh_populate_memory_range(struct domain *d,
>                                              unsigned long start,
> @@ -967,6 +941,8 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, 
> paddr_t madt_addr,
>      unsigned long size = sizeof(*xsdt);
>      unsigned int i, j, num_tables = 0;
>      int rc;
> +    struct acpi_table_fadt fadt;
> +    unsigned long fadt_addr = 0, dsdt_addr = 0, facs_addr = 0, fadt_size = 0;

paddr_t and size_t would be better.

>      struct acpi_table_header header = {
>          .signature    = "XSDT",
>          .length       = sizeof(struct acpi_table_header),
> @@ -1013,10 +989,33 @@ static int __init pvh_setup_acpi_xsdt(struct domain 
> *d, paddr_t madt_addr,
>      /* Copy the addresses of the rest of the allowed tables. */
>      for( i = 0, j = 1; i < acpi_gbl_root_table_list.count; i++ )
>      {
> +        void *table;

const __iomem.

> +
> +        pvh_steal_ram(d, tables[i].length, 0, GB(4), addr);
> +        table = acpi_os_map_memory(tables[i].address, tables[i].length);
> +        hvm_copy_to_guest_phys(*addr, table, tables[i].length, d->vcpu[0]);
> +        pvh_add_mem_range(d, *addr, *addr + tables[i].length, E820_ACPI);

Need to check for errors in the calls above.

> +
> +        if ( !strncmp(tables[i].signature.ascii, ACPI_SIG_FADT, 
> ACPI_NAME_SIZE) )
> +        {
> +            memcpy(&fadt, table, tables[i].length);
> +            fadt_addr = *addr;
> +            fadt_size = tables[i].length;
> +        }
> +        else if ( !strncmp(tables[i].signature.ascii, ACPI_SIG_DSDT, 
> ACPI_NAME_SIZE) )
> +                dsdt_addr = *addr;
> +        else if ( !strncmp(tables[i].signature.ascii, ACPI_SIG_FACS, 
> ACPI_NAME_SIZE) )
> +                facs_addr = *addr;

Wrong indentation.

> +
>          if ( pvh_acpi_xsdt_table_allowed(tables[i].signature.ascii,
> -                                         tables[i].address, 
> tables[i].length) )
> -            xsdt->table_offset_entry[j++] = tables[i].address;
> +                    tables[i].address, tables[i].length) )

Unrelated withe space adjustment?

Thanks, Roger.