Re: [Xen-devel] [v4][PATCH 14/19] tools/libxl: detect and avoid conflicts with RDM

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jun 23, 2015 at 05:57:25PM +0800, Tiejun Chen wrote:
> While building a VM, HVM domain builder provides struct hvm_info_table{}
> to help hvmloader. Currently it includes two fields to construct guest
> e820 table by hvmloader, low_mem_pgend and high_mem_pgend. So we should
> check them to fix any conflict with RAM.
> 

RAM -> RDM?

> RMRR can reside in address space beyond 4G theoretically, but we never
> see this in real world. So in order to avoid breaking highmem layout
> we don't solve highmem conflict. Note this means highmem rmrr could still
> be supported if no conflict.
> 
> But in the case of lowmem, RMRR probably scatter the whole RAM space.
> Especially multiple RMRR entries would worsen this to lead a complicated
> memory layout. And then its hard to extend hvm_info_table{} to work
> hvmloader out. So here we're trying to figure out a simple solution to
> avoid breaking existing layout. So when a conflict occurs,
> 
>     #1. Above a predefined boundary (2G)
>         - move lowmem_end below reserved region to solve conflict;
> 
>     #2. Below a predefined boundary (2G)
>         - Check strict/relaxed policy.
>         "strict" policy leads to fail libxl. Note when both policies
>         are specified on a given region, 'strict' is always preferred.
>         "relaxed" policy issue a warning message and also mask this entry 
> INVALID
>         to indicate we shouldn't expose this entry to hvmloader.
> 
> Note later we need to provide a parameter to set that predefined boundary
> dynamically.
> 
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> Reviewed-by: Kevin Tian <kevint.tian@xxxxxxxxx>
> ---
> v4:
> 
> * Consistent to use term "RDM".
> * Unconditionally set *nr_entries to 0
> * Grab to all sutffs to provide a parameter to set our predefined boundary
>   dynamically to as a separated patch later
> 
>  tools/libxl/libxl_create.c   |   2 +-
>  tools/libxl/libxl_dm.c       | 259 
> +++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_dom.c      |  17 ++-
>  tools/libxl/libxl_internal.h |  11 +-
>  tools/libxl/libxl_types.idl  |   7 ++
>  5 files changed, 293 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 6c8ec63..30e6593 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -460,7 +460,7 @@ int libxl__domain_build(libxl__gc *gc,
>  
>      switch (info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        ret = libxl__build_hvm(gc, domid, info, state);
> +        ret = libxl__build_hvm(gc, domid, d_config, state);
>          if (ret)
>              goto out;
>  
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 33f9ce6..5436bcf 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -90,6 +90,265 @@ const char *libxl__domain_device_model(libxl__gc *gc,
>      return dm;
>  }
>  
> +static struct xen_reserved_device_memory
> +*xc_device_get_rdm(libxl__gc *gc,
> +                   uint32_t flag,
> +                   uint16_t seg,
> +                   uint8_t bus,
> +                   uint8_t devfn,
> +                   unsigned int *nr_entries)

I just notice this function lives in libxl_dm.c. The function should be
renamed to libxl__xc_device_get_rdm. 

This function should return proper libxl error code (ERROR_FAIL or
something more appropriate). The allocated RDM entries should be
returned with an out parameter.

I had always thought this lived in libxc. Sorry for not having noticed
this earlier.

> +{
> +    struct xen_reserved_device_memory *xrdm;
> +    int rc;
> +
> +    /*
> +     * We really can't presume how many entries we can get in advance.
> +     */
> +    *nr_entries = 0;
> +    rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
> +                                       NULL, nr_entries);
> +    assert(rc <= 0);
> +    /* "0" means we have no any rdm entry. */
> +    if (!rc)
> +        goto out;
> +
> +    if (errno == ENOBUFS) {
> +        xrdm = malloc(*nr_entries * sizeof(xen_reserved_device_memory_t));

libxl__malloc(gc, ...);

> +        if (!xrdm) {
> +            LOG(ERROR, "Could not allocate RDM buffer!\n");
> +            goto out;
> +        }

Get rid of this.

> +        rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
> +                                           xrdm, nr_entries);
> +        if (rc) {
> +            LOG(ERROR, "Could not get reserved device memory maps.\n");
> +            *nr_entries = 0;
> +            free(xrdm);
> +            xrdm = NULL;

Get rid of free.

> +        }
> +    } else
> +        LOG(ERROR, "Could not get reserved device memory maps.\n");
> +
> + out:
> +    return xrdm;
> +}

The reset of this patch looks good to me. It does what we've discussed.

Wei.

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