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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +    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 `='.)

> +    printf("allow_memory_relocate %u\n",
> +           __func__, allow_memory_relocate);

Missing %s argument.  This shows that -Wformat isn't 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.

> +                || (((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 ?

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

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

Ian.

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