[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 03/05/2014 04:23 AM, Jan Beulich wrote:
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/" ?

I had intended this sentence to describe the model, where the return is
always false. However, I agree with the change to avoid confusion that
the two is_*_domain() functions are exclusive.

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

Agreed, the hardware domain also needs to be checked for here. The
reason the control domain is present is that it needs to see real
CPUID values in order to set CPUID policy for guests based on the
real hardware values.

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

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

OK. I prefer to avoid the if (!foo) bar; else baz; construct where
possible, but common case first is a good reason to use it.

--- a/xen/common/xenoprof.c
+++ b/xen/common/xenoprof.c
@@ -603,7 +603,7 @@ static int xenoprof_op_init(XEN_GUEST_HANDLE_PARAM(void) 

      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.


I don't directly consider profiling to be a control domain property, but
I also don't consider it a hardware domain property, and it does need to
be restricted in some way. I could make a separate patch changing the
condition to use an XSM hook, only setting xenoprof_primary_profiler if
the domain is allowed the XEN__PRIVPROFILE permission, but this still
could cause multiple domains to be eligible.

From my cursory examination when I made this change, the first domain to
try becoming the primary profiler will succeed and be assigned to
xenoprof_primary_profiler. Later domains will not be considered since the
primary will already be set.

One thing I had not considered that may be a problem: if the profiling
domain is shut down without calling XENOPROF_shutdown, it will not be
possible to use profiling this boot unless the struct domain* is reused.
This may then become a security issue because an arbitrary domain is
now the primary profiler, although the XSM policy should prevent any
actions by a domain not permitted to use the profiling hypercalls.
Using is_hardware_domain here avoids that problem (as the hardware domain
may never shut down or be destroyed), so that may be the simplest
solution until a better model of who is responsible for profiling is

Daniel De Graaf
National Security Agency

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.