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

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

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.

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.

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.

I was planning to do exactly the same for the vcpu count etc. as part of
my further cpuid work.  However, there is a substantial quantity of
development work required to make this function in a rational way.

For now,  I wouldn't worry too much.  It is just one of a very large
number of things which should be moved on migrate, but isn't.  (Most
notably, the cpuid policy.)

~Andrew