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

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

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_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.
>