Re: [PATCH 1/6] xen: use domid check in is_hardware_domain

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 03/05/2014 10:45 AM, Jan Beulich wrote:
>>>> On 05.03.14 at 16:25, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> On 03/05/2014 04:23 AM, Jan Beulich wrote:
>>>>>> On 04.03.14 at 23:51, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1505,7 +1505,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>>>            }
>>>>
>>>>            set_cpuid_faulting(is_pv_vcpu(next) &&
>>>> -                           (next->domain->domain_id != 0));
>>>> +                           !is_control_domain(next->domain));
>>>
>>> I can't see why the hardware domain (which can't be migrated)
>>> should be prevented from seeing the real CPUID values. It's
>>> less obvious whether a control domain should always see real
>>> values. Hence if you think the latter should be the case (as the
>>> change above shows), I think you should check for both cases
>>> here.
>>
>> Agreed, the hardware domain also needs to be checked for here. The
>> reason the control domain is present is that it needs to see real
>> CPUID values in order to set CPUID policy for guests based on the
>> real hardware values.
>
> So don't envision a staged system where the root domain hides
> some features from creation-capable ones, and those may hide
> further features from their descendants? Up to where even the
> controlling domains might become migratable?

Perhaps is_control_domain should be renamed to is_root_control_domain;
it is not necessary for every control domain to have is_control_domain
return true.  In fact, the domain builder I posted does not set the
is_privileged field on any guest it creates, and so once it shuts down,
there are no domains that the hypervisor considers control domains. The
XSM policy governs which domains are permitted to create, pause, and do
all the usual "control" operations.

A quick grep actually seems to point out that is_control_domain could
easily be removed, as the only references that remain are these CPUID
fields. This would end up simplifying the disaggregation model a bit.

>> Using is_hardware_domain here avoids that problem (as the hardware domain
>> may never shut down or be destroyed), so that may be the simplest
>> solution until a better model of who is responsible for profiling is
>> presented.
>
> Then please do so, with a short note to that effect in either the
> description or a code comment.
>
> Jan

Right, this change and a comment will be in the v2 when I post it.


-- 
Daniel De Graaf
National Security Agency