[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 2/2] xen: implement VCPUOP_register_runstate_phys_memory_area
On 13/05/2019 13:30, Andrii Anisov wrote: On 08.05.19 18:40, Julien Grall wrote:diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 6dc633e..8e24e63 100644{ - void __user *guest_handle = NULL; + if ( !guest_handle_is_null(runstate_guest(v)) ) + { + void __user *guest_handle = NULL; + if ( VM_ASSIST(v->domain, runstate_update_flag) ) + { + guest_handle = &v->runstate_guest.p->state_entry_time + 1; + guest_handle--; + v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE; + __raw_copy_to_guest(guest_handle,+ (void *)(&v->runstate.state_entry_time + 1) - 1,+ 1); + smp_wmb(); + } - if ( guest_handle_is_null(runstate_guest(v)) ) - return; + __copy_to_guest(runstate_guest(v), &v->runstate, 1); - if ( VM_ASSIST(v->domain, runstate_update_flag) ) - { - guest_handle = &v->runstate_guest.p->state_entry_time + 1; - guest_handle--; - v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE; - __raw_copy_to_guest(guest_handle,- (void *)(&v->runstate.state_entry_time + 1) - 1, 1);- smp_wmb(); + if ( guest_handle ) + { + v->runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE; + smp_wmb(); + __raw_copy_to_guest(guest_handle,+ (void *)(&v->runstate.state_entry_time + 1) - 1,+ 1); + } } - __copy_to_guest(runstate_guest(v), &v->runstate, 1); - - if ( guest_handle ) + spin_lock(&v->mapped_runstate_lock); + if ( v->mapped_runstate )The code looks a bit odd to me, you seem to allow a guest to provide 2 runstate areas: one using guest virtual address the other using guest physical address.It would be best if we prevent a guest to mix match them.Firstly I turned to implementing in that way, but the locking and decissions code become really ugly and complex while trying to cover 'guest's misbehavior' scenarios. I think it is possible to have a simple version taking the decision on which method to use. You can either use the spin_lock to protect everything or use something like: update_runstate_area(): if ( xchg(&v->runstate_in_use, 1) ) return; switch ( v->runstate_type ) { GVADDR: update_runstate_by_gvaddr(); GPADDR: update_runstate_by_gpaddr(); } xchg(&v->runstate_in_use, 0); registering an area while ( xchg(&v->runstate_in_use, 1) ); /* Clean-up and registering the area */ IOW, if the guest provide a physical address first, then *all* the call should be physical address. Alternatively this could be a per vCPU decision.I guess we should agree what to implement first. I think there are an agreement that the two methods should not be used together. [..] > The combination of mapped_runstate and runstate_guest is a bit confusing. I think you want to rework the interface to show that only one is possible at the time and make clear which one is used by who. Maybe:diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 312fec8..3fb6ea2 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -217,6 +217,8 @@ void vcpu_show_execution_state(struct vcpu *); void vcpu_show_registers(const struct vcpu *); void vcpu_switch_to_aarch64_mode(struct vcpu *); +void update_runstate_area(struct vcpu *); + /* * Due to the restriction of GICv3, the number of vCPUs in AFF0 is * limited to 16, thus only the first 4 bits of AFF0 are legal. We will diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index d1bfc82..ecddcfe 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -118,4 +118,6 @@ struct vnuma_info { void vnuma_destroy(struct vnuma_info *vnuma); +struct vcpu_register_runstate_memory_area; + #endif /* __XEN_DOMAIN_H__ */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 748bb0f..2afe31c 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -163,15 +163,23 @@ struct vcpu void *sched_priv; /* scheduler-specific data */ struct vcpu_runstate_info runstate; + + spinlock_t mapped_runstate_lock; + #ifndef CONFIG_COMPAT # define runstate_guest(v) ((v)->runstate_guest) XEN_GUEST_HANDLE(vcpu_runstate_info_t) runstate_guest; /* guest address */ + vcpu_runstate_info_t *mapped_runstate; #else # define runstate_guest(v) ((v)->runstate_guest.native) union { XEN_GUEST_HANDLE(vcpu_runstate_info_t) native; XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t) compat; } runstate_guest; /* guest address */ + union { + vcpu_runstate_info_t* native; + vcpu_runstate_info_compat_t* compat; + } mapped_runstate; /* guest address */As I said before, IMO coupling those interfaces makes the code complicated and ugly. Well, I can't see how it can be ugly (see my example above). Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |