|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V7 07/12] xen: Introduce monitor_op domctl
On Thu, Mar 26, 2015 at 11:50 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 12.03.15 at 18:58, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> @@ -91,41 +88,55 @@ static int hvm_event_traps(uint64_t parameters,
>> vm_event_request_t *req)
>> return 1;
>> }
>>
>> -static void hvm_event_cr(uint32_t reason, unsigned long value,
>> - unsigned long old, uint64_t parameters)
>> +static inline
>> +void hvm_event_cr(uint32_t reason, unsigned long value,
>> + unsigned long old, bool_t onchangeonly, bool_t
>> sync)
>> {
>> - vm_event_request_t req = {
>> - .reason = reason,
>> - .vcpu_id = current->vcpu_id,
>> - .u.mov_to_cr.new_value = value,
>> - .u.mov_to_cr.old_value = old
>> - };
>> -
>> - if ( (parameters & HVMPME_onchangeonly) && (value == old) )
>> + if ( onchangeonly && value == old )
>> + {
>> return;
>> -
>> - hvm_event_traps(parameters, &req);
>> + }
>> + else
>> + {
>> + vm_event_request_t req = {
>> + .reason = reason,
>> + .vcpu_id = current->vcpu_id,
>> + .u.mov_to_cr.new_value = value,
>> + .u.mov_to_cr.old_value = old
>> + };
>> +
>> + hvm_event_traps(sync, &req);
>> + }
>
> ... I'd really like to see such done without "else" (which would also
> have resulted in a smaller change).
Ack.
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5830,19 +5830,11 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> 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;
>> + /* Deprecated */
>> + rc = -EPERM;
>
> -EOPNOTSUPP would have been better, but anyway.
Ack.
>
>> @@ -5902,29 +5894,10 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> }
>> }
>>
>> - if ( rc == 0 )
>> + 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;
>> - }
>> - }
>> -
>> }
>> -
>
> There's a now pointless pair of braces left in place.
>
>> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>> +{
>> + int rc;
>> + struct arch_domain *ad = &d->arch;
>> +
>> + rc = xsm_vm_event_control(XSM_PRIV, d, mop->op, mop->event);
>> + if ( rc )
>> + return rc;
>> +
>> + /*
>> + * At the moment only Intel HVM domains are supported. However, event
>> + * delivery could be extended to AMD and PV domains.
>> + */
>> + if ( !is_hvm_domain(d) || !cpu_has_vmx )
>> + return -EOPNOTSUPP;
>> +
>> + /*
>> + * Sanity check
>> + */
>> + if ( mop->op != XEN_DOMCTL_MONITOR_OP_ENABLE &&
>> + mop->op != XEN_DOMCTL_MONITOR_OP_DISABLE )
>> + return -ENOSYS;
>
> Here and further down -EOPNOTSUPP would again be better - we
> try to use -ENOSYS only for hypercalls that aren't implemented at
> all, not for invalid sub-ops (but there may still be a few violators
> in the tree).
Ack.
>
>> case XEN_VM_EVENT_MONITOR_DISABLE:
>> {
>> if ( ved->ring_page )
>> {
>> rc = vm_event_disable(d, ved);
>> - d->arch.hvm_domain.introspection_enabled = 0;
>> }
>
> Again leaving in place a now pointless pair of braces.
>
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -333,12 +333,32 @@ struct arch_domain
>> struct e820entry *e820;
>> unsigned int nr_e820;
>>
>> - unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
>> -
>> /* Shared page for notifying that explicit PIRQ EOI is required. */
>> unsigned long *pirq_eoi_map;
>> unsigned long pirq_eoi_map_mfn;
>> -};
>> +
>> + unsigned int psr_rmid; /* RMID assigned to the domain for CMT */
>
> I cannot see why this is being moved.
For no reason at all.. Fixed.
>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/monitor.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * include/asm-x86/monitor.h
>> + *
>> + * Architecture-specific monitor_op domctl handler.
>> + *
>> + * Copyright (c) 2015 Tamas K Lengyel (tamas@xxxxxxxxxxxxx)
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public
>> + * License v2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public
>> + * License along with this program; if not, write to the
>> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>> + * Boston, MA 021110-1307, USA.
>> + */
>> +
>> +#ifndef __ASM_X86_MONITOR_H__
>> +#define __ASM_X86_MONITOR_H__
>> +
>> +#include <xen/sched.h>
>> +#include <public/domctl.h>
>> +
>> +int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
>
> Didn't we settle on the two includes above being pointless? We
> should really aim at reducing the header dependency tree rather
> than endlessly growing it (and having basically everyone include
> everything).
That discussion I believe was about a couple other headers which got
removed. IMHO these two are needed for the definitions of the
structures used in the function definition.
>
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -607,24 +607,38 @@ long p2m_set_mem_access(struct domain *d, unsigned long
>> start_pfn, uint32_t nr,
>> int p2m_get_mem_access(struct domain *d, unsigned long pfn,
>> xenmem_access_t *access);
>>
>> -/* Check for emulation and mark vcpu for skipping one instruction
>> - * upon rescheduling if required. */
>> -void p2m_mem_access_emulate_check(struct vcpu *v,
>> - const vm_event_response_t *rsp);
>> +/*
>> + * Emulating a memory access requires custom handling
>> + */
>> +static inline
>> +int p2m_mem_access_enable_emulate(struct domain *d)
>> +{
>> + if ( d->arch.mem_access_emulate_enabled )
>> + return -EEXIST;
>>
>> -/* Enable arch specific introspection options (such as MSR interception). */
>> -void p2m_setup_introspection(struct domain *d);
>> + d->arch.mem_access_emulate_enabled = 1;
>> + return 0;
>> +}
>>
>> -/* Sanity check for vm_event hardware support */
>> -static inline bool_t p2m_vm_event_sanity_check(struct domain *d)
>> +static inline
>> +int p2m_mem_access_disable_emulate(struct domain *d)
>> {
>> - return hap_enabled(d) && cpu_has_vmx;
>> + if ( !d->arch.mem_access_emulate_enabled )
>> + return -EEXIST;
>> +
>> + d->arch.mem_access_emulate_enabled = 0;
>> + return 0;
>> }
>
> The fact that these seemingly non-atomic checks/updates are safe
> (due to being under domctl lock) should be stated in a comment.
Ack.
>
> Jan
Thanks, I'll be sending v8 of the series today. Are there any plans to
merge the early parts of the series that have been acked while these
patches at the end receive final touch-ups?
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |