[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 4/4] x86/PV: enable the emulated PIT
On Tue, 2016-01-19 at 10:09 +0000, Andrew Cooper wrote: > On 19/01/16 09:24, Ian Campbell wrote: > > On Mon, 2016-01-18 at 18:03 +0000, Andrew Cooper wrote: > > > On 18/01/16 17:58, Roger Pau Monnà wrote: > > > > El 18/01/16 a les 11.41, Andrew Cooper ha escrit: > > > > > On 18/01/16 09:44, Jan Beulich wrote: > > > > > > > > > On 18.01.16 at 10:29, <andrew.cooper3@xxxxxxxxxx> wrote: > > > > > > > On 18/01/2016 07:43, Jan Beulich wrote: > > > > > > > > > > > On 15.01.16 at 18:45, <roger.pau@xxxxxxxxxx> wrote: > > > > > > > > > Changes since v2: > > > > > > > > > Â- Change 'if ( (a && b) || (!a && c) )' into 'if ( a ? b > > > > > > > > > : c > > > > > > > > > )'. > > > > > > > > Thanks, but after some more thinking about it I'm afraid > > > > > > > > there > > > > > > > > are > > > > > > > > a few more aspects to consider here: > > > > > > > > > > > > > > > > > --- a/xen/arch/x86/domain.c > > > > > > > > > +++ b/xen/arch/x86/domain.c > > > > > > > > > @@ -542,8 +542,9 @@ int arch_domain_create(struct domain > > > > > > > > > *d, > > > > > > > > > unsigned int > > > > > > > domcr_flags, > > > > > > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂd->domain_id, config- > > > > > > > > > >emulation_flags); > > > > > > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EINVAL; > > > > > > > > > ÂÂÂÂÂÂÂÂÂ} > > > > > > > > > -ÂÂÂÂÂÂÂÂif ( config->emulation_flags != 0 && > > > > > > > > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂ(!is_hvm_domain(d) || config- > > > > > > > > > >emulation_flags > > > > > > > > > != XEN_X86_EMU_ALL) > > > > > > > ) > > > > > > > > > +ÂÂÂÂÂÂÂÂif ( is_hvm_domain(d) ? (config->emulation_flags > > > > > > > > > != > > > > > > > > > XEN_X86_EMU_ALL && > > > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂconfig->emulation_flags != 0) : > > > > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ(config->emulation_flags != > > > > > > > > > XEN_X86_EMU_PIT) ) > > > > > > > > > ÂÂÂÂÂÂÂÂÂ{ > > > > > > > > For one I think it would be a good idea to allow zero for > > > > > > > > PV > > > > > > > > domains, > > > > > > > > and perhaps even default new DomU-s to have the PIT flag > > > > > > > > clear. > > > > > > > > (Also - indentation.) > > > > > > > > > > > > > > > > Which gets us to the second, broader issue: These flags > > > > > > > > shouldn't > > > > > > > > be forced to a particular value during migration, but > > > > > > > > instead > > > > > > > > they > > > > > > > > should be part of the state getting migrated. Incoming > > > > > > > > domains > > > > > > > > then would - if the field is missing due to coming from an > > > > > > > > older > > > > > > > > hypervisor - have the flag default to 1. > > > > > > > There is sadly another ratsnest here. > > > > > > I've been afraid of that. > > > > > > > > > > > > > These values are needed for domain creation, which means that > > > > > > > putting > > > > > > > them anywhere in the migration stream is already too late, as > > > > > > > the > > > > > > > domain > > > > > > > has been created before the stream header is read. > > > > > > Is that an inherent requirement, or just a result of current > > > > > > code > > > > > > structure? > > > > > Depends.ÂÂAs far as libxc/libxl migration levels go, current code > > > > > structure. > > > > > > > > > > Whatever (eventually) gets used to set these values will however > > > > > be > > > > > present in the xl configuration, which is at the very start of > > > > > the > > > > > stream, and is what is used to create the new domain. > > > > > > > > > > We really don't want the libxc migrate code to be making the > > > > > DOMCTL_createdomain hypercall itself; it opens up a whole new > > > > > attack > > > > > surface via cunningly-crafted save image.ÂÂThe best we can do is > > > > > have > > > > > a > > > > > sanity check later on. > > > > > > > > > > > ÂI ask because migrating the emulation flags is going to > > > > > > be a requirement for relaxing the current (almost) all-or- > > > > > > nothing > > > > > > policy on those flags. > > > > > > > > > > > > > In principle, the best which could occur is that a value gets > > > > > > > stashed in > > > > > > > the stream and used as a sanity check.ÂÂThat will at least > > > > > > > catch > > > > > > > the > > > > > > > case when they are different. > > > > > > That'd be a minimal first step. > > > > > This is a substantial quantity of work to do properly.ÂÂAs the > > > > > emulation > > > > > flags are just one in a very long list of fields handed like > > > > > this, I > > > > > don't think this issue should block the series. > > > > You certainly are more familiar with the migration code than me, > > > > but > > > > wouldn't it be enough to add a new field to libxl_domain_build_info > > > > (uint32_t emulation_flags), and teach > > > > libxl_domain_build_info_gen_json/libxl__domain_build_info_parse_jso > > > > n > > > > Âhow to properly parse it? > > > That would let it be configured from an xl.cfg file, and would > > > normally > > > be moved in the migration stream.ÂÂHowever, there is a specific > > > option > > > in xl to restore but using a brand new configuration file. > > > > > > What it doesn't do it check that the settings for the domain in the > > > stream match the settings of the domid being restored into. > > That would be the responsibility of the user who has chosen to override > > the > > configuration in this way. > > It is the responsibility of Xen to ensure there are no exploitable holes > due to partial or misconfiguration. Indeed, but it only needs to check things and fail, not work in the face of a bogus save file + cfg file configuration. Perhaps I misunderstood what was being contended here. Ian. > In particular, this PIT emulation patch fixes an accidental NULL pointer > dereference in Xen, due to the accidental disabling of the PIT in PV > guests. > > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |