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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.14 at 23:51, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> The 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.  It is also the domain managing devices that
> do not support pass-through: PCI configuration space access, parsing the
> hardware ACPI tables and system power or machine check events.  This is
> the only domain where is_hardware_domain() is true.  The return of
> is_control_domain() is false for this domain.

"s/is/may be/" ?

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

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -738,7 +738,7 @@ void pv_cpuid(struct cpu_user_regs *regs)
>      c = regs->ecx;
>      d = regs->edx;
>  
> -    if ( current->domain->domain_id != 0 )
> +    if ( !is_control_domain(current->domain) )

The same consideration applies here then, obviously.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -242,7 +242,7 @@ struct domain *domain_create(
>      else if ( domcr_flags & DOMCRF_pvh )
>          d->guest_type = guest_type_pvh;
>  
> -    if ( domid == 0 )
> +    if ( is_hardware_domain(d) )
>      {
>          d->is_pinned = opt_dom0_vcpus_pin;
>          d->disable_migrate = 1;
> @@ -267,10 +267,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;

I'd prefer the if/else cases to remain as they are - makes the patch
smaller, and fits better with the (weak) model of using the if branch
for the common case and the else one for the special one (outside
of error handling of course).

> --- a/xen/common/xenoprof.c
> +++ b/xen/common/xenoprof.c
> @@ -603,7 +603,7 @@ static int xenoprof_op_init(XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>  
>      xenoprof_init.is_primary = 
>          ((xenoprof_primary_profiler == d) ||
> -         ((xenoprof_primary_profiler == NULL) && (d->domain_id == 0)));
> +         ((xenoprof_primary_profiler == NULL) && is_control_domain(d)));

Do you really consider profiling a control domain property? This is
even more so questionable without knowing whether you checked
that there are no issues with all of the sudden there perhaps
being more than one domain eligible for becoming the primary
profiler - the oprofile code isn't in that good a shape to be certain
without explicit checking.

Jan


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