[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 19/01/16 10:28, Ian Campbell wrote: > 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. It would appear that the choices are: 1) Rearchitect all domain building/restore from scratch 2) Implement a check & fail properly (Still a large quantity of work, but less than 1) 3) Hack up a check & fail quickly There are a very large number of areas which should be checked on migrate which currently are not. I already have plans to address 2) for the cpuid work. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |