From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V5 3/3] xen/vm_event: Deny register writes if refused by vm_event reply Date: Tue, 14 Jul 2015 16:45:15 +0300 Message-ID: <55A5126B.7070600@bitdefender.com> References: <1436807687-9826-1-git-send-email-rcojocaru@bitdefender.com> <1436807687-9826-4-git-send-email-rcojocaru@bitdefender.com> <55A51E1D0200007800090AF4@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A51E1D0200007800090AF4@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: jun.nakajima@intel.com, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, eddie.dong@intel.com, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, tlengyel@novetta.com, keir@xen.org, boris.ostrovsky@oracle.com List-Id: xen-devel@lists.xenproject.org On 07/14/2015 03:35 PM, Jan Beulich wrote: >>>> On 13.07.15 at 19:14, wrote: >> Changes since V4: >> - Rebased the patch to take into account Tamas' "x86/vm_event: >> toggle singlestep from vm_event response". >> - Moved the management of heap-allocated vm_event-related >> domain data to the specific x86 vm_event domain init / cleanup >> helpers. > > I would have understood this to mean vm_event_cleanup_domain() > instead of ... > >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -667,6 +667,8 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, >> >> void arch_domain_destroy(struct domain *d) >> { >> + xfree(d->arch.event_write_data); > > ... here. And I see it is being done there, so why also here? I was not convinced that it's safe to assume that vm_event_cleanup_domain() always gets called on domain destruction (please see also the reply to the 1/3 patch review). That's quite likely a wrong assumption, but if it's not it's safer. >> --- a/xen/arch/x86/vm_event.c >> +++ b/xen/arch/x86/vm_event.c >> @@ -22,11 +22,19 @@ >> >> #include >> #include >> +#include >> >> int vm_event_init_domain(struct domain *d) >> { >> struct vcpu *v; >> >> + if ( !d->arch.event_write_data ) >> + d->arch.event_write_data = xzalloc_array(struct monitor_write_data, >> + d->max_vcpus); > > Looking at this again I wonder why the data isn't being made part of > struct arch_vcpu's vm_event sub-structure. That would also address > the complaint I have here about this not being a guaranteed maximum > page size runtime allocation. I think this is just how the initial suggestion was worded, I'll change it. >> @@ -61,6 +72,38 @@ void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v) >> hvm_toggle_singlestep(v); >> } >> >> +void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp) >> +{ >> + if ( rsp->flags & VM_EVENT_FLAG_DENY ) >> + { >> + struct monitor_write_data *w = >> + &v->domain->arch.event_write_data[v->vcpu_id]; > > That would also eliminate this strange construct. Indeed, I'm not a fan of it either. >> --- a/xen/include/public/vm_event.h >> +++ b/xen/include/public/vm_event.h >> @@ -74,6 +74,11 @@ >> * VM_EVENT_FLAG_SET_EMUL_READ_DATA are set, only the latter will be honored). >> */ >> #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 5) >> + /* >> + * Deny completion of the operation that triggered the event. >> + * Currently only useful for MSR, CR0, CR3 and CR4 write events. >> + */ >> +#define VM_EVENT_FLAG_DENY (1 << 6) > > Wouldn't this want adding to the get-capabilities sub-op too? Yes, it's best to add it. Ack. Thanks, Razvan