Re: [PATCH v3 1/2] hvm/vmx: save dr7 during vmx_vmcs_save

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 02/19/2016 07:26 PM, Lengyel, Tamas wrote:
> 
> 
> On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> wrote:
> 
>     On 19/02/16 17:06, Lengyel, Tamas wrote:
>>
>>
>>     On Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@suse.com
>>     <mailto:JBeulich@suse.com>> wrote:
>>
>>         >>> On 16.02.16 at 07:58, <<mailto:kevin.tian@intel.com>kevin.tian@intel.com
>>         <mailto:kevin.tian@intel.com>> wrote:
>>         >> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>         >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>         >> @@ -490,6 +490,7 @@ static void vmx_vmcs_save(struct vcpu
>>         *v, struct hvm_hw_cpu
>>         >> *c)
>>         >>      __vmread(GUEST_SYSENTER_CS, &c->sysenter_cs);
>>         >>      __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
>>         >>      __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
>>         >> +    __vmread(GUEST_DR7, &c->dr7);
>>         >>
>>         >
>>         > Hi, Tamas, I didn't see the open closed around "v !=
>>         current", if
>>         > I'm not missing some mails... Have you confirmed with Jan that
>>         > he is OK with it?
>>
>>         We didn't really settle on this yet. I'm not heavily opposed to it
>>         remaining unconditional for now, but as said in the other replay
>>         my fear is that this might later lead to further additions which
>>         may then also be of no interest to the main (save/migration)
>>         user of this code.
>>
>>
>>     Andrew, any comment if this is OK from your perspective?
> 
>     I specifically suggested the use of vmx_save_dr() to make all debug
>     state consistent.
> 
> 
> I might have missed that comment.
>  
> 
> 
>     I don't see much purpose in being able to introspect just %dr7.  If
>     any debug related activities are going on, all debug registers are
>     relevant.
> 
>     Is this not the case?
> 
> 
> Right now only dr7 is included in the automatic register snapshot sent
> with each vm_event. I personally don't use any of them so I can't
> comment on how it would be useful by itself (Razvan?). From my
> perspective the only issue at hand has been that the current way dr7 was
> gathered was incorrect. IMHO if someone needs the other debug registers
> for each vm_event, that change can be introduced in a separate patch.

Andrew is right, all debug registers are relevant for debug activities.
In fact, I've checked with the introspection engine team, and they no
longer use DR7 at the moment (I don't recall exactly why it has been
requested when I first wrote the patch a few years ago).

So if nobody minds - and I find it unlikely that anyone would - we can,
for the moment, simply remove DR7 altogether from the registers sent
with the vm_event. Should they become necessary, we should indeed
include all of them in a future patch.


Thanks,
Razvan