| Message ID | 20200206132452.11802-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/hvm: Fix handling of obsolete HVM_PARAMs | expand |
On 06.02.2020 14:24, Andrew Cooper wrote: > The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's > behaviour for the MEMORY_EVENT params, but is wrong for the get side, where > Xen would return 0 (which is also a bug). Delete the helper. > > In Xen, perform the checks in hvm_allow_set_param(), rather than > hvm_set_param(), and actually implement checks on the get side so the > hypercall doesn't return successfully with 0 as an answer. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Hypervisor parts Acked-by: Jan Beulich <jbeulich@suse.com> with one remark: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4105,8 +4105,14 @@ static int hvm_allow_set_param(struct domain *d, > break; > /* The following parameters are deprecated. */ > case HVM_PARAM_DM_DOMAIN: > + case HVM_PARAM_MEMORY_EVENT_CR0: > + case HVM_PARAM_MEMORY_EVENT_CR3: > + case HVM_PARAM_MEMORY_EVENT_CR4: > + case HVM_PARAM_MEMORY_EVENT_INT3: > + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: > case HVM_PARAM_BUFIOREQ_EVTCHN: > - rc = -EPERM; > + case HVM_PARAM_MEMORY_EVENT_MSR: > + rc = -EINVAL; > break; Other than in the header, where sorting by numeric value makes (more) sense, I think it would be better to sort case labels alphabetically. Otherwise when wanting to add to it, one needs to go look up (or memorize, from doing the header file change first) the insertion point. Jan
On Thu, Feb 6, 2020 at 6:24 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's > behaviour for the MEMORY_EVENT params, but is wrong for the get side, where > Xen would return 0 (which is also a bug). Delete the helper. > > In Xen, perform the checks in hvm_allow_set_param(), rather than > hvm_set_param(), and actually implement checks on the get side so the > hypercall doesn't return successfully with 0 as an answer. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Ian Jackson <Ian.Jackson@citrix.com> > CC: Razvan Cojocaru <rcojocaru@bitdefender.com> > CC: Tamas K Lengyel <tamas@tklengyel.com> > > Tamas: You introduced xc_hvm_param_deprecated_check() in 0a246b41ca while > introducing XEN_DOMCTL_monitor_op. Do you recall why? Well, from the looks of it the intent was to tell any remaining user of the deprecated HVM params that these are no longer supported since we now have a separate domctl, the monitor_op, to set these options. Tamas
On Thu, Feb 06, 2020 at 01:24:52PM +0000, Andrew Cooper wrote: > The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's > behaviour for the MEMORY_EVENT params, but is wrong for the get side, where > Xen would return 0 (which is also a bug). Delete the helper. > > In Xen, perform the checks in hvm_allow_set_param(), rather than > hvm_set_param(), and actually implement checks on the get side so the > hypercall doesn't return successfully with 0 as an answer. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> OK. Now the check is pushed down to hypervisor. I'm certainly happy to have less code in libxc. Acked-by: Wei Liu <wl@xen.org>
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c index e544218d2e..71829c2bce 100644 --- a/tools/libxc/xc_domain.c +++ b/tools/libxc/xc_domain.c @@ -1441,31 +1441,10 @@ int xc_domain_send_trigger(xc_interface *xch, return do_domctl(xch, &domctl); } -static inline int xc_hvm_param_deprecated_check(uint32_t param) -{ - switch ( param ) - { - case HVM_PARAM_MEMORY_EVENT_CR0: - case HVM_PARAM_MEMORY_EVENT_CR3: - case HVM_PARAM_MEMORY_EVENT_CR4: - case HVM_PARAM_MEMORY_EVENT_INT3: - case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: - case HVM_PARAM_MEMORY_EVENT_MSR: - return -EOPNOTSUPP; - default: - break; - }; - - return 0; -} - int xc_hvm_param_set(xc_interface *handle, uint32_t dom, uint32_t param, uint64_t value) { DECLARE_HYPERCALL_BUFFER(xen_hvm_param_t, arg); - int rc = xc_hvm_param_deprecated_check(param); - - if ( rc ) - return rc; + int rc; arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); if ( arg == NULL ) @@ -1484,10 +1463,7 @@ int xc_hvm_param_set(xc_interface *handle, uint32_t dom, uint32_t param, uint64_ int xc_hvm_param_get(xc_interface *handle, uint32_t dom, uint32_t param, uint64_t *value) { DECLARE_HYPERCALL_BUFFER(xen_hvm_param_t, arg); - int rc = xc_hvm_param_deprecated_check(param); - - if ( rc ) - return rc; + int rc; arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); if ( arg == NULL ) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 00a9e70b7c..93795dab92 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4105,8 +4105,14 @@ static int hvm_allow_set_param(struct domain *d, break; /* The following parameters are deprecated. */ case HVM_PARAM_DM_DOMAIN: + case HVM_PARAM_MEMORY_EVENT_CR0: + case HVM_PARAM_MEMORY_EVENT_CR3: + case HVM_PARAM_MEMORY_EVENT_CR4: + case HVM_PARAM_MEMORY_EVENT_INT3: + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: case HVM_PARAM_BUFIOREQ_EVTCHN: - rc = -EPERM; + case HVM_PARAM_MEMORY_EVENT_MSR: + rc = -EINVAL; break; /* * The following parameters must not be set by the guest @@ -4221,15 +4227,6 @@ static int hvm_set_param(struct domain *d, uint32_t index, uint64_t value) case HVM_PARAM_ACPI_IOPORTS_LOCATION: rc = pmtimer_change_ioport(d, value); break; - case HVM_PARAM_MEMORY_EVENT_CR0: - case HVM_PARAM_MEMORY_EVENT_CR3: - case HVM_PARAM_MEMORY_EVENT_CR4: - case HVM_PARAM_MEMORY_EVENT_INT3: - case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: - case HVM_PARAM_MEMORY_EVENT_MSR: - /* Deprecated */ - rc = -EOPNOTSUPP; - break; case HVM_PARAM_NESTEDHVM: rc = xsm_hvm_param_nested(XSM_PRIV, d); if ( rc ) @@ -4411,8 +4408,14 @@ static int hvm_allow_get_param(struct domain *d, break; /* The following parameters are deprecated. */ case HVM_PARAM_DM_DOMAIN: + case HVM_PARAM_MEMORY_EVENT_CR0: + case HVM_PARAM_MEMORY_EVENT_CR3: + case HVM_PARAM_MEMORY_EVENT_CR4: + case HVM_PARAM_MEMORY_EVENT_INT3: + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: case HVM_PARAM_BUFIOREQ_EVTCHN: - rc = -ENODATA; + case HVM_PARAM_MEMORY_EVENT_MSR: + rc = -EINVAL; break; /* The remaining parameters should not be read by the guest. */ default: diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index 36832e4b94..68293e314e 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -28,8 +28,14 @@ /* These parameters are deprecated and their meaning is undefined. */ #if defined(__XEN__) || defined(__XEN_TOOLS__) -#define HVM_PARAM_DM_DOMAIN 13 -#define HVM_PARAM_BUFIOREQ_EVTCHN 26 +#define HVM_PARAM_DM_DOMAIN 13 +#define HVM_PARAM_MEMORY_EVENT_CR0 20 +#define HVM_PARAM_MEMORY_EVENT_CR3 21 +#define HVM_PARAM_MEMORY_EVENT_CR4 22 +#define HVM_PARAM_MEMORY_EVENT_INT3 23 +#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP 25 +#define HVM_PARAM_BUFIOREQ_EVTCHN 26 +#define HVM_PARAM_MEMORY_EVENT_MSR 30 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ @@ -227,14 +233,6 @@ */ #define HVM_PARAM_ACPI_IOPORTS_LOCATION 19 -/* Deprecated */ -#define HVM_PARAM_MEMORY_EVENT_CR0 20 -#define HVM_PARAM_MEMORY_EVENT_CR3 21 -#define HVM_PARAM_MEMORY_EVENT_CR4 22 -#define HVM_PARAM_MEMORY_EVENT_INT3 23 -#define HVM_PARAM_MEMORY_EVENT_SINGLE_STEP 25 -#define HVM_PARAM_MEMORY_EVENT_MSR 30 - /* Boolean: Enable nestedhvm (hvm only) */ #define HVM_PARAM_NESTEDHVM 24
The local xc_hvm_param_deprecated_check() in libxc tries to guess Xen's behaviour for the MEMORY_EVENT params, but is wrong for the get side, where Xen would return 0 (which is also a bug). Delete the helper. In Xen, perform the checks in hvm_allow_set_param(), rather than hvm_set_param(), and actually implement checks on the get side so the hypercall doesn't return successfully with 0 as an answer. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Ian Jackson <Ian.Jackson@citrix.com> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> CC: Tamas K Lengyel <tamas@tklengyel.com> Tamas: You introduced xc_hvm_param_deprecated_check() in 0a246b41ca while introducing XEN_DOMCTL_monitor_op. Do you recall why? --- tools/libxc/xc_domain.c | 28 ++-------------------------- xen/arch/x86/hvm/hvm.c | 25 ++++++++++++++----------- xen/include/public/hvm/params.h | 18 ++++++++---------- 3 files changed, 24 insertions(+), 47 deletions(-)