Message ID | 20191001145714.556-1-paul.durrant@citrix.com (mailing list archive) |
---|---|
Headers | show |
Series | libxl: fix assertion failure | expand |
On 01.10.19 16:57, Paul Durrant wrote: > This was originally a single patch, which is now patch #2 of this series. > > Paul Durrant (2): > libxl: replace 'enabled' with 'unknown' in libxl_passthrough > enumeration > libxl: choose an appropriate default for passthrough... > > tools/libxl/libxl_create.c | 10 +++++++--- > tools/libxl/libxl_types.idl | 2 +- > tools/xl/xl_parse.c | 26 +++++++++++++++----------- > 3 files changed, 23 insertions(+), 15 deletions(-) For the series: Release-acked-by: Juergen Gross <jgross@suse.com> Juergen
Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): > This was originally a single patch, which is now patch #2 of this series. > > Paul Durrant (2): > libxl: replace 'enabled' with 'unknown' in libxl_passthrough > enumeration > libxl: choose an appropriate default for passthrough... Thanks. I have applied these, and also a style fixup (below). I am continuing to look at the defaulting and config management here with a view to getting rid of some of the duplicated code and moving it all into libxl. Ian. From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001 From: Ian Jackson <ian.jackson@eu.citrix.com> Date: Wed, 2 Oct 2019 16:55:47 +0100 Subject: [PATCH] libxl: create: style: Add a pair of missing { ] From CODING_STYLE: Every indented statement is braced, but blocks that contain just one statement may have the braces omitted. To avoid confusion, either all the blocks in an if...else chain have braces, or none of them do. CC: Paul Durrant <paul.durrant@citrix.com> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> --- tools/libxl/libxl_create.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 62e13f3e7c..099761a2d7 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -68,8 +68,9 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc, c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) || !info.cap_iommu_hap_pt_share) ? LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; - } else if (!info.cap_hvm_directio) + } else if (!info.cap_hvm_directio) { c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED; + } /* An explicit setting should now have been chosen */ assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote: > > Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): > > This was originally a single patch, which is now patch #2 of this series. > > > > Paul Durrant (2): > > libxl: replace 'enabled' with 'unknown' in libxl_passthrough > > enumeration > > libxl: choose an appropriate default for passthrough... > > Thanks. I have applied these, and also a style fixup (below). > Cool. > I am continuing to look at the defaulting and config management here > with a view to getting rid of some of the duplicated code and moving > it all into libxl. > That would indeed be beneficial for the likes of libvirt. Perhaps it would be reasonable to unify the create and build info at a libxl level (even though they may feed into distinct domctls underneath for the moment)? Cheers, Paul > Ian. > > From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001 > From: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Wed, 2 Oct 2019 16:55:47 +0100 > Subject: [PATCH] libxl: create: style: Add a pair of missing { ] > > From CODING_STYLE: > > Every indented statement is braced, but blocks that contain just one > statement may have the braces omitted. To avoid confusion, either all > the blocks in an if...else chain have braces, or none of them do. > > CC: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com> > --- > tools/libxl/libxl_create.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 62e13f3e7c..099761a2d7 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -68,8 +68,9 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc, > c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) || > !info.cap_iommu_hap_pt_share) ? > LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT; > - } else if (!info.cap_hvm_directio) > + } else if (!info.cap_hvm_directio) { > c_info->passthrough = LIBXL_PASSTHROUGH_DISABLED; > + } > > /* An explicit setting should now have been chosen */ > assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN); > -- > 2.11.0 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 02/10/2019 17:02, Ian Jackson wrote: > Paul Durrant writes ("[PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): >> This was originally a single patch, which is now patch #2 of this series. >> >> Paul Durrant (2): >> libxl: replace 'enabled' with 'unknown' in libxl_passthrough >> enumeration >> libxl: choose an appropriate default for passthrough... > Thanks. I have applied these, and also a style fixup (below). > > I am continuing to look at the defaulting and config management here > with a view to getting rid of some of the duplicated code and moving > it all into libxl. > > Ian. > > From b01b1dc046da70a2621a4d1f032ddb22b0cdde6b Mon Sep 17 00:00:00 2001 > From: Ian Jackson <ian.jackson@eu.citrix.com> > Date: Wed, 2 Oct 2019 16:55:47 +0100 > Subject: [PATCH] libxl: create: style: Add a pair of missing { ] s/]/}/ ~Andrew
Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): > On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote: > > I am continuing to look at the defaulting and config management here > > with a view to getting rid of some of the duplicated code and moving > > it all into libxl. > > That would indeed be beneficial for the likes of libvirt. I propose the following plan for 4.13: * Move the default calculations of b_info->shadow_memkb and b_info->iommu_memkb from xl_vmcontrol.c into libxl, in a new function libxl__need_memory_setdefault, called from initiate_domain_create. That has access to the whole of c_info and b_info. * Change the API/ABI for libxl_domain_need_memory to take a libxl_domain_config. Internally, this will call an implementation function libxl__domain_need_memory which takes the b_info and c_info separately, and which calls libxl__need_memory_setdefault. (This is the only other call site for libxl__domain_build_info_setdefault.) * There will be the usual backward compatible arrangement: here, a function libxl_domain_need_memory_0x040c00, which will pass NULL for c_info. The code in libxl__need_memory_setdefault will use 0 for the two additional memory amounts when c_info is NULL. * The overall effect is that old callers will get the old behaviour. New callers get the new right behaviour. This is the same as the present libxl 4.13 code. Note that libxl_domain_need_memory already has an API stability caveat. * Consequently, the need for libxl_get_required_shadow_memory and libxl_get_required_iommu_memory goes away. Delete them (they have not been in any release so we can just do this). * Invent a new value for c_info->passthrough "enabled". Defaulting will be 1. turn "unknown" into "disabled" or "enabled" according to the current logic based on pcidevs/dtdefs; 2. turn "enabled" into something specific according to the current logic based on type, hap_pt_share, etc. Make sure this is all correct inside libxl. * Delete the defaulting code in xl. xl can just leave settings not specified by the user as blank, and libxl will DTRT with them. What do people think ? I really want to fix this for 4.13 because the current 4.13 API is not one I want to support. > Perhaps it would be reasonable to unify the create and build info at > a libxl level (even though they may feed into distinct domctls > underneath for the moment)? Yes, but we are probably too late for such an API change at this point for 4.13. Thanks, Ian.
On Thu, Oct 03, 2019 at 02:30:31PM +0100, Ian Jackson wrote: > Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): > > On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote: > > > I am continuing to look at the defaulting and config management here > > > with a view to getting rid of some of the duplicated code and moving > > > it all into libxl. > > > > That would indeed be beneficial for the likes of libvirt. > > I propose the following plan for 4.13: > > * Move the default calculations of b_info->shadow_memkb and > b_info->iommu_memkb from xl_vmcontrol.c into libxl, in a new > function libxl__need_memory_setdefault, called from > initiate_domain_create. That has access to the whole of c_info and > b_info. > > * Change the API/ABI for libxl_domain_need_memory to take a > libxl_domain_config. Internally, this will call an implementation > function libxl__domain_need_memory which takes the b_info and > c_info separately, and which calls libxl__need_memory_setdefault. > (This is the only other call site for > libxl__domain_build_info_setdefault.) > > * There will be the usual backward compatible arrangement: here, a > function libxl_domain_need_memory_0x040c00, which will pass NULL > for c_info. The code in libxl__need_memory_setdefault will use 0 > for the two additional memory amounts when c_info is NULL. > > * The overall effect is that old callers will get the old behaviour. > New callers get the new right behaviour. This is the same as the > present libxl 4.13 code. Note that libxl_domain_need_memory > already has an API stability caveat. > > * Consequently, the need for libxl_get_required_shadow_memory and > libxl_get_required_iommu_memory goes away. Delete them (they have > not been in any release so we can just do this). libxl_get_required_shadow_memory is old, and libvirt is using. Only libxl_get_required_iommu_memory is new. > * Invent a new value for c_info->passthrough "enabled". Defaulting > will be 1. turn "unknown" into "disabled" or "enabled" according to > the current logic based on pcidevs/dtdefs; 2. turn "enabled" into > something specific according to the current logic based on type, > hap_pt_share, etc. Make sure this is all correct inside libxl. > > * Delete the defaulting code in xl. xl can just leave settings not > specified by the user as blank, and libxl will DTRT with them. > > What do people think ? I really want to fix this for 4.13 because the > current 4.13 API is not one I want to support. That plan sound fine to me.
On 03.10.19 15:30, Ian Jackson wrote: > Paul Durrant writes ("Re: [Xen-devel] [PATCH-for-4.13 v2 0/2] libxl: fix assertion failure"): >> On Wed, 2 Oct 2019 at 17:04, Ian Jackson <ian.jackson@citrix.com> wrote: >>> I am continuing to look at the defaulting and config management here >>> with a view to getting rid of some of the duplicated code and moving >>> it all into libxl. >> >> That would indeed be beneficial for the likes of libvirt. > > I propose the following plan for 4.13: Fine with me. Juergen