From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs Date: Thu, 11 Feb 2016 22:30:22 +0000 Message-ID: <56BD0B7E.9030507@citrix.com> References: <1455224749-2120-1-git-send-email-tlengyel@novetta.com> <56BCFEAE.8070608@citrix.com> <56BD01EC.5090202@bitdefender.com> <56BD0420.90809@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2267370142311112328==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aTzkl-0002Nk-0w for xen-devel@lists.xenproject.org; Thu, 11 Feb 2016 22:30:23 +0000 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: George Dunlap , xen-devel@lists.xenproject.org, Keir Fraser , Jan Beulich , Razvan Cojocaru List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============2267370142311112328== Content-Type: multipart/alternative; boundary="------------060104090908090208040401" This is a multi-part message in MIME format. --------------060104090908090208040401 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 11/02/2016 22:25, Lengyel, Tamas wrote: > > > On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper > > wrote: > > On 11/02/2016 21:49, Razvan Cojocaru wrote: > > On 02/11/2016 11:35 PM, Andrew Cooper wrote: > >> On 11/02/2016 21:05, Tamas K Lengyel wrote: > >> > >>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c > >>> index 08d678a..fa5d154 100644 > >>> --- a/xen/arch/x86/vm_event.c > >>> +++ b/xen/arch/x86/vm_event.c > >>> @@ -122,6 +122,64 @@ void vm_event_set_registers(struct vcpu > *v, vm_event_response_t *rsp) > >>> v->arch.user_regs.eip = rsp->data.regs.x86.rip; > >>> } > >>> > >>> +void vm_event_fill_regs(vm_event_request_t *req) > >>> +{ > >>> + const struct cpu_user_regs *regs = guest_cpu_user_regs(); > >>> + struct segment_register seg; > >>> + struct hvm_hw_cpu ctxt; > >>> + struct vcpu *curr = current; > >>> + > >>> + req->data.regs.x86.rax = regs->eax; > >>> + req->data.regs.x86.rcx = regs->ecx; > >>> + req->data.regs.x86.rdx = regs->edx; > >>> + req->data.regs.x86.rbx = regs->ebx; > >>> + req->data.regs.x86.rsp = regs->esp; > >>> + req->data.regs.x86.rbp = regs->ebp; > >>> + req->data.regs.x86.rsi = regs->esi; > >>> + req->data.regs.x86.rdi = regs->edi; > >>> + > >>> + req->data.regs.x86.r8 = regs->r8; > >>> + req->data.regs.x86.r9 = regs->r9; > >>> + req->data.regs.x86.r10 = regs->r10; > >>> + req->data.regs.x86.r11 = regs->r11; > >>> + req->data.regs.x86.r12 = regs->r12; > >>> + req->data.regs.x86.r13 = regs->r13; > >>> + req->data.regs.x86.r14 = regs->r14; > >>> + req->data.regs.x86.r15 = regs->r15; > >>> + > >>> + req->data.regs.x86.rflags = regs->eflags; > >>> + req->data.regs.x86.rip = regs->eip; > >>> + req->data.regs.x86.dr7 = curr->arch.debugreg[7]; > >> I think there is a %dr7 handling issue here. For an HVM > guests, this > >> field is only valid when you are not in the context of the > guest, as it > >> lives in the vmcs/vmcs. (PV guests keep it synchronously up to > date) > > Would this make it OK to use in p2m_vm_event_fill_regs() but not in > > hvm_event_fill_regs(), as it currently is? Maybe this is the > issue I'm > > remembering. > > Its use in p2m_mem_access_check() looks similarly buggy. That is also > in the context of 'current'. > > I would have thought that the use of hardware debugging facilities > would > be rare in the general case, which probably means that by chance, the > value is right most of the time (as it gets synchronised when a > vcpu is > scheduled on a new pcpu). > > > This is an issue that should be addressed in a separate patch. Agreed. > It does look like dr7 will need a separate hvm function we can call to > do a __vmread for us on GUEST_DR7. It would be better to modify the existing function to do the right thing, rather than to introduce a brand new one. In some copious free time, I already want to cull some of the redundant hvm_funcs. ~Andrew --------------060104090908090208040401 Content-Type: text/html; charset=utf-8 Content-Length: 7110 Content-Transfer-Encoding: quoted-printable
On 11/02/2016 22:25, Lengyel, Tamas wrote:


On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
On 11/02/2016 21:49, Razvan Cojocaru wrote:
> On 02/11/2016 11:35 PM, Andrew Cooper wrote:
>> On 11/02/2016 21:05, Tamas K Lengyel wrote:
>>
>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> index 08d678a..fa5d154 100644
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -122,6 +122,64 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
>>>=C2=A0 =C2=A0 =C2=A0 v->arch.user_regs.eip =3D rsp->data.regs.x86.rip;
>>>=C2=A0 }
>>>
>>> +void vm_event_fill_regs(vm_event_request_t *req)
>>> +{
>>> +=C2=A0 =C2=A0 const struct cpu_user_regs *regs =3D guest_cpu_user_regs();
>>> +=C2=A0 =C2=A0 struct segment_register seg;
>>> +=C2=A0 =C2=A0 struct hvm_hw_cpu ctxt;
>>> +=C2=A0 =C2=A0 struct vcpu *curr =3D current;
>>> +
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rax =3D regs->eax;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rcx =3D regs->ecx;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rdx =3D regs->edx;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rbx =3D regs->ebx;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rsp =3D regs->esp;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rbp =3D regs->ebp;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rsi =3D regs->esi;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rdi =3D regs->edi;
>>> +
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r8=C2=A0 =3D regs->r8;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r9=C2=A0 =3D regs->r9;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r10 =3D regs->r10;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r11 =3D regs->r11;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r12 =3D regs->r12;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r13 =3D regs->r13;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r14 =3D regs->r14;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.r15 =3D regs->r15;
>>> +
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rflags =3D regs->eflags;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.rip=C2=A0 =C2=A0 =3D regs->eip;
>>> +=C2=A0 =C2=A0 req->data.regs.x86.dr7=C2=A0 =C2=A0 =3D curr->arch.debugreg[7];
>> I think there is a %dr7 handling issue here.=C2=A0 For an HVM guests, this
>> field is only valid when you are not in the context of the guest, as it
>> lives in the vmcs/vmcs.=C2=A0 (PV guests keep it synchronously up to date)
> Would this make it OK to use in p2m_vm_event_fill_regs() but not in
> hvm_event_fill_regs(), as it currently is=3F Maybe this is the issue I'm
> remembering.

Its use in p2m_mem_access_check() looks similarly buggy.=C2=A0 That is also
in the context of 'current'.

I would have thought that the use of hardware debugging facilities would
be rare in the general case, which probably means that by chance, the
value is right most of the time (as it gets synchronised when a vcpu is
scheduled on a new pcpu).

This is an issue that should be addressed in a separate patch.

Agreed.

It does look like dr7 will need a separate hvm function we can call to do a __vmread for us on GUEST_DR7.

It would be better to modify the existing function to do the right thing, rather than to introduce a brand new one.=C2=A0 In some copious free time, I already want to cull some of the redundant hvm_funcs.

~Andrew
--------------060104090908090208040401-- --===============2267370142311112328== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============2267370142311112328==--