Re: [Xen-devel] [PATCH 1/4] xen: use domid check in is_hardware_domain

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Jul 8, 2013 at 4:58 PM, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> On 07/08/2013 10:58 AM, Jan Beulich wrote:
>>>>>
>>>>> On 08.07.13 at 16:46, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
>>>
>>> --- 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;
>>> @@ -263,10 +263,10 @@ struct domain *domain_create(
>>>           d->is_paused_by_controller = 1;
>>>           atomic_inc(&d->pause_count);
>>>
>>> -        if ( domid )
>>> -            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
>>> -        else
>>> +        if ( is_hardware_domain(d) )
>>>               d->nr_pirqs = nr_static_irqs + extra_dom0_irqs;
>>> +        else
>>> +            d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
>>>           if ( d->nr_pirqs > nr_irqs )
>>>               d->nr_pirqs = nr_irqs;
>>>
>>> @@ -600,7 +600,7 @@ void domain_shutdown(struct domain *d, u8 reason)
>>>           d->shutdown_code = reason;
>>>       reason = d->shutdown_code;
>>>
>>> -    if ( d->domain_id == 0 )
>>> +    if ( is_hardware_domain(d) )
>>>           dom0_shutdown(reason);
>>
>>
>> All earlier changes can be explained in one way or another to also
>> apply to other than Dom0. This one, however, can't: There can
>> only ever be one domain controling when to shut down the system,
>> and hence I think it is misleading to use is_hardware_domain()
>> here. Or is your targeted abstract model aiming at a single such
>> domain, just perhaps with a domain ID other than zero?
>>
>> Jan
>
>
> Yes, that is the model I am using. It splits dom0 into three domains:
>
> 0. Domain builder: bootstraps the system. May remain to perform requested
>    builds of domains that need a minimal trust chain (i.e. vTPM domains).
>    Other than being built by the hypervisor, nothing is special about this
>    domain - although it may be useful to have is_control_domain be true.
> 1. Hardware domain: manages devices for PCI pass-through to driver domains
>    or can act as a driver domain itself, depending on the desired degree
>    of disaggregation. This is the only domain where is_hardware_domain()
>    is true. The return of is_control_domain() is false for this domain.
> 2. Control domain: manages other domains, controls guest launch/shutdown,
>    manages resource constraints, etc; is_control_domain() returns true.
>
> This model has a working implementation derived from the work in the XOAR
> paper. It requires a patch on top of these to change the IOMMU setup to
> happen after dom0 starts, and a dedicated domain builder domain.

Is the reason you have this broken down into four patches in order to
be able to CC different sets of people?

I think that will make it really hard for archaeologists to go back
and figure out what on earth these changes were for.  Since all the
patches basically do the same thing, I think it would be better to
have one big patch.

In any case, we need a description in either a comment or at least in
a changeset what your target model is (which you describe above).

 -George

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