From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT Date: Mon, 18 Jan 2016 18:03:15 +0000 Message-ID: <569D28E3.6000006@citrix.com> References: <5699358D02000078000C7800@prv-mh.provo.novell.com> <1452879951-76391-1-git-send-email-roger.pau@citrix.com> <569CA5CB02000078000C7C43@prv-mh.provo.novell.com> <569CB067.7070905@citrix.com> <569CC22402000078000C7D9C@prv-mh.provo.novell.com> <569CC143.9030703@citrix.com> <569D27AB.3090304@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aLE9E-0003Dc-On for xen-devel@lists.xenproject.org; Mon, 18 Jan 2016 18:03:24 +0000 In-Reply-To: <569D27AB.3090304@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Jan Beulich Cc: Ian Jackson , Wei Liu , Ian Campbell , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 18/01/16 17:58, Roger Pau Monn=E9 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, wrote: >>>> On 18/01/2016 07:43, Jan Beulich wrote: >>>>>>>> On 15.01.16 at 18:45, 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, unsigne= d int = >>>> domcr_flags, >>>>>> d->domain_id, config->emulation_flags); >>>>>> return -EINVAL; >>>>>> } >>>>>> - if ( config->emulation_flags !=3D 0 && >>>>>> - (!is_hvm_domain(d) || config->emulation_flags !=3D XEN= _X86_EMU_ALL) = >>>> ) >>>>>> + if ( is_hvm_domain(d) ? (config->emulation_flags !=3D XEN_X= 86_EMU_ALL && >>>>>> + config->emulation_flags !=3D 0) : >>>>>> + (config->emulation_flags !=3D 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 doma= in >>>> 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 struct= ure. >> >> 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_json > 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. ~Andrew > > This however raises the question about how to signal that the field is > not initialised, because 0 is a valid value (maybe ~0)? > > Roger. >