Re: [PATCH v2 5/5] libxl, hvmloader: Don't relocate memory for MMIO hole

[George Dunlap <george.dunlap@eu.citrix.com>]

On 19/06/13 18:18, Stefano Stabellini wrote:
> On Tue, 18 Jun 2013, George Dunlap wrote:
>> +    const char *s;
>> +    bool allow_memory_relocate = 1;
> Arguably the default should be 0, given that the default device model is
> qemu-xen that cannot cope with memory relocation.

OK, so time to think a bit harder about this. This will only matter if 
someone is using this hvmloader with a non-libxl toolstack which 
includes xend, or a home-grown one.

* If we default to 1, then:
  - VMs running qemu-traditional will be have exactly as before
  - VMs running qemu-xen will have the risk of crashing mysteriously.
  - If qemu-xen is the default, then there is a work-around: run 
qemu-traditional

* If we default to 0, then:
  - VMs running qemu-xen will be fine
  - VMs running qemu-traditional may have strange problems; we haven't 
tested relocating things into 64-bit with qemu-tradiational.
  - There is no work-around available; if the device either can't be 
relocated, or the OS / qemu can't handle the relocation, then the user 
is just hosed.

Furthermore, I think xm defaults to qemu-traditional, right?  Does xm 
even know how to drive qemu-xen?  If it does default to 
qemu-traditional, defaulting to 0 will pretty much guarantee a whole 
slew of currently-working configurations will be affected (perhaps 
break, perhaps not).

Overall I think defaulting to 1 is probably the lowest-risk option.

>
>
>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>> +    if ( s )
>> +        allow_memory_relocate = (bool)strtoll(s, NULL, 0);
>> +    printf("Relocating guest memory for lowmem MMIO space %s\n",
>> +           allow_memory_relocate?"enabled":"disabled");
> It doesn't take a strtoll to parse a boolean.

As discussed in v1, strtoll is the only "XtoY" function available in 
hvmloader. :-)  The only other option would be to explicitly compare for 
"1" or "0" (or do some half-baked *s-'0' thing).

This does make me think though -- what is the semantics of casting to a 
bool?  Is it !!, or will it essentially clip off the high bits?  (e.g., 
would "2" become "1", or "0"?)

>>       /* Program PCI-ISA bridge with appropriate link routes. */
>>       isa_irq = 0;
>>       for ( link = 0; link < 4; link++ )
>> @@ -209,14 +220,38 @@ void pci_setup(void)
>>           pci_writew(devfn, PCI_COMMAND, cmd);
>>       }
>>   
>> -    while ( (mmio_total > (pci_mem_end - pci_mem_start)) &&
>> -            ((pci_mem_start << 1) != 0) )
>> +    /*
>> +     * At the moment qemu-xen can't deal with relocated memory regions.
>> +     * It's too close to the release to make a proper fix; for now,
>> +     * only allow the MMIO hole to grow large enough to move guest memory
>> +     * if we're running qemu-traditional.  Items that don't fit will be
>> +     * relocated into the 64-bit address space.
> I would avoid mentioning release issues in a comment within the code.

I guess it depends on whether we intend to keep this change or to get 
rid of it once the 4.4. window opens. If we intend to get rid of it, 
then I think the comment should stay; if for some reason someone comes 
along later and and wants to know, "Can I change this?" Knowing that it 
was only meant to be a temporary measure is important information.

Really, I'm of the opinion that if KVM is using SeaBIOS's pci routines, 
we should just move do the same.  No sense in duplicating the effort for 
something like this.

>> +     * This loop now does the following:
>> +     * - If allow_memory_relocate, increase the MMIO hole until it's
>> +     *   big enough, or until it's 2GiB
>> +     * - If !allow_memory_relocate, increase the MMIO hole until it's
>> +     *   big enough, or until it's 2GiB, or until it overlaps guest
>> +     *   memory
>> +     */
>> +    while ( (mmio_total > (pci_mem_end - pci_mem_start))
>> +            && ((pci_mem_start << 1) != 0)
>> +            && (allow_memory_relocate
>> +                || (((pci_mem_start << 1) >> PAGE_SHIFT)
>> +                    < hvm_info->low_mem_pgend)) )
> Isn't this last condition inverted? It should be '>=' ?

Yep -- replied to myself Tuesday saying as much. :-)

Good catch though -- thanks for the close review.

  -George