From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V3 3/3] xen/vm_event: Deny register writes if refused by vm_event reply Date: Mon, 6 Jul 2015 20:16:09 +0300 Message-ID: <559AB7D9.9030303@bitdefender.com> References: <1436197873-4559-1-git-send-email-rcojocaru@bitdefender.com> <1436197873-4559-4-git-send-email-rcojocaru@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Lengyel, Tamas" Cc: Jun Nakajima , Wei Liu , kevin.tian@intel.com, keir@xen.org, Ian Campbell , Stefano Stabellini , George Dunlap , Andrew Cooper , eddie.dong@intel.com, Xen-devel , Aravind.Gopalakrishnan@amd.com, Jan Beulich , suravee.suthikulpanit@amd.com, boris.ostrovsky@oracle.com, Ian Jackson List-Id: xen-devel@lists.xenproject.org On 07/06/2015 08:05 PM, Lengyel, Tamas wrote: > @@ -410,6 +414,8 @@ void vm_event_resume(struct domain *d, struct > vm_event_domain *ved) > > > #ifdef HAS_MEM_ACCESS > case VM_EVENT_REASON_MEM_ACCESS: > + case VM_EVENT_REASON_MOV_TO_MSR: > + case VM_EVENT_REASON_WRITE_CTRLREG: > > > This doesn't really make much sense to be associated with MEM_ACCESS. > I'm adding a separate arch-specific vm_event file in my other singlestep > patch, I think these should trigger their appropriate handler there, not > in mem_access_resume. To be honest I've somewhat anticipated this critique, and it's definitely valid. I just wasn't sure if somebody would have said that it's better to process all responses in one place for now, so I've decided to try it this way first. But yes, that's actually my preference as well, so no problem. > mem_access_resume(v, &rsp); > break; > #endifdiff --git a/xen/include/public/vm_event.h > b/xen/include/public/vm_event.h > index f0da008..bc97334 100644 > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -158,6 +158,11 @@ struct vm_event_regs_x86 { > * MEM_ACCESS_EMULATE_NOWRITE. > */ > #define MEM_ACCESS_SET_EMUL_READ_DATA (1 << 8) > + /* > + * Deny completion of the operation that triggered the event. > + * Currently only useful for MSR, CR0, CR3 and CR4 write events. > + */ > +#define MEM_ACCESS_DENY (1 << 9) > > > Same comment here, this feature is not really denying a mem_access, it > denies register writes. Associating it with mem_access just makes it > confusing. IMHO defining it as a VM_EVENT_FLAG_DENY_REGISTER_CHANGE or > something similar instead would make it a lot more descriptive and > inline with that it is actually doing. Fair enough, I'll think of a more appropriate name for it. Thanks, Razvan