Re: [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Isaila Alexandru <aisaila@bitdefender.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	xen-devel@lists.xen.org, Paul Durrant <paul.durrant@citrix.com>,
	Jan Beulich <JBeulich@suse.com>
Subject: Re: [PATCH v17 13/13] x86/domctl: Don't pause the whole domain if only getting vcpu state
Date: Mon, 3 Sep 2018 16:56:05 +0200	[thread overview]
Message-ID: <20180903145605.6igxigbiyozsizqg@mac.bytemobile.com> (raw)
In-Reply-To: <1535985763.3202.2.camel@bitdefender.com>

On Mon, Sep 03, 2018 at 05:42:43PM +0300, Isaila Alexandru wrote:
> On Lu, 2018-09-03 at 16:36 +0200, Roger Pau Monné wrote:
> > On Fri, Aug 31, 2018 at 04:56:21PM +0300, Isaila Alexandru wrote:
> > > 
> > > On Mi, 2018-08-29 at 08:13 -0600, Jan Beulich wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 29.08.18 at 16:02, <aisaila@bitdefender.com> wrote:
> > > > > On Mi, 2018-08-22 at 18:15 +0300, Isaila Alexandru wrote:
> > > > > > 
> > > > > > 
> > > > > > On Mi, 2018-08-22 at 16:41 +0200, Roger Pau Monné wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > If you look at vcpu_hvm in tools/libxc/xc_dom_x86.c it
> > > > > > > saves
> > > > > > > the
> > > > > > > full
> > > > > > > domain context just to get the CPU and the MTRR state of
> > > > > > > VCPU#0. Do
> > > > > > > you think you could switch this code to use the newly
> > > > > > > introduced
> > > > > > > machinery to save a single instance of a specific type?
> > > > > > Sure, I will add a tool patch at the end of the series
> > > > > Is this urgent to be in this series? If not I will add a new
> > > > > patch
> > > > > after it is all in. 
> > > > Considering the problems that there have been with this series,
> > > > anything to help build confidence in things still working for all
> > > > cases would help here, so I'm pretty glad Roger thought of this,
> > > > and while I wouldn't make it as strong as "the series can't go
> > > > in without this", I'd still much prefer if you too the time.
> > > I don't think it is possible to use getcontext_partial()
> > > in vcpu_hvm()
> > > because of the need to have a header for xc_domain_hvm_setcontext()
> > > and
> > > the only way to get it is by xc_domain_hvm_getcontext(). There is
> > > also
> > > a comment there that states the same thing
> > > "/*
> > >      * Get the full HVM context in order to have the header, it is
> > > not
> > >      * possible to get the header with getcontext_partial, and
> > > crafting
> > > one
> > >      * from userspace is also not an option since cpuid is trapped
> > > and
> > >      * modified by Xen.
> > >      */
> > > "
> > > I hope I understood the request correctly to start with and if not
> > > please clarify. 
> > But I expect you also get such header when fetching the state of a
> > single device, or else how do you use this new hypercall in
> > conjunction with xc_domain_hvm_setcontext?
> > 
> The new *save_one functions are based on the
> old xc_domain_hvm_getcontext_partial() that did not send the header. I
> had no requests to change this behavior by this point.

Hm, I have to admit I think this interface is broken, but I'm not
going to insist on this anyway.

IMO it would be good for you to fix this so that this new functions
can be used in vcpu_hvm, then you will have a user in-tree, so the
chances of someone accidentally breaking the interface would be very
low. Right now there's no user of the interface in-tree, so breakages
in this interface would go unnoticed by the test system.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel