[Xen-devel] [XEN PATCH for-4.13 0/3] Fix stub dm since pci pt overhaul

[Xen-devel] [XEN PATCH for-4.13 0/3] Fix stub dm since pci pt overhaul

[Ian Jackson]

This series fixes guest creation with stub device models, which was
broken by ad011ad08843 "libxl/xl: Overhaul passthrough setting logic".

I have tested this with all three patches and it fixes the regression.
I'm not sure about the 3rd and would like an opinion from x86 folks,
for the reasons explained in its commit message.

Ian Jackson (3):
  libxl: domain_config_setdefault: Document use of domid
  libxl: libxl__spawn_stub_dm: Call domain_config_setdefault
  libxl: Set shadow_memkb for stub device model domains

 tools/libxl/libxl_create.c | 2 +-
 tools/libxl/libxl_dm.c     | 9 +--------
 2 files changed, 2 insertions(+), 9 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH for-4.13 1/3] libxl: domain_config_setdefault: Document use of domid

[Ian Jackson]

We are going to want to call this from a site which has a domid which
is good for logging but not the domid of the domain we are creating
(namely, the stub device domain).

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_create.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index d59ac2ab05..32d45dcef0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -891,7 +891,7 @@ static unsigned long libxl__get_required_iommu_memory(unsigned long maxmem_kb)
 
 int libxl__domain_config_setdefault(libxl__gc *gc,
                                     libxl_domain_config *d_config,
-                                    uint32_t domid)
+                                    uint32_t domid /* for logging, only */)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     int ret;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH for-4.13 2/3] libxl: libxl__spawn_stub_dm: Call domain_config_setdefault

[Ian Jackson]

Previously, defaulting and checking of some aspects of the domain
config was skipped for stub dms.  This has been the case forever.

In ad011ad08843 "libxl/xl: Overhaul passthrough setting logic" some
defaulting that was needed for stub dms was moved from
libxl__domain_create_info_setdefault to .._config_setdefault with the
result that for stub dms, libxl__domain_make fails with this
assertion:
  xl: libxl_create.c:582: libxl__domain_make: Assertion
  `info->passthrough != LIBXL_PASSTHROUGH_DEFAULT' failed.

Fix this by properly doing all defaulting and all checking for stub
dms.  This is more correct, but (especially at this stage of the
release) it is necessary to more closely evaluate the effects by
reviewing the body of _config_setdefault.  The changes are as follows:

One actual functional change:

* The new passthrough defaulting is properly done.  This is what we
  are trying to actually fix here.

And a lot of things that make no difference:

* shadow_memkb would now be set.  Whether this would be correct is not
  entirely clear.  It seems better to make this patch (whose purpose
  is to fix the passthrough defaulting) *not* include that semantic
  change, so here I have included a hunk to explicitly override this.

* FLASK ssid_label is processed.  But the actual ssidref is copied
  from the guest domain by spawn_stub_dm, and ssid_label is set to
  NULL.  So no change.

* We set iommu_memkb.  But to 0 since passthrough is disabled.

* cpuid pool_name is processed.  But this is not set by
  spawn_stub_dm.  (Arguably this is a bug: stub dms should inherit the
  parent cpupool.)  The effect is to leave poolid set to 0 and call
  libxl_cpupoolid_is_valid but that always succeeds for 0.  So no
  change.

* Various extra checks are done: reject PCI passthrough for HVM with
  POD (stub dm is PV); reject pod + vnuma, or PV + vnuma (stub dm has
  no vnuma); reject nested HVM or pod, with alt2pm-hvm (again, stub dm
  is PV).  So these checks will always pass.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_dm.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e6a48974f8..7e52f09731 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2141,6 +2141,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl_domain_build_info_init(&dm_config->b_info);
     libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
 
+    dm_config->b_info.shadow_memkb = 0;
     dm_config->b_info.max_vcpus = 1;
     dm_config->b_info.max_memkb = 28 * 1024 +
         guest_config->b_info.video_memkb;
@@ -2167,14 +2168,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     dm_config->c_info.run_hotplug_scripts =
         guest_config->c_info.run_hotplug_scripts;
 
-    libxl_physinfo physinfo;
-    ret = libxl_get_physinfo(CTX, &physinfo);
-    if (ret) goto out;
-
-    ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info,
-                                               &physinfo);
-    if (ret) goto out;
-    ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);
+    ret = libxl__domain_config_setdefault(gc, dm_config, guest_domid);
     if (ret) goto out;
 
     if (libxl_defbool_val(guest_config->b_info.u.hvm.vnc.enable)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH for-4.13 3/3] libxl: Set shadow_memkb for stub device model domains

[Ian Jackson]

Previously we did not do this.  Indeed we have never done so.  Stub
domains have had no memory allowance for shadow memory.  This seems to
be an existing bug which we fix.

x86 maintainers: please comment.

I am not sure of the interaction between this change and dom0
autoballooning.  The memory requirement disclosed to libxl's
caller (eg, xl) by libxl_domain_need_memory do not include this
additional memory.  If they should do, then
libxl_get_required_shadow_memory and/or libxl_domain_need_memory
may need adjusting to pay attention to whether a stub dm is going to
be required.  Currently libxl__domain_need_memory simply adds 32Kby
for guests with a stub dm.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_dm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 7e52f09731..ff746a890a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2141,7 +2141,6 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl_domain_build_info_init(&dm_config->b_info);
     libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
 
-    dm_config->b_info.shadow_memkb = 0;
     dm_config->b_info.max_vcpus = 1;
     dm_config->b_info.max_memkb = 28 * 1024 +
         guest_config->b_info.video_memkb;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 1/3] libxl: domain_config_setdefault: Document use of domid

[Jürgen Groß]

On 28.10.19 16:29, Ian Jackson wrote:
> We are going to want to call this from a site which has a domid which
> is good for logging but not the domid of the domain we are creating
> (namely, the stub device domain).
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Juergen Gross <jgross@suse.com>

For the series: Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 0/3] Fix stub dm since pci pt overhaul

[Jürgen Groß]

On 28.10.19 16:29, Ian Jackson wrote:
> This series fixes guest creation with stub device models, which was
> broken by ad011ad08843 "libxl/xl: Overhaul passthrough setting logic".
> 
> I have tested this with all three patches and it fixes the regression.
> I'm not sure about the 3rd and would like an opinion from x86 folks,
> for the reasons explained in its commit message.
> 
> Ian Jackson (3):
>    libxl: domain_config_setdefault: Document use of domid
>    libxl: libxl__spawn_stub_dm: Call domain_config_setdefault
>    libxl: Set shadow_memkb for stub device model domains
> 
>   tools/libxl/libxl_create.c | 2 +-
>   tools/libxl/libxl_dm.c     | 9 +--------
>   2 files changed, 2 insertions(+), 9 deletions(-)
> 

I'd appreciate a rather fast review of these patches, as they repair an
issue blocking an OSStest push.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 3/3] libxl: Set shadow_memkb for stub device model domains

[Andrew Cooper]

On 28/10/2019 15:29, Ian Jackson wrote:
> Previously we did not do this.  Indeed we have never done so.  Stub
> domains have had no memory allowance for shadow memory.  This seems to
> be an existing bug which we fix.
>
> x86 maintainers: please comment.

PV guests need a shadow allocation to migrate, or if they trip over the
PV L1TF safety checks.

The former is not applicable to stubdoms, and the latter is arguably
better left with a shadow allocation of 0.

These are infrastructure VMs rather than customer VMs, and there is no
excuse really to be running an L1TF-vulnerable stubdom which is taking
the 20% perf hit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 1/3] libxl: domain_config_setdefault: Document use of domid

[Anthony PERARD]

On Mon, Oct 28, 2019 at 03:29:46PM +0000, Ian Jackson wrote:
> We are going to want to call this from a site which has a domid which
> is good for logging but not the domid of the domain we are creating
> (namely, the stub device domain).
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> ---
>  tools/libxl/libxl_create.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index d59ac2ab05..32d45dcef0 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -891,7 +891,7 @@ static unsigned long libxl__get_required_iommu_memory(unsigned long maxmem_kb)
>  
>  int libxl__domain_config_setdefault(libxl__gc *gc,
>                                      libxl_domain_config *d_config,
> -                                    uint32_t domid)
> +                                    uint32_t domid /* for logging, only */)

Should libxl__arch_passthrough_mode_setdefault() have the same comment?
Just in case, since it's called with that same domid.

Beside that:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 2/3] libxl: libxl__spawn_stub_dm: Call domain_config_setdefault

[Anthony PERARD]

On Mon, Oct 28, 2019 at 03:29:47PM +0000, Ian Jackson wrote:
> Previously, defaulting and checking of some aspects of the domain
> config was skipped for stub dms.  This has been the case forever.
> 
> In ad011ad08843 "libxl/xl: Overhaul passthrough setting logic" some
> defaulting that was needed for stub dms was moved from
> libxl__domain_create_info_setdefault to .._config_setdefault with the
> result that for stub dms, libxl__domain_make fails with this
> assertion:
>   xl: libxl_create.c:582: libxl__domain_make: Assertion
>   `info->passthrough != LIBXL_PASSTHROUGH_DEFAULT' failed.
> 
> Fix this by properly doing all defaulting and all checking for stub
> dms.  This is more correct, but (especially at this stage of the
> release) it is necessary to more closely evaluate the effects by
> reviewing the body of _config_setdefault.  The changes are as follows:
> 
> One actual functional change:
> 
> * The new passthrough defaulting is properly done.  This is what we
>   are trying to actually fix here.
> 
> And a lot of things that make no difference:
> 
> * shadow_memkb would now be set.  Whether this would be correct is not
>   entirely clear.  It seems better to make this patch (whose purpose
>   is to fix the passthrough defaulting) *not* include that semantic
>   change, so here I have included a hunk to explicitly override this.
> 
> * FLASK ssid_label is processed.  But the actual ssidref is copied
>   from the guest domain by spawn_stub_dm, and ssid_label is set to
>   NULL.  So no change.
> 
> * We set iommu_memkb.  But to 0 since passthrough is disabled.
> 
> * cpuid pool_name is processed.  But this is not set by
>   spawn_stub_dm.  (Arguably this is a bug: stub dms should inherit the
>   parent cpupool.)  The effect is to leave poolid set to 0 and call
>   libxl_cpupoolid_is_valid but that always succeeds for 0.  So no
>   change.
> 
> * Various extra checks are done: reject PCI passthrough for HVM with
>   POD (stub dm is PV); reject pod + vnuma, or PV + vnuma (stub dm has
>   no vnuma); reject nested HVM or pod, with alt2pm-hvm (again, stub dm
>   is PV).  So these checks will always pass.
> 
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Juergen Gross <jgross@suse.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 1/3] libxl: domain_config_setdefault: Document use of domid

[Ian Jackson]

Anthony PERARD writes ("Re: [XEN PATCH for-4.13 1/3] libxl: domain_config_setdefault: Document use of domid"):
> Should libxl__arch_passthrough_mode_setdefault() have the same comment?
> Just in case, since it's called with that same domid.

Yes, you are right, it should.  I just added that along with your ack
while pushing.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [XEN PATCH for-4.13 3/3] libxl: Set shadow_memkb for stub device model domains

[Ian Jackson]

Andrew Cooper writes ("Re: [XEN PATCH for-4.13 3/3] libxl: Set shadow_memkb for stub device model domains"):
> On 28/10/2019 15:29, Ian Jackson wrote:
> > Previously we did not do this.  Indeed we have never done so.  Stub
> > domains have had no memory allowance for shadow memory.  This seems to
> > be an existing bug which we fix.
> >
> > x86 maintainers: please comment.
> 
> PV guests need a shadow allocation to migrate, or if they trip over the
> PV L1TF safety checks.
> 
> The former is not applicable to stubdoms, and the latter is arguably
> better left with a shadow allocation of 0.
> 
> These are infrastructure VMs rather than customer VMs, and there is no
> excuse really to be running an L1TF-vulnerable stubdom which is taking
> the 20% perf hit.

Thanks for the review.

I think this means I should drop this patch.  The other two were
reviewed and release-acked (thanks Jürgen and Anthony) so I have just
pushed them to staging.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel