| Message ID | 20240508112323.38946-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: support foreign mappings for HVM/PVH | expand |
On 08/05/2024 12:23 pm, Roger Pau Monne wrote: > Enabling it using an HVM param is fragile, and complicates the logic when > deciding whether options that interact with altp2m can also be enabled. > > Leave the HVM param value for consumption by the guest, but prevent it from > being set. Enabling is now done using the misc_flags field in > xen_arch_domainconfig. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - New in this version. Ha. So this is actually work that Petr has been wanting to do. Petr has a series hoping to make it into 4.19 (x86: Make MAX_ALTP2M configurable), which just missed out on this side of things. altp2m is not architecture specific at all, and there's even support for ARM out on the mailing list. Therefore, the altp2m mode wants to be common, just like the new MAX_ALTP2M setting already is. Both fields can reasonably share uint32_t, but could you work with Petr to make both halfs of this land cleanly. As to the HVMPARAM, I'd really quite like to delete it. It was always a bodge, and there's a full set of HVMOP_altp2m_* for a guest to use. > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 20e83cf38bbd..dff790060605 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -708,13 +711,33 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) > } > } > > - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED ) > + if ( config->arch.misc_flags & ~XEN_X86_MISC_FLAGS_ALL ) > { > dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n", > config->arch.misc_flags); > return -EINVAL; > } > > + if ( altp2m && (altp2m & (altp2m - 1)) ) > + { > + dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: %#x\n", > + config->flags); > + return -EINVAL; I think this would be clearer to follow by having a 2 bit field called altp2m_mode and check for <= 2. > + } > + > + if ( altp2m && nested_virt ) > + { > + dprintk(XENLOG_INFO, > + "Nested virt and altp2m are mutually incompatible\n"); There's nothing inherently incompatible. I think it's more that noone had any interest in trying to make it work in combination with nested p2ms. I'd phrase it as "not supported", rather than incompatible. > + return -EINVAL; > + } > + > + if ( altp2m && !hap ) > + { > + dprintk(XENLOG_INFO, "altp2m requires HAP\n"); > + return -EINVAL; > + } altp2m ought to work fine with shadow. It's only if you want VMFUNC/#VE acceleration that you depend on EPT. Again, I'd phrase this as "not supported". ~Andrew
On Wed, May 8, 2024 at 9:38 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > Both fields can reasonably share uint32_t, but could you work with Petr > to make both halfs of this land cleanly. Hi, I think creating a new anonymous struct "altp2m" within `struct domain` would be a good fit. uint16_t for my MAX_ALTP2M replacement field will be enough, so the other uint16_t could be used for various altp2m modes/flags. P.
On Wed, May 08, 2024 at 08:38:07PM +0100, Andrew Cooper wrote: > On 08/05/2024 12:23 pm, Roger Pau Monne wrote: > > Enabling it using an HVM param is fragile, and complicates the logic when > > deciding whether options that interact with altp2m can also be enabled. > > > > Leave the HVM param value for consumption by the guest, but prevent it from > > being set. Enabling is now done using the misc_flags field in > > xen_arch_domainconfig. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - New in this version. > > Ha. So this is actually work that Petr has been wanting to do. > > Petr has a series hoping to make it into 4.19 (x86: Make MAX_ALTP2M > configurable), which just missed out on this side of things. > > altp2m is not architecture specific at all, and there's even support for > ARM out on the mailing list. Therefore, the altp2m mode wants to be > common, just like the new MAX_ALTP2M setting already is. Initially I had it as a set of XEN_DOMCTL_CDF_* flags, but it wasn't clear to me whether the modes could be shared between arches. > Both fields can reasonably share uint32_t, but could you work with Petr > to make both halfs of this land cleanly. I'm happy for Petr to pick this patch as part of the series if he feels like. I assume the plan would be to add an XEN_DOMCTL_CDF_altp2m flag, and then a new field to signal the mode. > > As to the HVMPARAM, I'd really quite like to delete it. It was always a > bodge, and there's a full set of HVMOP_altp2m_* for a guest to use. I've assumed we must keep HVM_PARAM_ALTP2M for backwards compatibility. > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > > index 20e83cf38bbd..dff790060605 100644 > > --- a/xen/arch/x86/domain.c > > +++ b/xen/arch/x86/domain.c > > @@ -708,13 +711,33 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) > > } > > } > > > > - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED ) > > + if ( config->arch.misc_flags & ~XEN_X86_MISC_FLAGS_ALL ) > > { > > dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n", > > config->arch.misc_flags); > > return -EINVAL; > > } > > > > + if ( altp2m && (altp2m & (altp2m - 1)) ) > > + { > > + dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: %#x\n", > > + config->flags); > > + return -EINVAL; > > I think this would be clearer to follow by having a 2 bit field called > altp2m_mode and check for <= 2. Don't we need 3 bits, for mixed, external and limited modes? We could do with 2 bits if we signal altp2m enabled in a different field, and then introduce a field to just contain the mode. FWIW, the check should be `if ( altp2m & (altp2m - 1) )`. I had updated this, but seems like I missed to re-generate the patches. > > + } > > + > > + if ( altp2m && nested_virt ) > > + { > > + dprintk(XENLOG_INFO, > > + "Nested virt and altp2m are mutually incompatible\n"); > > There's nothing inherently incompatible. I think it's more that noone > had any interest in trying to make it work in combination with nested p2ms. > > I'd phrase it as "not supported", rather than incompatible. "Nested virt and altp2m are not supported together\n" > > + return -EINVAL; > > + } > > + > > + if ( altp2m && !hap ) > > + { > > + dprintk(XENLOG_INFO, "altp2m requires HAP\n"); > > + return -EINVAL; > > + } > > altp2m ought to work fine with shadow. It's only if you want VMFUNC/#VE > acceleration that you depend on EPT. > > Again, I'd phrase this as "not supported". "altp2m is only supported with HAP\n" To avoid the double negation of "not supported without HAP" wording. Thanks, Roger.
On 09.05.2024 10:23, Roger Pau Monné wrote: > On Wed, May 08, 2024 at 08:38:07PM +0100, Andrew Cooper wrote: >> On 08/05/2024 12:23 pm, Roger Pau Monne wrote: >>> Enabling it using an HVM param is fragile, and complicates the logic when >>> deciding whether options that interact with altp2m can also be enabled. >>> >>> Leave the HVM param value for consumption by the guest, but prevent it from >>> being set. Enabling is now done using the misc_flags field in >>> xen_arch_domainconfig. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> Changes since v1: >>> - New in this version. >> >> Ha. So this is actually work that Petr has been wanting to do. >> >> Petr has a series hoping to make it into 4.19 (x86: Make MAX_ALTP2M >> configurable), which just missed out on this side of things. >> >> altp2m is not architecture specific at all, and there's even support for >> ARM out on the mailing list. Therefore, the altp2m mode wants to be >> common, just like the new MAX_ALTP2M setting already is. > > Initially I had it as a set of XEN_DOMCTL_CDF_* flags, but it wasn't > clear to me whether the modes could be shared between arches. > >> Both fields can reasonably share uint32_t, but could you work with Petr >> to make both halfs of this land cleanly. > > I'm happy for Petr to pick this patch as part of the series if he > feels like. > > I assume the plan would be to add an XEN_DOMCTL_CDF_altp2m flag, and > then a new field to signal the mode. > >> >> As to the HVMPARAM, I'd really quite like to delete it. It was always a >> bodge, and there's a full set of HVMOP_altp2m_* for a guest to use. > > I've assumed we must keep HVM_PARAM_ALTP2M for backwards > compatibility. > >>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c >>> index 20e83cf38bbd..dff790060605 100644 >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -708,13 +711,33 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) >>> } >>> } >>> >>> - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED ) >>> + if ( config->arch.misc_flags & ~XEN_X86_MISC_FLAGS_ALL ) >>> { >>> dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n", >>> config->arch.misc_flags); >>> return -EINVAL; >>> } >>> >>> + if ( altp2m && (altp2m & (altp2m - 1)) ) >>> + { >>> + dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: %#x\n", >>> + config->flags); >>> + return -EINVAL; >> >> I think this would be clearer to follow by having a 2 bit field called >> altp2m_mode and check for <= 2. > > Don't we need 3 bits, for mixed, external and limited modes? > > We could do with 2 bits if we signal altp2m enabled in a different > field, and then introduce a field to just contain the mode. I think what Andrew meant is #define XEN_X86_ALTP2M_MIXED (1U << 1) /* Enable altp2m external mode. */ #define XEN_X86_ALTP2M_EXT (2U << 1) /* Enable altp2m limited mode. */ #define XEN_X86_ALTP2M_LIMITED (3U << 1) (leaving aside the x86-only vs common aspect). That would also eliminate the ability to request multiple (conflicting) modes. Jan
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c index a50ec37eb3eb..86ee8132536c 100644 --- a/tools/libs/light/libxl_x86.c +++ b/tools/libs/light/libxl_x86.c @@ -6,8 +6,21 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, libxl_domain_config *d_config, struct xen_domctl_createdomain *config) { + unsigned int altp2m = d_config->b_info.altp2m; + switch(d_config->c_info.type) { case LIBXL_DOMAIN_TYPE_HVM: + /* + * The config parameter "altp2m" replaces the parameter "altp2mhvm". + * For legacy reasons, both parameters are accepted on x86 HVM guests. + * + * If the legacy field info->u.hvm.altp2m is set, activate altp2m. + * Otherwise set altp2m based on the field info->altp2m. + */ + if (altp2m == LIBXL_ALTP2M_MODE_DISABLED && + libxl_defbool_val(d_config->b_info.u.hvm.altp2m)) + altp2m = libxl_defbool_val(d_config->b_info.u.hvm.altp2m); + config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI); if (!libxl_defbool_val(d_config->b_info.u.hvm.pirq)) config->arch.emulation_flags &= ~XEN_X86_EMU_USE_PIRQ; @@ -26,6 +39,22 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed)) config->arch.misc_flags |= XEN_X86_MSR_RELAXED; + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV) { + switch (altp2m) { + case LIBXL_ALTP2M_MODE_MIXED: + config->arch.misc_flags |= XEN_X86_ALTP2M_MIXED; + break; + + case LIBXL_ALTP2M_MODE_EXTERNAL: + config->arch.misc_flags |= XEN_X86_ALTP2M_EXT; + break; + + case LIBXL_ALTP2M_MODE_LIMITED: + config->arch.misc_flags |= XEN_X86_ALTP2M_LIMITED; + break; + } + } + return 0; } @@ -407,19 +436,9 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid, libxl_ctx *ctx = libxl__gc_owner(gc); xc_interface *xch = ctx->xch; int ret = ERROR_FAIL; - unsigned int altp2m = info->altp2m; switch(info->type) { case LIBXL_DOMAIN_TYPE_HVM: - /* The config parameter "altp2m" replaces the parameter "altp2mhvm". For - * legacy reasons, both parameters are accepted on x86 HVM guests. - * - * If the legacy field info->u.hvm.altp2m is set, activate altp2m. - * Otherwise set altp2m based on the field info->altp2m. */ - if (info->altp2m == LIBXL_ALTP2M_MODE_DISABLED && - libxl_defbool_val(info->u.hvm.altp2m)) - altp2m = libxl_defbool_val(info->u.hvm.altp2m); - if (xc_hvm_param_set(xch, domid, HVM_PARAM_HPET_ENABLED, libxl_defbool_val(info->u.hvm.hpet))) { LOG(ERROR, "Couldn't set HVM_PARAM_HPET_ENABLED"); @@ -444,10 +463,6 @@ static int hvm_set_conf_params(libxl__gc *gc, uint32_t domid, LOG(ERROR, "Couldn't set HVM_PARAM_TIMER_MODE"); goto out; } - if (xc_hvm_param_set(xch, domid, HVM_PARAM_ALTP2M, altp2m)) { - LOG(ERROR, "Couldn't set HVM_PARAM_ALTP2M"); - goto out; - } break; default: diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 20e83cf38bbd..dff790060605 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -637,6 +637,9 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) bool hap = config->flags & XEN_DOMCTL_CDF_hap; bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt; unsigned int max_vcpus; + unsigned int altp2m = config->arch.misc_flags & (XEN_X86_ALTP2M_MIXED | + XEN_X86_ALTP2M_EXT | + XEN_X86_ALTP2M_LIMITED); if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) ) { @@ -708,13 +711,33 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) } } - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED ) + if ( config->arch.misc_flags & ~XEN_X86_MISC_FLAGS_ALL ) { dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n", config->arch.misc_flags); return -EINVAL; } + if ( altp2m && (altp2m & (altp2m - 1)) ) + { + dprintk(XENLOG_INFO, "Multiple altp2m options selected in flags: %#x\n", + config->flags); + return -EINVAL; + } + + if ( altp2m && nested_virt ) + { + dprintk(XENLOG_INFO, + "Nested virt and altp2m are mutually incompatible\n"); + return -EINVAL; + } + + if ( altp2m && !hap ) + { + dprintk(XENLOG_INFO, "altp2m requires HAP\n"); + return -EINVAL; + } + return 0; } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 0ce45b177cf4..f3e1b9364588 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -659,6 +659,14 @@ int hvm_domain_initialise(struct domain *d, d->arch.hvm.params[HVM_PARAM_TRIPLE_FAULT_REASON] = SHUTDOWN_reboot; + /* Set altp2m based on domctl flags. */ + if ( config->arch.misc_flags & XEN_X86_ALTP2M_MIXED ) + d->arch.hvm.params[HVM_PARAM_ALTP2M] = XEN_ALTP2M_mixed; + else if ( config->arch.misc_flags & XEN_X86_ALTP2M_EXT ) + d->arch.hvm.params[HVM_PARAM_ALTP2M] = XEN_ALTP2M_external; + else if ( config->arch.misc_flags & XEN_X86_ALTP2M_LIMITED ) + d->arch.hvm.params[HVM_PARAM_ALTP2M] = XEN_ALTP2M_limited; + vpic_init(d); rc = vioapic_init(d); @@ -4163,6 +4171,12 @@ static int hvm_allow_set_param(struct domain *d, case HVM_PARAM_CONSOLE_EVTCHN: case HVM_PARAM_X87_FIP_WIDTH: break; + + /* The following parameters are read-only. */ + case HVM_PARAM_ALTP2M: + rc = -EEXIST; + break; + /* The following parameters are deprecated. */ case HVM_PARAM_PAE_ENABLED: case HVM_PARAM_DM_DOMAIN: @@ -4204,7 +4218,6 @@ static int hvm_allow_set_param(struct domain *d, case HVM_PARAM_BUFIOREQ_PFN: case HVM_PARAM_IOREQ_SERVER_PFN: case HVM_PARAM_NR_IOREQ_SERVER_PAGES: - case HVM_PARAM_ALTP2M: case HVM_PARAM_MCA_CAP: if ( value != 0 && new_value != value ) rc = -EEXIST; diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index a9a87d9b50de..bc444e7744a5 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -300,6 +300,24 @@ struct xen_arch_domainconfig { * doesn't allow the guest to read or write to the underlying MSR. */ #define XEN_X86_MSR_RELAXED (1u << 0) + +/* altp2m options. Only one can be set. + * + * Note that 'mixed' mode has not been evaluated for safety from a + * security perspective. Before using this mode in a + * security-critical environment, each subop should be evaluated for + * safety, with unsafe subops blacklisted in XSM. + * + * Enable altp2m mixed mode. + */ +#define XEN_X86_ALTP2M_MIXED (1U << 1) +/* Enable altp2m external mode. */ +#define XEN_X86_ALTP2M_EXT (1U << 2) +/* Enable altp2m limited mode. */ +#define XEN_X86_ALTP2M_LIMITED (1U << 3) + +#define XEN_X86_MISC_FLAGS_ALL (XEN_X86_MSR_RELAXED | XEN_X86_ALTP2M_MIXED | \ + XEN_X86_ALTP2M_EXT | XEN_X86_ALTP2M_LIMITED) uint32_t misc_flags; }; diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index a22b4ed45d2e..99c40b4287f1 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -246,16 +246,11 @@ #define HVM_PARAM_VM_GENERATION_ID_ADDR 34 /* - * Set mode for altp2m: - * disabled: don't activate altp2m (default) + * Get mode for altp2m: + * disabled: altp2m not active (default) * mixed: allow access to all altp2m ops for both in-guest and external tools * external: allow access to external privileged tools only * limited: guest only has limited access (ie. control VMFUNC and #VE) - * - * Note that 'mixed' mode has not been evaluated for safety from a - * security perspective. Before using this mode in a - * security-critical environment, each subop should be evaluated for - * safety, with unsafe subops blacklisted in XSM. */ #define HVM_PARAM_ALTP2M 35 #define XEN_ALTP2M_disabled 0
Enabling it using an HVM param is fragile, and complicates the logic when deciding whether options that interact with altp2m can also be enabled. Leave the HVM param value for consumption by the guest, but prevent it from being set. Enabling is now done using the misc_flags field in xen_arch_domainconfig. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- tools/libs/light/libxl_x86.c | 43 +++++++++++++++++++++---------- xen/arch/x86/domain.c | 25 +++++++++++++++++- xen/arch/x86/hvm/hvm.c | 15 ++++++++++- xen/include/public/arch-x86/xen.h | 18 +++++++++++++ xen/include/public/hvm/params.h | 9 ++----- 5 files changed, 87 insertions(+), 23 deletions(-)