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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.01.16 at 11:41, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

I.e. fixable.

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

Which makes me repeat the question: Is this an inherent property
or just "that's the way it is right now"? And then of course the
question arises whether setting those flags at domain creation time
is the right model. I.e. ...

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

... what about deriving the emulation flags from the various
pieces of state getting loaded, at least when there are matching
pairs (which namely is the case for PIT)?

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

Ugly, but the way things seem to stand it may indeed be
unavoidable to ignore the issue for now.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel