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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, Oct 12, 2016 at 09:55:44AM -0600, Jan Beulich wrote:
> >>> 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.

I've initially used a back-listing approach. We can always change this later 
on.

So I've ended up crafting a new MADT, XSDT and RSDP. Note that I'm not 
crafting a new custom RSDT (and in fact I'm setting rsdt_physical_address = 
0 in the RSDP together with revision = 2). This is all placed in RAM stolen 
from the guest memory map and marked as E820_ACPI, which means that the new 
RSDP no longer resides below 1MB, and that the Dom0 kernel _must_ use the 
rsdp_paddr provided in the start info, or else it's going to access the 
native RSDP.

> >> > +    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.

Hm, I would say that we leave this as it is currently, and then we can 
always play more tricks later on if we found any of such systems.
 
> > 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.

As said before, I would leave the current implementation and look into that 
option if needed.

> >> > +    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.

Yes, I agree that having a 1:1 relation between physical and virtual IO 
APICs is the best option. I've added a warning printk if the native hardware 
has more than one IO APIC, and I plan to expand the current IO APIC 
emulation code when I'm done with this initial PVHv2 Dom0 implementation, so 
that it can support emulating multiple IO APICs with varying number of pins.

Roger.

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