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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

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

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

 -George

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