|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx] > Sent: 25 April 2015 01:12 > To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx > Cc: Keir (Xen.org); Jan Beulich > Subject: Re: [PATCH 1/3] x86/hvm: give HVMOP_set_param and > HVMOP_get_param their own functions > > On 24/04/15 17:21, Paul Durrant wrote: > > The level of switch nesting in those ops is getting unreadable. Giving > > them their own functions does introduce some code duplication in the > > the pre-op checks but the overall result is easier to follow. > > > > This patch is code movement. There is no functional change. > > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx> > > Cc: Keir Fraser <keir@xxxxxxx> > > Cc: Jan Beulich <jbeulich@xxxxxxxx> > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > > Please latch current at the top of the function, and fix Xen style > during motion. (newlines between break and case lines, drop superfluous > braces, brace layout around get bufiorq_evtchn). > Sure, will do. > With that fixed, Reviewed-by: Andrew Cooper > <andrew.cooper3@xxxxxxxxxx> > Cool. Paul > > --- > > xen/arch/x86/hvm/hvm.c | 562 ++++++++++++++++++++++++++---------- > ------------ > > 1 file changed, 300 insertions(+), 262 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > index 3c62b80..87c47b1 100644 > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -5640,6 +5640,300 @@ static int hvmop_set_evtchn_upcall_vector( > > return 0; > > } > > > > +static int hvmop_set_param( > > + XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg) > > +{ > > + struct xen_hvm_param a; > > + struct domain *d; > > + struct vcpu *v; > > + int rc = 0; > > + > > + if ( copy_from_guest(&a, arg, 1) ) > > + return -EFAULT; > > + > > + if ( a.index >= HVM_NR_PARAMS ) > > + return -EINVAL; > > + > > + d = rcu_lock_domain_by_any_id(a.domid); > > + if ( d == NULL ) > > + return -ESRCH; > > + > > + rc = -EINVAL; > > + if ( !has_hvm_container_domain(d) ) > > + goto out; > > + > > + if ( is_pvh_domain(d) > > + && (a.index != HVM_PARAM_CALLBACK_IRQ) ) > > + goto out; > > + > > + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param); > > + if ( rc ) > > + goto out; > > + > > + switch ( a.index ) > > + { > > + case HVM_PARAM_CALLBACK_IRQ: > > + hvm_set_callback_via(d, a.value); > > + hvm_latch_shinfo_size(d); > > + break; > > + case HVM_PARAM_TIMER_MODE: > > + if ( a.value > HVMPTM_one_missed_tick_pending ) > > + rc = -EINVAL; > > + break; > > + case HVM_PARAM_VIRIDIAN: > > + /* This should only ever be set once by the tools and read by the > guest. */ > > + rc = -EPERM; > > + if ( d == current->domain ) > > + break; > > + > > + if ( a.value != d->arch.hvm_domain.params[a.index] ) > > + { > > + rc = -EEXIST; > > + if ( d->arch.hvm_domain.params[a.index] != 0 ) > > + break; > > + > > + rc = -EINVAL; > > + if ( (a.value & ~HVMPV_feature_mask) || > > + !(a.value & HVMPV_base_freq) ) > > + break; > > + } > > + > > + rc = 0; > > + break; > > + case HVM_PARAM_IDENT_PT: > > + /* Not reflexive, as we must domain_pause(). */ > > + rc = -EPERM; > > + if ( d == current->domain ) > > + break; > > + > > + rc = -EINVAL; > > + if ( d->arch.hvm_domain.params[a.index] != 0 ) > > + break; > > + > > + rc = 0; > > + if ( !paging_mode_hap(d) ) > > + break; > > + > > + /* > > + * Update GUEST_CR3 in each VMCS to point at identity map. > > + * All foreign updates to guest state must synchronise on > > + * the domctl_lock. > > + */ > > + rc = -ERESTART; > > + if ( !domctl_lock_acquire() ) > > + break; > > + > > + rc = 0; > > + domain_pause(d); > > + d->arch.hvm_domain.params[a.index] = a.value; > > + for_each_vcpu ( d, v ) > > + paging_update_cr3(v); > > + domain_unpause(d); > > + > > + domctl_lock_release(); > > + break; > > + case HVM_PARAM_DM_DOMAIN: > > + /* Not reflexive, as we may need to domain_pause(). */ > > + rc = -EPERM; > > + if ( d == current->domain ) > > + break; > > + > > + if ( a.value == DOMID_SELF ) > > + a.value = current->domain->domain_id; > > + > > + rc = hvm_set_dm_domain(d, a.value); > > + break; > > + case HVM_PARAM_ACPI_S_STATE: > > + /* Not reflexive, as we must domain_pause(). */ > > + rc = -EPERM; > > + if ( current->domain == d ) > > + break; > > + > > + rc = 0; > > + if ( a.value == 3 ) > > + hvm_s3_suspend(d); > > + else if ( a.value == 0 ) > > + hvm_s3_resume(d); > > + else > > + rc = -EINVAL; > > + > > + break; > > + case HVM_PARAM_ACPI_IOPORTS_LOCATION: > > + rc = pmtimer_change_ioport(d, a.value); > > + break; > > + case HVM_PARAM_MEMORY_EVENT_CR0: > > + case HVM_PARAM_MEMORY_EVENT_CR3: > > + case HVM_PARAM_MEMORY_EVENT_CR4: > > + if ( d == current->domain ) > > + rc = -EPERM; > > + break; > > + case HVM_PARAM_MEMORY_EVENT_INT3: > > + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: > > + case HVM_PARAM_MEMORY_EVENT_MSR: > > + if ( d == current->domain ) > > + { > > + rc = -EPERM; > > + break; > > + } > > + if ( a.value & HVMPME_onchangeonly ) > > + rc = -EINVAL; > > + break; > > + case HVM_PARAM_NESTEDHVM: > > + rc = xsm_hvm_param_nested(XSM_PRIV, d); > > + if ( rc ) > > + break; > > + if ( a.value > 1 ) > > + rc = -EINVAL; > > + /* Remove the check below once we have > > + * shadow-on-shadow. > > + */ > > + if ( cpu_has_svm && !paging_mode_hap(d) && a.value ) > > + rc = -EINVAL; > > + /* Set up NHVM state for any vcpus that are already up */ > > + if ( a.value && > > + !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] ) > > + for_each_vcpu(d, v) > > + if ( rc == 0 ) > > + rc = nestedhvm_vcpu_initialise(v); > > + if ( !a.value || rc ) > > + for_each_vcpu(d, v) > > + nestedhvm_vcpu_destroy(v); > > + break; > > + case HVM_PARAM_BUFIOREQ_EVTCHN: > > + rc = -EINVAL; > > + break; > > + case HVM_PARAM_TRIPLE_FAULT_REASON: > > + if ( a.value > SHUTDOWN_MAX ) > > + rc = -EINVAL; > > + break; > > + case HVM_PARAM_IOREQ_SERVER_PFN: > > + if ( d == current->domain ) > > + { > > + rc = -EPERM; > > + break; > > + } > > + d->arch.hvm_domain.ioreq_gmfn.base = a.value; > > + break; > > + case HVM_PARAM_NR_IOREQ_SERVER_PAGES: > > + { > > + unsigned int i; > > + > > + if ( d == current->domain ) > > + { > > + rc = -EPERM; > > + break; > > + } > > + if ( a.value == 0 || > > + a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 ) > > + { > > + rc = -EINVAL; > > + break; > > + } > > + for ( i = 0; i < a.value; i++ ) > > + set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask); > > + > > + break; > > + } > > + } > > + > > + if ( rc != 0 ) > > + goto out; > > + > > + d->arch.hvm_domain.params[a.index] = a.value; > > + > > + switch( a.index ) > > + { > > + case HVM_PARAM_MEMORY_EVENT_INT3: > > + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: > > + { > > + domain_pause(d); > > + domain_unpause(d); /* Causes guest to latch new status */ > > + break; > > + } > > + case HVM_PARAM_MEMORY_EVENT_CR3: > > + { > > + for_each_vcpu ( d, v ) > > + hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 mask > through CR0 code */ > > + break; > > + } > > + } > > + > > + HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64, > > + a.index, a.value); > > + > > + out: > > + rcu_unlock_domain(d); > > + return rc; > > +} > > + > > +static int hvmop_get_param( > > + XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg) > > +{ > > + struct xen_hvm_param a; > > + struct domain *d; > > + int rc = 0; > > + > > + if ( copy_from_guest(&a, arg, 1) ) > > + return -EFAULT; > > + > > + if ( a.index >= HVM_NR_PARAMS ) > > + return -EINVAL; > > + > > + d = rcu_lock_domain_by_any_id(a.domid); > > + if ( d == NULL ) > > + return -ESRCH; > > + > > + rc = -EINVAL; > > + if ( !has_hvm_container_domain(d) ) > > + goto out; > > + > > + if ( is_pvh_domain(d) > > + && (a.index != HVM_PARAM_CALLBACK_IRQ) ) > > + goto out; > > + > > + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param); > > + if ( rc ) > > + goto out; > > + > > + switch ( a.index ) > > + { > > + case HVM_PARAM_ACPI_S_STATE: > > + a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0; > > + break; > > + case HVM_PARAM_IOREQ_SERVER_PFN: > > + case HVM_PARAM_NR_IOREQ_SERVER_PAGES: > > + if ( d == current->domain ) > > + { > > + rc = -EPERM; > > + goto out; > > + } > > + case HVM_PARAM_IOREQ_PFN: > > + case HVM_PARAM_BUFIOREQ_PFN: > > + case HVM_PARAM_BUFIOREQ_EVTCHN: { > > + domid_t domid; > > + > > + /* May need to create server */ > > + domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; > > + rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL); > > + if ( rc != 0 && rc != -EEXIST ) > > + goto out; > > + /*FALLTHRU*/ > > + } > > + default: > > + a.value = d->arch.hvm_domain.params[a.index]; > > + break; > > + } > > + > > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > > + > > + HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64, > > + a.index, a.value); > > + > > + out: > > + rcu_unlock_domain(d); > > + return rc; > > +} > > + > > /* > > * Note that this value is effectively part of the ABI, even if we don't > > need > > * to make it a formal part of it: A guest suspended for migration in the > > @@ -5651,7 +5945,6 @@ static int hvmop_set_evtchn_upcall_vector( > > long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) > arg) > > > > { > > - struct domain *curr_d = current->domain; > > unsigned long start_iter, mask; > > long rc = 0; > > > > @@ -5705,269 +5998,14 @@ long do_hvm_op(unsigned long op, > XEN_GUEST_HANDLE_PARAM(void) arg) > > break; > > > > case HVMOP_set_param: > > - case HVMOP_get_param: > > - { > > - struct xen_hvm_param a; > > - struct domain *d; > > - struct vcpu *v; > > - > > - if ( copy_from_guest(&a, arg, 1) ) > > - return -EFAULT; > > - > > - if ( a.index >= HVM_NR_PARAMS ) > > - return -EINVAL; > > - > > - d = rcu_lock_domain_by_any_id(a.domid); > > - if ( d == NULL ) > > - return -ESRCH; > > - > > - rc = -EINVAL; > > - if ( !has_hvm_container_domain(d) ) > > - goto param_fail; > > - > > - if ( is_pvh_domain(d) > > - && (a.index != HVM_PARAM_CALLBACK_IRQ) ) > > - goto param_fail; > > - > > - rc = xsm_hvm_param(XSM_TARGET, d, op); > > - if ( rc ) > > - goto param_fail; > > - > > - if ( op == HVMOP_set_param ) > > - { > > - rc = 0; > > - > > - switch ( a.index ) > > - { > > - case HVM_PARAM_CALLBACK_IRQ: > > - hvm_set_callback_via(d, a.value); > > - hvm_latch_shinfo_size(d); > > - break; > > - case HVM_PARAM_TIMER_MODE: > > - if ( a.value > HVMPTM_one_missed_tick_pending ) > > - rc = -EINVAL; > > - break; > > - case HVM_PARAM_VIRIDIAN: > > - /* This should only ever be set once by the tools and read > > by the > guest. */ > > - rc = -EPERM; > > - if ( curr_d == d ) > > - break; > > - > > - if ( a.value != d->arch.hvm_domain.params[a.index] ) > > - { > > - rc = -EEXIST; > > - if ( d->arch.hvm_domain.params[a.index] != 0 ) > > - break; > > - > > - rc = -EINVAL; > > - if ( (a.value & ~HVMPV_feature_mask) || > > - !(a.value & HVMPV_base_freq) ) > > - break; > > - } > > - > > - rc = 0; > > - break; > > - case HVM_PARAM_IDENT_PT: > > - /* Not reflexive, as we must domain_pause(). */ > > - rc = -EPERM; > > - if ( curr_d == d ) > > - break; > > - > > - rc = -EINVAL; > > - if ( d->arch.hvm_domain.params[a.index] != 0 ) > > - break; > > - > > - rc = 0; > > - if ( !paging_mode_hap(d) ) > > - break; > > - > > - /* > > - * Update GUEST_CR3 in each VMCS to point at identity map. > > - * All foreign updates to guest state must synchronise on > > - * the domctl_lock. > > - */ > > - rc = -ERESTART; > > - if ( !domctl_lock_acquire() ) > > - break; > > - > > - rc = 0; > > - domain_pause(d); > > - d->arch.hvm_domain.params[a.index] = a.value; > > - for_each_vcpu ( d, v ) > > - paging_update_cr3(v); > > - domain_unpause(d); > > - > > - domctl_lock_release(); > > - break; > > - case HVM_PARAM_DM_DOMAIN: > > - /* Not reflexive, as we may need to domain_pause(). */ > > - rc = -EPERM; > > - if ( curr_d == d ) > > - break; > > - > > - if ( a.value == DOMID_SELF ) > > - a.value = curr_d->domain_id; > > - > > - rc = hvm_set_dm_domain(d, a.value); > > - break; > > - case HVM_PARAM_ACPI_S_STATE: > > - /* Not reflexive, as we must domain_pause(). */ > > - rc = -EPERM; > > - if ( curr_d == d ) > > - break; > > - > > - rc = 0; > > - if ( a.value == 3 ) > > - hvm_s3_suspend(d); > > - else if ( a.value == 0 ) > > - hvm_s3_resume(d); > > - else > > - rc = -EINVAL; > > - > > - break; > > - case HVM_PARAM_ACPI_IOPORTS_LOCATION: > > - rc = pmtimer_change_ioport(d, a.value); > > - break; > > - case HVM_PARAM_MEMORY_EVENT_CR0: > > - case HVM_PARAM_MEMORY_EVENT_CR3: > > - case HVM_PARAM_MEMORY_EVENT_CR4: > > - if ( d == current->domain ) > > - rc = -EPERM; > > - break; > > - case HVM_PARAM_MEMORY_EVENT_INT3: > > - case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: > > - case HVM_PARAM_MEMORY_EVENT_MSR: > > - if ( d == current->domain ) > > - { > > - rc = -EPERM; > > - break; > > - } > > - if ( a.value & HVMPME_onchangeonly ) > > - rc = -EINVAL; > > - break; > > - case HVM_PARAM_NESTEDHVM: > > - rc = xsm_hvm_param_nested(XSM_PRIV, d); > > - if ( rc ) > > - break; > > - if ( a.value > 1 ) > > - rc = -EINVAL; > > - /* Remove the check below once we have > > - * shadow-on-shadow. > > - */ > > - if ( cpu_has_svm && !paging_mode_hap(d) && a.value ) > > - rc = -EINVAL; > > - /* Set up NHVM state for any vcpus that are already up */ > > - if ( a.value && > > - !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] ) > > - for_each_vcpu(d, v) > > - if ( rc == 0 ) > > - rc = nestedhvm_vcpu_initialise(v); > > - if ( !a.value || rc ) > > - for_each_vcpu(d, v) > > - nestedhvm_vcpu_destroy(v); > > - break; > > - case HVM_PARAM_BUFIOREQ_EVTCHN: > > - rc = -EINVAL; > > - break; > > - case HVM_PARAM_TRIPLE_FAULT_REASON: > > - if ( a.value > SHUTDOWN_MAX ) > > - rc = -EINVAL; > > - break; > > - case HVM_PARAM_IOREQ_SERVER_PFN: > > - if ( d == current->domain ) > > - { > > - rc = -EPERM; > > - break; > > - } > > - d->arch.hvm_domain.ioreq_gmfn.base = a.value; > > - break; > > - case HVM_PARAM_NR_IOREQ_SERVER_PAGES: > > - { > > - unsigned int i; > > - > > - if ( d == current->domain ) > > - { > > - rc = -EPERM; > > - break; > > - } > > - if ( a.value == 0 || > > - a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) > > * 8 ) > > - { > > - rc = -EINVAL; > > - break; > > - } > > - for ( i = 0; i < a.value; i++ ) > > - set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask); > > - > > - break; > > - } > > - } > > - > > - if ( rc == 0 ) > > - { > > - d->arch.hvm_domain.params[a.index] = a.value; > > - > > - switch( a.index ) > > - { > > - case HVM_PARAM_MEMORY_EVENT_INT3: > > - case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP: > > - { > > - domain_pause(d); > > - domain_unpause(d); /* Causes guest to latch new status > > */ > > - break; > > - } > > - case HVM_PARAM_MEMORY_EVENT_CR3: > > - { > > - for_each_vcpu ( d, v ) > > - hvm_funcs.update_guest_cr(v, 0); /* Latches new > > CR3 mask > through CR0 code */ > > - break; > > - } > > - } > > - > > - } > > - > > - } > > - else > > - { > > - switch ( a.index ) > > - { > > - case HVM_PARAM_ACPI_S_STATE: > > - a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0; > > - break; > > - case HVM_PARAM_IOREQ_SERVER_PFN: > > - case HVM_PARAM_NR_IOREQ_SERVER_PAGES: > > - if ( d == current->domain ) > > - { > > - rc = -EPERM; > > - break; > > - } > > - case HVM_PARAM_IOREQ_PFN: > > - case HVM_PARAM_BUFIOREQ_PFN: > > - case HVM_PARAM_BUFIOREQ_EVTCHN: { > > - domid_t domid; > > - > > - /* May need to create server */ > > - domid = d- > >arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN]; > > - rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL); > > - if ( rc != 0 && rc != -EEXIST ) > > - goto param_fail; > > - /*FALLTHRU*/ > > - } > > - default: > > - a.value = d->arch.hvm_domain.params[a.index]; > > - break; > > - } > > - rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > > - } > > - > > - HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64, > > - op == HVMOP_set_param ? "set" : "get", > > - a.index, a.value); > > + rc = hvmop_set_param( > > + guest_handle_cast(arg, xen_hvm_param_t)); > > + break; > > > > - param_fail: > > - rcu_unlock_domain(d); > > + case HVMOP_get_param: > > + rc = hvmop_get_param( > > + guest_handle_cast(arg, xen_hvm_param_t)); > > break; > > - } > > > > case HVMOP_set_pci_intx_level: > > rc = hvmop_set_pci_intx_level( _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |