Re: [Xen-devel] [PATCH v2 18/30] xen/x86: setup PVHv2 Dom0 ACPI tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.16 at 17:35, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Oct 06, 2016 at 09:40:50AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
>> > +static void __init acpi_zap_table_signature(char *name)
>> > +{
>> > +    struct acpi_table_header *table;
>> > +    acpi_status status;
>> > +    union {
>> > +        char str[ACPI_NAME_SIZE];
>> > +        uint32_t bits;
>> > +    } signature;
>> > +    char tmp;
>> > +    int i;
>> > +
>> > +    status = acpi_get_table(name, 0, &table);
>> > +    if ( ACPI_SUCCESS(status) )
>> > +    {
>> > +        memcpy(&signature.str[0], &table->signature[0], ACPI_NAME_SIZE);
>> > +        for ( i = 0; i < ACPI_NAME_SIZE / 2; i++ )
>> > +        {
>> > +            tmp = signature.str[ACPI_NAME_SIZE - i - 1];
>> > +            signature.str[ACPI_NAME_SIZE - i - 1] = signature.str[i];
>> > +            signature.str[i] = tmp;
>> > +        }
>> > +        write_atomic((uint32_t*)&table->signature[0], signature.bits);
>> > +    }
>> > +}
>> 
>> Together with moving MADT elsewhere we should also move
>> XSDT/RSDT, at which point we can simply avoid copying the
>> pointers for tables we don't want Dom0 to see (and down the
>> road we also have the option of adding tables).
>> 
>> The downside to both approaches is that this once again is a
>> black-listing model, i.e. new structure types we may need to
>> also suppress will remain visible to Dom0 until a patched
>> hypervisor would become available.
> 
> Maybe we could do whitelisting instead? I can see that at least the 
> following tables should be available to Dom0 if present, but TBH, it's hard 
> to tell:

Taking an abstract perspective I agree with Andrew that we should
be whitelisting here. However, as you already see from the list you
provided (which afaict is far from complete wrt ACPI 6.1), this may
become cumbersome already initially, not to speak of down the road.

>> > +    for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
>> > +    {
>> > +        pfn = PFN_DOWN(acpi_gbl_root_table_list.tables[i].address);
>> > +        nr_pages = DIV_ROUND_UP(acpi_gbl_root_table_list.tables[i].length,
>> > +                                PAGE_SIZE);
>> > +        rc = modify_mmio_11(d, pfn, nr_pages, true);
>> > +        if ( rc )
>> > +        {
>> > +            printk(
>> > +                "Failed to map ACPI region %#lx - %#lx into Dom0 memory 
>> > map\n",
>> > +                   pfn, pfn + nr_pages);
>> > +            return rc;
>> > +        }
>> > +    }
>> > +
>> > +    /*
>> > +     * Special treatment for memory < 1MB:
>> > +     *  - Copy the data in e820 regions marked as RAM (BDA, EBDA...).
>> 
>> Copy? What if some of it needs to get modified to interact with
>> firmware? I think you'd be better off mapping everything Xen
>> doesn't use into Dom0, and only mapping fresh RAM pages
>> over regions Xen does use (namely the trampoline).
> 
> Hm, that was my first approach (mapping the whole first MB into Dom0), but 
> sadly it doesn't seem to work because FreeBSD at least places the AP boot 
> trampoline there, and since APs are launched in 16b mode, the emulator 
> cannot handle executing memory from MMIO regions and crashes the domain. 
> That's why I had to map and copy data from RAM regions below 1MB. The BDA 
> it's all static data AFAICT, and the EBDA should reside in a reserved 
> region (or at least does on my systems).

I'm afraid there are systems where the EBDA is not marked reserved.
But maybe there are no current (64-bit capable) ones of that sort.

> Might it be possible to solve this by identity mapping the first 1MB, and 
> marking the RAM regions there as p2m_ram_rw? Or would that create even 
> further problems?

Hmm, not sure - the range below 1Mb is marked as MMIO in
frame_table[], so there would be a (benign?) conflict at least there.

>> > +    io_apic = (struct acpi_madt_io_apic *)(madt + 1);
>> > +    io_apic->header.type = ACPI_MADT_TYPE_IO_APIC;
>> > +    io_apic->header.length = sizeof(*io_apic);
>> > +    io_apic->id = 1;
>> > +    io_apic->address = VIOAPIC_DEFAULT_BASE_ADDRESS;
>> 
>> I think you need to make as many IO-APICs available to Dom0 as
>> there are hardware ones.
> 
> Right, I've been wondering about this, and then I need to expand the IO APIC 
> emulation code so that Xen is able to emulate two IO-APICs.
> 
> Can I ask why do you think this is needed? If the number of pins in the 
> multiple IO APIC case doesn't exceed 48 (which is what the emulated IO APIC 
> provides), shouldn't this be enough?

The number of pins easily can be larger. And I think the mapping
code would end up simpler if there was a 1:1 relationship between
physical and virtual IO-APICs. I've just not checked one of my
larger (but older) systems - it has 5 IO-APICs with a total of 120 pins.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel