Re: [PATCH v4 31/31] libxl: allow the creation of HVM domains without a device model.

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Aug 07, 2015 at 05:51:02PM +0200, Roger Pau Monné wrote:
[...]
> >>  It is recommended to accept the default value for new guests.  If
> >> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> >> index 1599de4..d67feb0 100644
> >> --- a/tools/libxc/xc_dom_x86.c
> >> +++ b/tools/libxc/xc_dom_x86.c
> >> @@ -1269,6 +1269,13 @@ static int meminit_hvm(struct xc_dom_image *dom)
> >>      if ( nr_pages > target_pages )
> >>          memflags |= XENMEMF_populate_on_demand;
> >>  
> >> +    /* Make sure there's a MMIO hole for the special pages. */
> >> +    if ( dom->mmio_size == 0 )
> >> +    {
> >> +        dom->mmio_size = NR_SPECIAL_PAGES << PAGE_SHIFT;
> >> +        dom->mmio_start = special_pfn(0);
> >> +    }
> >> +
> > 
> > Better to just assert(dom->mmio_size != 0);
> > 
> > It's really libxl's responsibility to generate memory layout for guest.
> > Libxc doesn't have all information to make the decision.
> 
> As said in a previous email, libxl doesn't know the size or position of
> the special pages created by libxc code, so right now it's impossible
> for libxl to create a correct mmio hole for a HVMlite guest.
> 

Then your change here doesn't solve the real problem. You can't guarantee
when dom->mmio_size != 0, 1) the hole is large enough to accommodate all
special pages, 2) special pages don't clash with real mmio pages.

I still think there should be only one entity that controls what guest
memory layout looks like. And that entity should be the one which has
all the information available. In this case, libxl should be the one who
decides.

> > 
> >>      if ( dom->nr_vmemranges == 0 )
> >>      {
> >>          /* Build dummy vnode information
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> index 083f099..a01868a 100644
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -1033,11 +1033,13 @@ int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid)
> >>      }
> >>  
> >>      if (type == LIBXL_DOMAIN_TYPE_HVM) {
> >> -        rc = libxl__domain_resume_device_model(gc, domid);
> >> -        if (rc < 0) {
> >> -            LOG(ERROR, "failed to unpause device model for domain %u:%d",
> >> -                domid, rc);
> >> -            goto out;
> >> +        if (libxl__domain_has_device_model(gc, domid)) {
> > 
> > Checking for device model version is not enough?
> 
> Sadly we don't have access to the device model version here, and it is
> not stored anywhere, so the only option I see is to actually check if
> there's a device model running or not in order to figure out if we have
> to unpause it.
> 

There is a function called libxl__device_model_version_running. You can
use that.

> >> +            rc = libxl__domain_resume_device_model(gc, domid);
> >> +            if (rc < 0) {
> >> +                LOG(ERROR, "failed to unpause device model for domain %u:%d",
> >> +                    domid, rc);
> >> +                goto out;
> >> +            }
> >>          }
> >>      }
> >>      ret = xc_domain_unpause(ctx->xch, domid);
> >> @@ -1567,7 +1569,6 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
> >>      libxl_ctx *ctx = CTX;
> >>      uint32_t domid = dis->domid;
> >>      char *dom_path;
> >> -    char *pid;
> >>      int rc, dm_present;
> >>  
> >>      libxl__ev_child_init(&dis->destroyer);
> >> @@ -1584,14 +1585,13 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
> >>  
> >>      switch (libxl__domain_type(gc, domid)) {
> >>      case LIBXL_DOMAIN_TYPE_HVM:
> >> -        if (!libxl_get_stubdom_id(CTX, domid))
> >> -            dm_present = 1;
> >> -        else
> >> +        if (libxl_get_stubdom_id(CTX, domid)) {
> >>              dm_present = 0;
> >> -        break;
> >> +            break;
> >> +        }
> >> +        /* fall through */
> >>      case LIBXL_DOMAIN_TYPE_PV:
> >> -        pid = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/image/device-model-pid", domid));
> >> -        dm_present = (pid != NULL);
> >> +        dm_present = libxl__domain_has_device_model(gc, domid);
> >>          break;
> >>      case LIBXL_DOMAIN_TYPE_INVALID:
> >>          rc = ERROR_FAIL;
> >> @@ -3203,7 +3203,7 @@ out:
> >>  /******************************************************************************/
> >>  
> >>  int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
> >> -                                 uint32_t domid)
> >> +                                 uint32_t domid, libxl_domain_build_info *info)
> >>  {
> >>      int rc;
> >>  
> >> @@ -3240,8 +3240,15 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
> >>  
> >>      switch (libxl__domain_type(gc, domid)) {
> >>      case LIBXL_DOMAIN_TYPE_HVM:
> >> -        if (!nic->nictype)
> >> -            nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> >> +        if (!nic->nictype) {
> >> +            if (info != NULL &&
> >> +                info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE)
> >> +                nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> >> +            else if (libxl__domain_has_device_model(gc, domid))
> >> +                nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU;
> >> +            else
> >> +                nic->nictype = LIBXL_NIC_TYPE_VIF;
> > 
> > I think all you need is to pass in device model version and
> > 
> >    if version != none
> >        nictype = ioemu
> >    else
> >        nictype = vif
> > 
> > ?
> > 
> > Otherwise the code suggests that there can be case you have specified a
> > device model when creating a domain but it somehow disappears when
> > domain is running?
> 
> That's exactly the case. During domain creation we know if there's a
> device model or not. But if we are doing a hot-attach of a nic this
> information is not available, so the only solution I see is to actually
> check for the device model presence.
> 

See above, use the other function instead.

Wei.

> Thanks for the review, Roger.