[PATCH] xen : Replace hard coded domain_id checks with a macro

[PATCH] xen : Replace hard coded domain_id checks with a macro

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 178 bytes --]

Replace a number of checks for Dom0, that have been hard coded to check for
domain_id being zero with a macro is_dom0_domain().

Signed-off by: Mihir Nanavati <mihirn@cs.ubc.ca>

[-- Attachment #1.2: Type: text/html, Size: 247 bytes --]

[-- Attachment #2: dom0_domain_check.patch --]
[-- Type: text/x-patch, Size: 7414 bytes --]

diff -r 924ce68580ff xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/arch/x86/domain_build.c	Thu Dec 09 20:56:43 2010 -0800
@@ -345,7 +345,7 @@
     paddr_t mpt_alloc;
 
     /* Sanity! */
-    BUG_ON(d->domain_id != 0);
+    BUG_ON(!is_dom0_domain(d));
     BUG_ON(d->vcpu[0] == NULL);
     BUG_ON(v->is_initialised);
 
diff -r 924ce68580ff xen/arch/x86/hvm/i8254.c
--- a/xen/arch/x86/hvm/i8254.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/arch/x86/hvm/i8254.c	Thu Dec 09 20:56:43 2010 -0800
@@ -538,7 +538,7 @@
         .data = data
     };
 
-    if ( (current->domain->domain_id == 0) && dom0_pit_access(&ioreq) )
+    if ( is_dom0_domain(current->domain) && dom0_pit_access(&ioreq) )
     {
         /* nothing to do */;
     }
diff -r 924ce68580ff xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/arch/x86/time.c	Thu Dec 09 20:56:43 2010 -0800
@@ -1777,7 +1777,7 @@
                   uint32_t tsc_mode, uint64_t elapsed_nsec,
                   uint32_t gtsc_khz, uint32_t incarnation)
 {
-    if ( d->domain_id == 0 || d->domain_id == DOMID_INVALID )
+    if ( is_dom0_domain(d) || d->domain_id == DOMID_INVALID )
     {
         d->arch.vtsc = 0;
         return;
@@ -1854,7 +1854,7 @@
                "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_dom0_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);
diff -r 924ce68580ff xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/arch/x86/traps.c	Thu Dec 09 20:56:43 2010 -0800
@@ -748,7 +748,7 @@
     c = regs->ecx;
     d = regs->edx;
 
-    if ( current->domain->domain_id != 0 )
+    if ( !is_dom0_domain(current->domain) )
     {
         if ( !cpuid_hypervisor_leaves(a, c, &a, &b, &c, &d) )
             domain_cpuid(current->domain, a, c, &a, &b, &c, &d);
@@ -1694,7 +1694,7 @@
 static int is_cpufreq_controller(struct domain *d)
 {
     return ((cpufreq_controller == FREQCTL_dom0_kernel) &&
-            (d->domain_id == 0));
+            is_dom0_domain(d));
 }
 
 static int emulate_privileged_op(struct cpu_user_regs *regs)
@@ -2287,7 +2287,7 @@
         case MSR_IA32_THERM_CONTROL:
             if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL )
                 goto fail;
-            if ( (v->domain->domain_id != 0) || !v->domain->is_pinned )
+            if ( !is_dom0_domain(v->domain) || !v->domain->is_pinned )
                 break;
             if ( wrmsr_safe(regs->ecx, msr_content) != 0 )
                 goto fail;
diff -r 924ce68580ff xen/common/domain.c
--- a/xen/common/domain.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/common/domain.c	Thu Dec 09 20:56:43 2010 -0800
@@ -536,7 +536,7 @@
         d->shutdown_code = reason;
     reason = d->shutdown_code;
 
-    if ( d->domain_id == 0 )
+    if ( is_dom0_domain(d) )
         dom0_shutdown(reason);
 
     if ( d->is_shutting_down )
diff -r 924ce68580ff xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/common/sched_sedf.c	Thu Dec 09 20:56:43 2010 -0800
@@ -349,7 +349,7 @@
     inf->status      = EXTRA_AWARE | SEDF_ASLEEP;
     inf->extraweight = 1;
 
-    if ( v->domain->domain_id == 0 )
+    if ( is_dom0_domain(v->domain) )
     {
         /* Domain0 gets 75% guaranteed (15ms every 20ms). */
         inf->period    = MILLISECS(20);
diff -r 924ce68580ff xen/common/xenoprof.c
--- a/xen/common/xenoprof.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/common/xenoprof.c	Thu Dec 09 20:56:43 2010 -0800
@@ -605,7 +605,7 @@
 
     xenoprof_init.is_primary = 
         ((xenoprof_primary_profiler == d) ||
-         ((xenoprof_primary_profiler == NULL) && (d->domain_id == 0)));
+         ((xenoprof_primary_profiler == NULL) && is_dom0_domain(d)));
     if ( xenoprof_init.is_primary )
         xenoprof_primary_profiler = current->domain;
 
diff -r 924ce68580ff xen/drivers/char/console.c
--- a/xen/drivers/char/console.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/drivers/char/console.c	Thu Dec 09 20:56:43 2010 -0800
@@ -374,7 +374,7 @@
 
 #ifndef VERBOSE
     /* Only domain 0 may access the emergency console. */
-    if ( current->domain->domain_id != 0 )
+    if ( is_dom0_domain(current->domain) != 0 )
         return -EPERM;
 #endif
 
diff -r 924ce68580ff xen/drivers/passthrough/amd/pci_amd_iommu.c
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c	Thu Dec 09 20:56:43 2010 -0800
@@ -90,7 +90,7 @@
 
     BUG_ON( !hd->root_table || !hd->paging_mode || !iommu->dev_table.buffer );
 
-    if ( iommu_passthrough && (domain->domain_id == 0) )
+    if ( iommu_passthrough && is_dom0_domain(domain) )
         valid = 0;
 
     /* get device-table entry */
diff -r 924ce68580ff xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/drivers/passthrough/vtd/iommu.c	Thu Dec 09 20:56:43 2010 -0800
@@ -1231,7 +1231,7 @@
         return res;
     }
 
-    if ( iommu_passthrough && (domain->domain_id == 0) )
+    if ( iommu_passthrough && is_dom0_domain(domain) )
     {
         context_set_translation_type(*context, CONTEXT_TT_PASS_THRU);
         agaw = level_to_agaw(iommu->nr_pt_levels);
@@ -1597,7 +1597,7 @@
     int iommu_domid;
 
     /* do nothing if dom0 and iommu supports pass thru */
-    if ( iommu_passthrough && (d->domain_id == 0) )
+    if ( iommu_passthrough && is_dom0_domain(d) )
         return 0;
 
     spin_lock(&hd->mapping_lock);
@@ -1658,7 +1658,7 @@
 static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
 {
     /* Do nothing if dom0 and iommu supports pass thru. */
-    if ( iommu_passthrough && (d->domain_id == 0) )
+    if ( iommu_passthrough && is_dom0_domain(d) )
         return 0;
 
     dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
@@ -1764,7 +1764,7 @@
     /* If the device belongs to dom0, and it has RMRR, don't remove it
      * from dom0, because BIOS may use RMRR at booting time.
      */
-    if ( pdev->domain->domain_id == 0 )
+    if ( is_dom0_domain(pdev->domain) )
     {
         for_each_rmrr_device ( rmrr, bdf, i )
         {
diff -r 924ce68580ff xen/drivers/passthrough/vtd/x86/vtd.c
--- a/xen/drivers/passthrough/vtd/x86/vtd.c	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/drivers/passthrough/vtd/x86/vtd.c	Thu Dec 09 20:56:43 2010 -0800
@@ -131,7 +131,7 @@
     u64 i, j, tmp, max_pfn;
     extern int xen_in_range(unsigned long mfn);
 
-    BUG_ON(d->domain_id != 0);
+    BUG_ON(!is_dom0_domain(d));
 
     max_pfn = max_t(u64, max_page, 0x100000000ull >> PAGE_SHIFT);
 
diff -r 924ce68580ff xen/include/asm-x86/domain.h
--- a/xen/include/asm-x86/domain.h	Wed Dec 08 15:16:00 2010 -0800
+++ b/xen/include/asm-x86/domain.h	Thu Dec 09 20:56:43 2010 -0800
@@ -23,6 +23,8 @@
         d->arch.hvm_domain.irq.callback_via_type == HVMIRQ_callback_vector)
 #define is_hvm_pv_evtchn_vcpu(v) (is_hvm_pv_evtchn_domain(v->domain))
 
+#define is_dom0_domain(d)      ((d)->domain_id == 0)
+
 #define VCPU_TRAP_NMI          1
 #define VCPU_TRAP_MCE          2
 #define VCPU_TRAP_LAST         VCPU_TRAP_MCE

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Ian Campbell]

On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
> Replace a number of checks for Dom0, that have been hard coded to
> check for domain_id being zero with a macro is_dom0_domain().

Is the intention for this macro return true for some domid != 0 under
some future circumstance? In that case the macro name will turn out to
be badly chosen.

I'm not sure there is any benefit to hard coding a 0 in the function
name as opposed to hardcoding at the call site. I suppose it's a little
easier to search and replace...

Is there a name which describes the actual semantics which the callers
want, as opposed to testing the dom0-ness? Or perhaps there is more than
one desired semantic, in which case multiple predicates would be ok
IMHO. Does the existing IS_PRIV cover some of the cases?

Ian.

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 1634 bytes --]

Yes, the idea is to later have it, or another similar macro return true for
domids != 0. At the moment, I think it's likely that there will be other
separate predicates (maybe something like is_xenstore_domain,
is_control_domain, etc) for different disaggregated domains, and then have
the last bit continue to use this, even though it may no longer be domid 0.

You're right about the name being ill-chosen, but the only other name I
could come up with was is_what_used_to_be_dom0 which was even worse ;) I'm
open to suggestions. Perhaps, hardware domain or pci domain?

At the moment, IS_PRIV could be used, but it would lead to a coupling of the
privileges with functionality which could be problematic later on.

~M

On Fri, Dec 10, 2010 at 1:08 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:

> On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
> > Replace a number of checks for Dom0, that have been hard coded to
> > check for domain_id being zero with a macro is_dom0_domain().
>
> Is the intention for this macro return true for some domid != 0 under
> some future circumstance? In that case the macro name will turn out to
> be badly chosen.
>
> I'm not sure there is any benefit to hard coding a 0 in the function
> name as opposed to hardcoding at the call site. I suppose it's a little
> easier to search and replace...
>
> Is there a name which describes the actual semantics which the callers
> want, as opposed to testing the dom0-ness? Or perhaps there is more than
> one desired semantic, in which case multiple predicates would be ok
> IMHO. Does the existing IS_PRIV cover some of the cases?
>
> Ian.
>
>

[-- Attachment #1.2: Type: text/html, Size: 2057 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Keir Fraser]

I'm undecided. The patch by itself is kind of harmless but also kind of
pointless. Probably we should leave this until you have something more
substantial to propose. Trickling in trivial patches like this is a waste of
time.

 -- Keir

On 10/12/2010 10:13, "Mihir Nanavati" <mihirn@cs.ubc.ca> wrote:

> Yes, the idea is to later have it, or another similar macro return true for
> domids != 0. At the moment, I think it's likely that there will be other
> separate predicates (maybe something like is_xenstore_domain,
> is_control_domain, etc) for different disaggregated domains, and then have the
> last bit continue to use this, even though it may no longer be domid 0.
> 
> You're right about the name being ill-chosen, but the only other name I could
> come up with was is_what_used_to_be_dom0 which was even worse ;) I'm open to
> suggestions. Perhaps, hardware domain or pci domain?
> 
> At the moment, IS_PRIV could be used, but it would lead to a coupling of the
> privileges with functionality which could be problematic later on.
> 
> ~M
> 
> On Fri, Dec 10, 2010 at 1:08 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
>>> Replace a number of checks for Dom0, that have been hard coded to
>>> check for domain_id being zero with a macro is_dom0_domain().
>> 
>> Is the intention for this macro return true for some domid != 0 under
>> some future circumstance? In that case the macro name will turn out to
>> be badly chosen.
>> 
>> I'm not sure there is any benefit to hard coding a 0 in the function
>> name as opposed to hardcoding at the call site. I suppose it's a little
>> easier to search and replace...
>> 
>> Is there a name which describes the actual semantics which the callers
>> want, as opposed to testing the dom0-ness? Or perhaps there is more than
>> one desired semantic, in which case multiple predicates would be ok
>> IMHO. Does the existing IS_PRIV cover some of the cases?
>> 
>> Ian.
>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Ian Campbell]

On Fri, 2010-12-10 at 10:13 +0000, Mihir Nanavati wrote:
> Yes, the idea is to later have it, or another similar macro return
> true for domids != 0. At the moment, I think it's likely that there
> will be other separate predicates (maybe something like
> is_xenstore_domain, is_control_domain, etc) for different
> disaggregated domains, and then have the last bit continue to use
> this, even though it may no longer be domid 0.
> 
> You're right about the name being ill-chosen, but the only other name
> I could come up with was is_what_used_to_be_dom0 which was even
> worse ;) I'm open to suggestions. Perhaps, hardware domain or pci
> domain?

I don't have any better suggestions :-(

I think it very much depends on what the call site is actually trying to
test and how it will be disaggregated in the future. If you are just
going to have to switch to another macro when you do the disaggregation
what's the benefit of switching now?

> At the moment, IS_PRIV could be used, but it would lead to a coupling
> of the privileges with functionality which could be problematic later
> on.
> 
> ~M
> 
> On Fri, Dec 10, 2010 at 1:08 AM, Ian Campbell
> <Ian.Campbell@citrix.com> wrote:
>         On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
>         > Replace a number of checks for Dom0, that have been hard
>         coded to
>         > check for domain_id being zero with a macro
>         is_dom0_domain().
>         
>         
>         Is the intention for this macro return true for some domid !=
>         0 under
>         some future circumstance? In that case the macro name will
>         turn out to
>         be badly chosen.
>         
>         I'm not sure there is any benefit to hard coding a 0 in the
>         function
>         name as opposed to hardcoding at the call site. I suppose it's
>         a little
>         easier to search and replace...
>         
>         Is there a name which describes the actual semantics which the
>         callers
>         want, as opposed to testing the dom0-ness? Or perhaps there is
>         more than
>         one desired semantic, in which case multiple predicates would
>         be ok
>         IMHO. Does the existing IS_PRIV cover some of the cases?
>         
>         Ian.
>         
> 

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Mihir Nanavati]

[-- Attachment #1.1: Type: text/plain, Size: 2566 bytes --]

Fair enough, I guess it's more useful for us here in testing to be able to
switch to having another domid as dom0 from a single place then it would be
in a production system. Will keep it on hold till we've gotten some more
pieces into place.

~M

On Fri, Dec 10, 2010 at 2:42 AM, Keir Fraser <keir@xen.org> wrote:

> I'm undecided. The patch by itself is kind of harmless but also kind of
> pointless. Probably we should leave this until you have something more
> substantial to propose. Trickling in trivial patches like this is a waste
> of
> time.
>
>  -- Keir
>
> On 10/12/2010 10:13, "Mihir Nanavati" <mihirn@cs.ubc.ca> wrote:
>
> > Yes, the idea is to later have it, or another similar macro return true
> for
> > domids != 0. At the moment, I think it's likely that there will be other
> > separate predicates (maybe something like is_xenstore_domain,
> > is_control_domain, etc) for different disaggregated domains, and then
> have the
> > last bit continue to use this, even though it may no longer be domid 0.
> >
> > You're right about the name being ill-chosen, but the only other name I
> could
> > come up with was is_what_used_to_be_dom0 which was even worse ;) I'm open
> to
> > suggestions. Perhaps, hardware domain or pci domain?
> >
> > At the moment, IS_PRIV could be used, but it would lead to a coupling of
> the
> > privileges with functionality which could be problematic later on.
> >
> > ~M
> >
> > On Fri, Dec 10, 2010 at 1:08 AM, Ian Campbell <Ian.Campbell@citrix.com>
> wrote:
> >> On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
> >>> Replace a number of checks for Dom0, that have been hard coded to
> >>> check for domain_id being zero with a macro is_dom0_domain().
> >>
> >> Is the intention for this macro return true for some domid != 0 under
> >> some future circumstance? In that case the macro name will turn out to
> >> be badly chosen.
> >>
> >> I'm not sure there is any benefit to hard coding a 0 in the function
> >> name as opposed to hardcoding at the call site. I suppose it's a little
> >> easier to search and replace...
> >>
> >> Is there a name which describes the actual semantics which the callers
> >> want, as opposed to testing the dom0-ness? Or perhaps there is more than
> >> one desired semantic, in which case multiple predicates would be ok
> >> IMHO. Does the existing IS_PRIV cover some of the cases?
> >>
> >> Ian.
> >>
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 3462 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] xen : Replace hard coded domain_id checks with a macro

[Keir Fraser]

Yeah, that's the pain of an out-of-tree patch queue I'm afraid. You have to
suck it up. :-)

On 10/12/2010 11:06, "Mihir Nanavati" <mihirn@cs.ubc.ca> wrote:

> Fair enough, I guess it's more useful for us here in testing to be able to
> switch to having another domid as dom0 from a single place then it would be in
> a production system. Will keep it on hold till we've gotten some more pieces
> into place.
> 
> ~M
> 
> On Fri, Dec 10, 2010 at 2:42 AM, Keir Fraser <keir@xen.org> wrote:
>> I'm undecided. The patch by itself is kind of harmless but also kind of
>> pointless. Probably we should leave this until you have something more
>> substantial to propose. Trickling in trivial patches like this is a waste of
>> time.
>> 
>>  -- Keir
>> 
>> On 10/12/2010 10:13, "Mihir Nanavati" <mihirn@cs.ubc.ca> wrote:
>> 
>>> Yes, the idea is to later have it, or another similar macro return true for
>>> domids != 0. At the moment, I think it's likely that there will be other
>>> separate predicates (maybe something like is_xenstore_domain,
>>> is_control_domain, etc) for different disaggregated domains, and then have
>>> the
>>> last bit continue to use this, even though it may no longer be domid 0.
>>> 
>>> You're right about the name being ill-chosen, but the only other name I
>>> could
>>> come up with was is_what_used_to_be_dom0 which was even worse ;) I'm open to
>>> suggestions. Perhaps, hardware domain or pci domain?
>>> 
>>> At the moment, IS_PRIV could be used, but it would lead to a coupling of the
>>> privileges with functionality which could be problematic later on.
>>> 
>>> ~M
>>> 
>>> On Fri, Dec 10, 2010 at 1:08 AM, Ian Campbell <Ian.Campbell@citrix.com>
>>> wrote:
>>>> On Fri, 2010-12-10 at 07:07 +0000, Mihir Nanavati wrote:
>>>>> Replace a number of checks for Dom0, that have been hard coded to
>>>>> check for domain_id being zero with a macro is_dom0_domain().
>>>> 
>>>> Is the intention for this macro return true for some domid != 0 under
>>>> some future circumstance? In that case the macro name will turn out to
>>>> be badly chosen.
>>>> 
>>>> I'm not sure there is any benefit to hard coding a 0 in the function
>>>> name as opposed to hardcoding at the call site. I suppose it's a little
>>>> easier to search and replace...
>>>> 
>>>> Is there a name which describes the actual semantics which the callers
>>>> want, as opposed to testing the dom0-ness? Or perhaps there is more than
>>>> one desired semantic, in which case multiple predicates would be ok
>>>> IMHO. Does the existing IS_PRIV cover some of the cases?
>>>> 
>>>> Ian.
>>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xensource.com
>>> http://lists.xensource.com/xen-devel
>> 
>> 
> 
>