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

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Feb 12, 2016 03:41, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.

That was the idea.

>
> 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()).
>

I don't think we need an assert here. The function is fine for pv guests as well up to that point. Filling in the rest of the registers for pv guests can be done when pv events are implemented. Maybe a comment saying so is warranted.

Tamas

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