Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0

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

On 04/12/2013 04:07 AM, Jan Beulich wrote:
>>>> On 11.04.13 at 22:13, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
>>           }
>>
>>           set_cpuid_faulting(!is_hvm_vcpu(next) &&
>> -                           (next->domain->domain_id != 0));
>> +                           !is_hardware_domain(next->domain));
>
> This one is highly questionable. Following the comments on the
> previous patch, I'd think is_control_domain() here is more
> appropriate, but maybe it would really need to become the or
> of both? The question really is which domains we specifically
> don't want CPUID faulting for.

My impression was that this would be the hardware domain since it is
the one that would be parsing ACPI tables and similar things that need
CPUID to work. The control domain would just be involved in creating
and managing domains, which doesn't need anything out of CPUID that a
normal PV domain would need.

>> --- a/xen/arch/x86/hvm/i8254.c
>> +++ b/xen/arch/x86/hvm/i8254.c
>> @@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
>>           .data = data
>>       };
>>
>> -    if ( (current->domain->domain_id == 0) && dom0_pit_access(&ioreq) )
>> +    if ( is_hardware_domain(current->domain) && dom0_pit_access(&ioreq) )
>
> And this one is now becoming inconsistent in itself: You remove the
> explicit 0 on the left side, but leave the "dom0" in place on the right
> side. If we drop the association with domid == 0 being the special
> one, then all uses of "dom0" would logically also need to go away.

I didn't want to take on the mass function rename that this would imply. I
had considered naming the function "is_dom0_domain" but this seemed less
clear given that I was trying to isolate hardware accesses.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
>>                     uint32_t tsc_mode, uint64_t elapsed_nsec,
>>                     uint32_t gtsc_khz, uint32_t incarnation)
>>   {
>> -    if ( is_idle_domain(d) || (d->domain_id == 0) )
>> +    if ( is_idle_domain(d) || is_hardware_domain(d) )
>
> Just like with the CPUID faulting, it is unclear (without a proper
> model described end established first) which domains we really
> want to special case here. Hence, rather than papering over the
> issue, I'd prefer keeping the explicit domid check in place until
> such a model can get established.

OK; my logic here was the same as for CPUID faulting.

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
>>       c = regs->ecx;
>>       d = regs->edx;
>>
>> -    if ( current->domain->domain_id != 0 )
>> +    if ( !is_hardware_domain(current->domain) )
>
> Again, if anything this is more likely is_control_domain() or the or
> of both.
>
> Jan
>
>


-- 
Daniel De Graaf
National Security Agency