From mboxrd@z Thu Jan  1 00:00:00 1970
From: Razvan Cojocaru <rcojocaru@bitdefender.com>
Subject: Re: [PATCH V2 3/3] xen/vm_event: Deny register writes
 if refused by vm_event reply
Date: Wed, 1 Jul 2015 18:21:14 +0300
Message-ID: <5594056A.5040805@bitdefender.com>
References: <1434359007-9302-1-git-send-email-rcojocaru@bitdefender.com>
	<1434359007-9302-4-git-send-email-rcojocaru@bitdefender.com>
	<558D2954020000780008A029@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <558D2954020000780008A029@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, jun.nakajima@intel.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, keir@xen.org, boris.ostrovsky@oracle.com
List-Id: xen-devel@lists.xenproject.org

On 06/26/2015 11:28 AM, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -2010,9 +2010,9 @@ static int vmx_cr_access(unsigned long exit_qualification)
>> >      }
>> >      case VMX_CONTROL_REG_ACCESS_TYPE_CLTS: {
>> >          unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
>> > +        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>> >          curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
>> >          vmx_update_guest_cr(curr, 0);
>> > -        hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
>> >          HVMTRACE_0D(CLTS);
>> >          break;
>> >      }
> I suppose it is intentional to ignore a possible deny here? If so,
> shouldn't the be documented by way of a comment?

On second though, I will document it by way of a comment and leave it as
it is, since this is a corner case that would need special handling
(vmx_update_guest_cr(curr, 0), etc. vs how it's now done in
hvm_do_resume(), which would need a new parameter being passed around,
conditional code, etc.), and AFAIK this event is not interesting from a
DENY perspective and so not currently worth the overhead.

Looking at the code again, I'll also fix another mistake: when cutting
and pasting the hvm_event_crX() above to make it a pre-write event to be
consistent, the hvm_event_crX(CR0, curr->arch.hvm_vcpu.guest_cr[0], old)
code became wrong: old == curr->arch.hvm_vcpu.guest_cr[0], whereas
before, curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS before that call.
I'll fix this too in V3.


Thanks,
Razvan