Re: [Xen-devel] [PATCH RFC 2/9] xen: Optimize introspection access to guest state

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.14 at 10:12, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 07/02/2014 06:37 PM, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -6016,6 +6016,38 @@ int hvm_debug_op(struct vcpu *v, int32_t op)
>>>      return rc;
>>>  }
>>>  
>>> +static inline void hvm_mem_event_fill_regs(mem_event_request_t *req)
>>> +{
>>> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
>>> +    struct vcpu *v = current;
>>> +
>>> +    req->regs.rax = regs->eax;
>>> +    req->regs.rcx = regs->ecx;
>>> +    req->regs.rdx = regs->edx;
>>> +    req->regs.rbx = regs->ebx;
>>> +    req->regs.rsp = regs->esp;
>>> +    req->regs.rbp = regs->ebp;
>>> +    req->regs.rsi = regs->esi;
>>> +    req->regs.rdi = regs->edi;
>>> +
>>> +    req->regs.r8  = regs->r8;
>>> +    req->regs.r9  = regs->r9;
>>> +    req->regs.r10 = regs->r10;
>>> +    req->regs.r11 = regs->r11;
>>> +    req->regs.r12 = regs->r12;
>>> +    req->regs.r13 = regs->r13;
>>> +    req->regs.r14 = regs->r14;
>>> +    req->regs.r15 = regs->r15;
>>> +
>>> +    req->regs.rflags = regs->eflags;
>>> +    req->regs.rip    = regs->eip;
>>> +
>>> +    req->regs.msr_efer = v->arch.hvm_vcpu.guest_efer;
>>> +    req->regs.cr0 = v->arch.hvm_vcpu.guest_cr[0];
>>> +    req->regs.cr3 = v->arch.hvm_vcpu.guest_cr[3];
>>> +    req->regs.cr4 = v->arch.hvm_vcpu.guest_cr[4];
>>> +}
>> 
>> This fills far not as many fields as the p2m function further down.
>> Why?
> 
> That is because hvm_mem_event_fill_regs() is used for events such as CR3
> changes or MSR access, and p2m_mem_event_fill_regs() is used for EPT
> events, and our application needs full information while handling EPT
> callbacks, and not as much for the other events.
> 
> Hence I've tried to avoid the unnecessary overhead in that case,
> thinking that if somebody needed those values, they would be added then.

Fair enough, if only it was visible (or stated) that this doesn't lead
to uninitialized data making it back to the event listener.

>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -425,6 +425,7 @@ static void vmx_vmcs_save(struct vcpu *v, struct 
>>> hvm_hw_cpu *c)
>>>      c->cr4 = v->arch.hvm_vcpu.guest_cr[4];
>>>  
>>>      c->msr_efer = v->arch.hvm_vcpu.guest_efer;
>>> +    c->guest_x86_mode = vmx_guest_x86_mode(v);
>> 
>> This seems unrelated and/or lacking an SVM counterpart.
> 
> Yes, it does lack a SVM counterpart. Is SVM support required for acceptance?

Not necessarily, but you should state this explicitly rather than leaving
it to be discovered by the readers. And presumably when enabling any
of this, you should check you're on VMX (at least I don't recall having
seen such a check).

> It is, however, not unrelated. Our application required that
> information, and it is cached in the mem_event (or am I missing something?).

The problem is that from the patch (including its description) it's not
clear where the consumer of this is.

Jan


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