Re: [Xen-devel] [PATCH] vm_event: consolidate hvm_event_fill_regs and p2m_vm_event_fill_regs

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/02/2016 22:25, Lengyel, Tamas wrote:

On Thu, Feb 11, 2016 at 2:58 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel