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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 02/22/2016 03:51 PM, Lengyel, Tamas wrote:
> 
> On Feb 22, 2016 04:23, "Razvan Cojocaru" <rcojocaru@bitdefender.com
> <mailto:rcojocaru@bitdefender.com>> wrote:
>>
>> 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>
> <mailto: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>
>> >>     <mailto:JBeulich@suse.com <mailto:JBeulich@suse.com>>> wrote:
>> >>
>> >>         >>> On 16.02.16 at 07:58, <<mailto:kevin.tian@intel.com
> <mailto:kevin.tian@intel.com>>kevin.tian@intel.com
> <mailto:kevin.tian@intel.com>
>> >>         <mailto: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.
>>
> 
> I would rather not remove it if it's not necessary as it's a change in
> the vm_event interface. If we do, we would have to bump the version, the
> user needs to be aware of the change, etc. So while the whole vm_event
> rework was done partly to allow us to do such changes, I would rather
> just keep it for now.

Fine with me.


Thanks,
Razvan