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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.02.16 at 11:19, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 02/12/2016 11:57 AM, Jan Beulich wrote:
>>>>> On 12.02.16 at 01:22, <tlengyel@xxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -122,6 +122,65 @@ 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;
>>> +
>>> +    if ( !is_hvm_domain(curr->domain) )
>>> +        return;
>> 
>> No such check existed in either of the two original functions. Why is
>> it needed all of the sudden? And if it is needed, why do the other
>> fields not get filled (as far as possible at least) for PV guests?
> 
> I can't speak for Tamas, but I suspect the check has been placed there
> because calls to hvm_funcs.save_cpu_ctxt(curr, &ctxt) and
> hvm_get_segment_register(curr, x86_seg_fs, &seg) follow, and he's put
> vm_event_fill_regs() in xen/arch/x86/vm_event.c (a previous function was
> called hvm_event_fill_regs(), in arch/x86/hvm/event.c, so no checking
> for HVM was needed).
> 
> I don't think the check is needed for the current codepaths, but since
> the code has been moved to xen/arch/x86/ the question about future PV
> events is fair.

In which case ASSERT(is_hvm_vcpu(curr)) would be the common
way to document this (at once avoiding the open coding of
is_hvm_vcpu()).

Jan


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