Re: [Xen-devel] [PATCH v2] xen: use domid check in is_hardware_domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.07.13 at 15:00, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Wed, Jul 10, 2013 at 12:43 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 10.07.13 at 12:57, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
>>> On 09/07/13 21:28, Daniel De Graaf wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1459,7 +1459,7 @@ void context_switch(struct vcpu *prev, struct vcpu 
> *next)
>>>>           }
>>>>
>>>>           set_cpuid_faulting(!is_hvm_vcpu(next) &&
>>>> -                           (next->domain->domain_id != 0));
>>>> +                           !is_control_domain(next->domain));
>>>
>>> Won't this mean that in your separate hardware/control domain model that
>>> the hardware domain *will* fault on cpuid? Is this what we want?
>>
>> I think this is correct, as it's the control domain that ought to be
>> able to set up CPUID via the tool stack for all other domains.
>>
>> The implication is that it's the first booted domain. If that's not
>> generally the case, we'd need another qualification here. And
>> perhaps that qualification would in the end be domid == 0...
> 
> The question isn't why the control domain has it; the question is why
> the hardware domain doesn't have it.

Because - as said - for _all_ other domains, the control domain can
set a CPUID policy via the tool stack.

>>>> @@ -1962,7 +1962,7 @@ static void dump_softtsc(unsigned char key)
>>>>                  "warp=%lu (count=%lu)\n", tsc_max_warp, tsc_check_count);
>>>>       for_each_domain ( d )
>>>>       {
>>>> -        if ( d->domain_id == 0 && d->arch.tsc_mode == TSC_MODE_DEFAULT )
>>>> +        if ( is_hardware_domain(d) && d->arch.tsc_mode == 
>>>> TSC_MODE_DEFAULT )
>>>>               continue;
>>>>           printk("dom%u%s: mode=%d",d->domain_id,
>>>>                   is_hvm_domain(d) ? "(hvm)" : "", d->arch.tsc_mode);
>>>
>>> Why have direct access to the tsc for the hardware domain and not the
>>> control domain?
>>
>> Because, as its name says, it has direct access to the hardware
>> (including the TSC)?
> 
> Again, my question wasn't so much about why the hardware domain does
> have it, but why the control domain does *not* have it.

And I think I answered it. Or should I return the question and ask:
"Why do you think the control domain should be using the TSC
directly? I.e. in what way is it different from other non-hardware
domains in its interaction with hardware?"

>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -238,7 +238,7 @@ struct domain *domain_create(
>>>>       if ( domcr_flags & DOMCRF_hvm )
>>>>           d->is_hvm = 1;
>>>>
>>>> -    if ( domid == 0 )
>>>> +    if ( is_hardware_domain(d) )
>>>>       {
>>>>           d->is_pinned = opt_dom0_vcpus_pin;
>>>>           d->disable_migrate = 1;
>>>
>>> At some point this will have to be thought about a bit more -- which of
>>> the disaggregated domains do we actually want pinned here?  But I think
>>> this is fine for now.
>>
>> The pinning really is mainly for the Dom0-controlled CPU frequency
>> scaling to work (and hence validly qualified by is_hardware_domain()).
>> Any other uses, no matter how frequently they are seen, are abusing
>> dom0_vcpus_pin afaict.
> 
> If that feature was meant to be used exclusively for cpu frequency,
> then it should have been named something related to cpu frequency.

I agree.

> Using something originally designed for one purpose for another
> purpose isn't an "abuse", it's a "cleaver hack". :-)

Okay, you can call it that way if you like.

Jan


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