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

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

On 06/17/2013 06:15 PM, Ian Jackson wrote:
> George Dunlap writes ("[PATCH] libxl,hvmloader: Don't relocate memory for MMIO hole"):
>> At the moment, qemu-xen can't handle memory being relocated by
>> hvmloader.  This may happen if a device with a large enough memory
>> region is passed through to the guest.  At the moment, if this
>> happens, then at some point in the future qemu will crash and the
>> domain will hang.  (qemu-traditional is fine.)
>
> I think the approach is good.  Arguably the two things should be in
> two patches.

You mean one in libxl to set allow_memory_relocate, one to do something 
about it?

>
>> +    const char *s;
>> +    uint8_t allow_memory_relocate=1;
>> +
>> +    s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
>> +    if (s)
>> +        allow_memory_relocate=(uint8_t)strtoll(s, NULL, 0);
>
> Use strtoul, surely, and bool_t (or _Bool).  Then there is no need for
> the cast.  (Also, spaces round `='.)

Remember that hvmloader doesn't actually have a libc; this is a locally 
implemented function, and AFAICT the only one implemented is strtoll.

>
>> +    printf("allow_memory_relocate %u\n",
>> +           __func__, allow_memory_relocate);
>
> Missing %s argument.  This shows that -Wformat isn't working.

No, my bad -- I had a script which I thought was compiling all the 
tools, but in fact was *only* compiling libxl/xl.  -Wformat is indeed 
working.

>
>> -    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,
>> +     * if we're using qemu-xen, just crash and tell people to switch
>> +     * to qemu-traditional.
>> +     */
>> +    while ((mmio_total > (pci_mem_end - pci_mem_start))
>> +           && ((pci_mem_start << 1) != 0)
>> +           && ( allow_memory_relocate
>
> I think this is an unnecessarily confusing way of writing it.
> allow_memory_relocate does not change.  I would lift it out of the
> loop into an if.  That would also mean that "git-diff -b" can show
> more clearly whether the change is truly what you intend.

But the behavior we want is this:
* If allow_memory_relocate, increase the size of the memory hole until 
it's big enough, or it reaches 2GiB
* If !allow_memory_relocate, increase the size of the memory hole until 
it's big enough, or it reaches 2GiB, or it overlaps guest memory.

If we pulled "allow_memory_relocate" out, we'd have to make two separate 
while() loops.  That might make it more clear what was going on, but 
would risk introducing errors between the two copies for the other 
conditions.

>
>> +                || (((pci_mem_start << 1)>>PAGE_SHIFT )<hvm_info->low_mem_pgend))
>>           pci_mem_start <<= 1;
>>
>> -    if ( (pci_mem_start << 1) != 0 )
>> +    if (mmio_total > (pci_mem_end - pci_mem_start))
>>           bar64_relocate = 1;
>
> Missing spaces.
>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index ac1f90e..5167ee0 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -1154,6 +1154,13 @@ void libxl__spawn_local_dm(libxl__egc *egc, libxl__dm_spawn_state *dmss)
>>           libxl__xs_write(gc, XBT_NULL,
>>                           libxl__sprintf(gc, "%s/hvmloader/bios", path),
>>                           "%s", libxl_bios_type_to_string(b_info->u.hvm.bios));
>> +        /* Disable relocating memory to make the MMIO hole larger unless we'
> re
>> +         * running qemu-traditional */
>
> This whole hunk could do with more aggressive wrapping.  See what it
> looks like in my MUA.  Can you keep it down to 70-75 columns ?

Sure.

>
>> +        libxl__xs_write(gc, XBT_NULL,
>> +                        libxl__sprintf(gc, "%s/hvmloader/allow-memory-reloca
> te", path),
>> +                        "%s",
>> +                        (b_info->device_model_version==LIBXL_DEVICE_MODEL_VE
> RSION_QEMU_XEN_TRADITIONAL)?
>> +                        "1":"0");
>>           free(path);
>
> Use GCSPRINTF not libxl_sprintf.  Lacks error check (check return
> value from libxl__xs_write).

Once again, I disagree with mixing different code styles in the same 
function.  Using GCSPRINTF and doing an error check here will make it 
seem like it's doing something different than the line immediately above 
it (which is what I copied to get here), which will confuse people.

In theory the "fix it up as we go along" sounds good, but in practice 
it's awful.  How many times have you had to say exactly this kind of 
thing, when all I did was copy some bit of code that was nearby and do 
exactly the same thing?  It just annoys you and frustrates me.

I shouldn't have to keep in my head, "This is the new way of doing 
things".  It's a total waste of mental energy; I have better things to 
spend my brain capacity on.  If you want things to change, I think we're 
going to have to have a "flag day" where we go through and change all 
the code, at least file-by-file.  I'd be willing to help with this 
cleanup once the 4.4 development window opens.

> Instead of "%s", blah?"1":"0" use "%d" and the == expression itself.

That's a good idea.

  -George