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

Re: [Xen-devel] [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0



On 04/12/2013 04:07 AM, Jan Beulich wrote:
On 11.04.13 at 22:13, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
          }

          set_cpuid_faulting(!is_hvm_vcpu(next) &&
-                           (next->domain->domain_id != 0));
+                           !is_hardware_domain(next->domain));

This one is highly questionable. Following the comments on the
previous patch, I'd think is_control_domain() here is more
appropriate, but maybe it would really need to become the or
of both? The question really is which domains we specifically
don't want CPUID faulting for.

My impression was that this would be the hardware domain since it is
the one that would be parsing ACPI tables and similar things that need
CPUID to work. The control domain would just be involved in creating
and managing domains, which doesn't need anything out of CPUID that a
normal PV domain would need.

--- a/xen/arch/x86/hvm/i8254.c
+++ b/xen/arch/x86/hvm/i8254.c
@@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write)
          .data = data
      };

-    if ( (current->domain->domain_id == 0) && dom0_pit_access(&ioreq) )
+    if ( is_hardware_domain(current->domain) && dom0_pit_access(&ioreq) )

And this one is now becoming inconsistent in itself: You remove the
explicit 0 on the left side, but leave the "dom0" in place on the right
side. If we drop the association with domid == 0 being the special
one, then all uses of "dom0" would logically also need to go away.

I didn't want to take on the mass function rename that this would imply. I
had considered naming the function "is_dom0_domain" but this seemed less
clear given that I was trying to isolate hardware accesses.

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d,
                    uint32_t tsc_mode, uint64_t elapsed_nsec,
                    uint32_t gtsc_khz, uint32_t incarnation)
  {
-    if ( is_idle_domain(d) || (d->domain_id == 0) )
+    if ( is_idle_domain(d) || is_hardware_domain(d) )

Just like with the CPUID faulting, it is unclear (without a proper
model described end established first) which domains we really
want to special case here. Hence, rather than papering over the
issue, I'd prefer keeping the explicit domid check in place until
such a model can get established.

OK; my logic here was the same as for CPUID faulting.

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs)
      c = regs->ecx;
      d = regs->edx;

-    if ( current->domain->domain_id != 0 )
+    if ( !is_hardware_domain(current->domain) )

Again, if anything this is more likely is_control_domain() or the or
of both.

Jan




--
Daniel De Graaf
National Security Agency

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


 


Rackspace

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