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

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

On Feb 22, 2016 04:23, "Razvan Cojocaru" <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>
> On 02/19/2016 07:26 PM, Lengyel, Tamas wrote:
> >
> >
> > On Fri, Feb 19, 2016 at 10:18 AM, Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx <mailto:andrew.cooper3@xxxxxxxxxx>> wrote:
> >
> >Â Â ÂOn 19/02/16 17:06, Lengyel, Tamas wrote:
> >>
> >>
> >>Â Â ÂOn Tue, Feb 16, 2016 at 3:47 AM, Jan Beulich <JBeulich@xxxxxxxx
> >>Â Â Â<mailto:JBeulich@xxxxxxxx>> wrote:
> >>
> >>Â Â Â Â Â>>> On 16.02.16 at 07:58, <<mailto:kevin.tian@xxxxxxxxx>kevin.tian@xxxxxxxxx
> >>Â Â Â Â Â<mailto:kevin.tian@xxxxxxxxx>> 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.

Tamas

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