Re: [Xen-devel] [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 03, 2015 at 10:25:47AM +0800, Chen, Tiejun wrote:
[...]
> >>+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)
> >>+{
> >>+    struct xen_reserved_device_memory *xrdm = NULL;
> >>+    int rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
> >>+                                           xrdm, nr_entries);
> >>+
> >
> >Please separate declaration and function call. Also change xrdm to NULL
> 
> Are you saying this?
> 
>     struct xen_reserved_device_memory *xrdm = NULL;
>     int rc;
> 
>     rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>                                        xrdm, nr_entries);

Yes, splitting "rc = " to a separate line. The other thing is: 

     rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
                                        NULL, nr_entries);
                                        ^ here

It's mostly cosmetic. IMHO it is clearer than passing xrdm which is
always NULL in that function call.

> 
> >in that function call.
> 
> Sorry, what do you mean by this point? Or you let me to change xrdm to NULL
> inside xc_reserved_device_memory_map()?
> 
> >
> >>+    assert( rc <= 0 );
> >>+    /* "0" means we have no any rdm entry. */
> >>+    if ( !rc )
> >>+        goto out;
> >
> >Also set *nr_entries = 0; otherwise you can't distinguish error vs 0
> >entries.
> 
> *nr_entries is always updated by xc_reserved_device_memory_map() above.
> 

Actually no. If xc_hypercall_bounce_pre fails in the function,
nr_entries is untouched.

> >
> >>+
> >>+    if ( errno == ENOBUFS )
> >>+    {
> >>+        if ( (xrdm = malloc(*nr_entries *
> >>+                            sizeof(xen_reserved_device_memory_t))) == NULL 
> >>)
[...]
> >>+    return -1;
> >
> >Please return libxl error code.
> 
> ERROR_FAIL?
> 

Yes, that's fine.

[...]
> >>+        args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH;
> >>+    mmio_start = (1ull << 32) - args.mmio_size;
> >>+
> >>+    if (args.lowmem_size > mmio_start)
> >>+        args.lowmem_size = mmio_start;
> >>+
> >>+    /*
> >>+     * We'd like to set a memory boundary to determine if we need to check
> >>+     * any overlap with reserved device memory.
> >>+     */
> >>+    rdm_mem_boundary = 0x80000000;
> >>+    if (info->rdm_mem_boundary_memkb)
> >
> 
> I'm going to update this chunk of codes as follows:
> 
> #1. @@ -858,6 +858,12 @@ const char *libxl_defbool_to_string(libxl_defbool
> b);
>  #define LIBXL_TIMER_MODE_DEFAULT -1
>  #define LIBXL_MEMKB_DEFAULT ~0ULL
> 
> +/*
> + * We'd like to set a memory boundary to determine if we need to check
> + * any overlap with reserved device memory.
> + */
> +#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
> +
>  #define LIBXL_MS_VM_GENID_LEN 16
>  typedef struct {
>      uint8_t bytes[LIBXL_MS_VM_GENID_LEN];
> 
> >I think you mean info->rdm_mem_boundary_memkb != LIBXL_MEMKB_DEFAULT?
> >
> 
> #2.
> 
> @@ -109,6 +109,10 @@ void libxl__rdm_setdefault(libxl__gc *gc,
> libxl_domain_build_info *b_info)
>  {
>      if (b_info->rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID)
>          b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
> +
> +    if (b_info->rdm_mem_boundary_memkb == LIBXL_MEMKB_DEFAULT)
> +        b_info->rdm_mem_boundary_memkb =
> +                            LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;

This looks plausible.

Wei.

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