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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/7 19:20, Wei Liu wrote:
> 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.
Right.

> > > 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.
Sure.

> > > > +
> > > > +    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.
Thanks
Tiejun

> [...]
> > > > +        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