From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?windows-1252?Q?Roger_Pau_Monn=E9?= <roger.pau@citrix.com>
Subject: Re: [PATCH v4 31/31] libxl: allow the creation of HVM
 domains without a device model.
Date: Fri, 7 Aug 2015 17:51:02 +0200
Message-ID: <55C4D3E6.1010703@citrix.com>
References: <1438942688-7610-1-git-send-email-roger.pau@citrix.com>
	<1438942688-7610-32-git-send-email-roger.pau@citrix.com>
	<20150807125805.GR6005@zion.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=654947d1e=roger.pau@citrix.com>)
	id 1ZNjvJ-00081j-Sp
	for xen-devel@lists.xenproject.org; Fri, 07 Aug 2015 15:51:10 +0000
In-Reply-To: <20150807125805.GR6005@zion.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wei Liu <wei.liu2@citrix.com>
Cc: xen-devel@lists.xenproject.org, Ian Jackson <ian.jackson@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
List-Id: xen-devel@lists.xenproject.org

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 pa=
rsed
>> 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=E9 <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.
>>  =

>> +=3Ditem 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.

>> +
>>  =3Dback
>>  =

>>  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 |=3D XENMEMF_populate_on_demand;
>>  =

>> +    /* Make sure there's a MMIO hole for the special pages. */
>> +    if ( dom->mmio_size =3D=3D 0 )
>> +    {
>> +        dom->mmio_size =3D NR_SPECIAL_PAGES << PAGE_SHIFT;
>> +        dom->mmio_start =3D special_pfn(0);
>> +    }
>> +
> =

> Better to just assert(dom->mmio_size !=3D 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 =3D=3D 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 =3D=3D LIBXL_DOMAIN_TYPE_HVM) {
>> -        rc =3D 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 =3D 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 =3D 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 =3D CTX;
>>      uint32_t domid =3D 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 =3D 1;
>> -        else
>> +        if (libxl_get_stubdom_id(CTX, domid)) {
>>              dm_present =3D 0;
>> -        break;
>> +            break;
>> +        }
>> +        /* fall through */
>>      case LIBXL_DOMAIN_TYPE_PV:
>> -        pid =3D libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local=
/domain/%d/image/device-model-pid", domid));
>> -        dm_present =3D (pid !=3D NULL);
>> +        dm_present =3D libxl__domain_has_device_model(gc, domid);
>>          break;
>>      case LIBXL_DOMAIN_TYPE_INVALID:
>>          rc =3D 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_inf=
o *info)
>>  {
>>      int rc;
>>  =

>> @@ -3240,8 +3240,15 @@ int libxl__device_nic_setdefault(libxl__gc *gc, l=
ibxl_device_nic *nic,
>>  =

>>      switch (libxl__domain_type(gc, domid)) {
>>      case LIBXL_DOMAIN_TYPE_HVM:
>> -        if (!nic->nictype)
>> -            nic->nictype =3D LIBXL_NIC_TYPE_VIF_IOEMU;
>> +        if (!nic->nictype) {
>> +            if (info !=3D NULL &&
>> +                info->device_model_version !=3D LIBXL_DEVICE_MODEL_VERS=
ION_NONE)
>> +                nic->nictype =3D LIBXL_NIC_TYPE_VIF_IOEMU;
>> +            else if (libxl__domain_has_device_model(gc, domid))
>> +                nic->nictype =3D LIBXL_NIC_TYPE_VIF_IOEMU;
>> +            else
>> +                nic->nictype =3D LIBXL_NIC_TYPE_VIF;
> =

> I think all you need is to pass in device model version and
> =

>    if version !=3D none
>        nictype =3D ioemu
>    else
>        nictype =3D 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.