Re: [PATCH v3 4/4] x86/PV: enable the emulated PIT

[Andrew Cooper <andrew.cooper3@citrix.com>]

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@citrix.com> wrote:
>>>>>>>> On 18/01/2016 07:43, Jan Beulich wrote:
>>>>>>>>>>>> On 15.01.16 at 18:45, <roger.pau@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel