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

["Roger Pau Monné" <roger.pau@citrix.com>]

El 07/08/15 a les 14.58, Wei Liu ha escrit:
> On Fri, Aug 07, 2015 at 12:18:08PM +0200, Roger Pau Monne wrote:
>> Replace the firmware loaded into HVM guests with an OS kernel. Since the HVM
>> builder now uses the PV xc_dom_* set of functions this kernel will be parsed
>> and loaded inside the guest like on PV, but the container is a pure HVM
>> guest.
>>
>> Also, if device_model_version is set to none or a device model for the
>> specified domain is not present unconditinally set the nic type to
>> LIBXL_NIC_TYPE_VIF.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>> Changes since v3:
>>  - Add explicit /* fall through */ comments.
>>  - Expand libxl__device_nic_setdefault so that it sets the right nic type
>>    for HVMlite guests.
>>  - Remove stray space in hvm_build_set_params.
>>  - Fix the error paths of libxl__domain_firmware.
>> ---
>>  docs/man/xl.cfg.pod.5        |  5 ++++
>>  tools/libxc/xc_dom_x86.c     |  7 +++++
>>  tools/libxl/libxl.c          | 39 ++++++++++++++-----------
>>  tools/libxl/libxl_create.c   | 16 ++++++++++-
>>  tools/libxl/libxl_dm.c       | 13 ++++++++-
>>  tools/libxl/libxl_dom.c      | 68 ++++++++++++++++++++++++++++++--------------
>>  tools/libxl/libxl_internal.h |  5 +++-
>>  tools/libxl/libxl_types.idl  |  1 +
>>  tools/libxl/libxl_x86.c      |  4 ++-
>>  tools/libxl/xl_cmdimpl.c     |  2 ++
>>  10 files changed, 118 insertions(+), 42 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 80e51bb..8cd7726 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1741,6 +1741,11 @@ This device-model is the default for Linux dom0.
>>  Use the device-model based upon the historical Xen fork of Qemu.
>>  This device-model is still the default for NetBSD dom0.
>>  
>> +=item B<none>
>> +
>> +Don't use any device model. This requires a kernel capable of booting
>> +in this mode.
> 
> booting without emulated devices?

Yes, that's more clear.

>> +
>>  =back
>>  
>>  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.

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

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

Thanks for the review, Roger.