[Patch 0/2] Misc coverity fixes (set 2)

[Patch 0/2] Misc coverity fixes (set 2)

[Andrew Cooper]

Here are fixes for two minior Coverity-identified issues.

In both cases, despite the identified issue having no effect, I felt the
improvements were appropriate.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

--
1.7.10.4

[Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()

[Andrew Cooper]

Coverity ID: 1054967

Coverity spotted that iommu->root_maddr was optionally allocated within the
protection of the iommu->lock, but was referenced with the protection of the
iommu->register_lock, and freed without any lock.

Luckily, the code as-is is not vulnerable to the potential risks identified.

However, the alloc_pgtable_maddr() is far more appropriately done in
iommu_alloc(), removing a set of spinlock calls, and a possibility for the
iommu setup to fail later than iommu_alloc() with an -ENOMEM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Xiantao Zhang <xiantao.zhang@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 12e0165..2dbe97a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -696,25 +696,9 @@ static void iommu_free_pagetable(u64 pt_maddr, int level)
 
 static int iommu_set_root_entry(struct iommu *iommu)
 {
-    struct acpi_drhd_unit *drhd;
     u32 sts;
     unsigned long flags;
 
-    spin_lock(&iommu->lock);
-
-    if ( iommu->root_maddr == 0 )
-    {
-        drhd = iommu_to_drhd(iommu);
-        iommu->root_maddr = alloc_pgtable_maddr(drhd, 1);
-    }
-
-    if ( iommu->root_maddr == 0 )
-    {
-        spin_unlock(&iommu->lock);
-        return -ENOMEM;
-    }
-
-    spin_unlock(&iommu->lock);
     spin_lock_irqsave(&iommu->register_lock, flags);
     dmar_writeq(iommu->reg, DMAR_RTADDR_REG, iommu->root_maddr);
 
@@ -1144,6 +1128,9 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
     iommu->intel->drhd = drhd;
     drhd->iommu = iommu;
 
+    if ( !(iommu->root_maddr = alloc_pgtable_maddr(drhd, 1)) )
+        return -ENOMEM;
+
     iommu->reg = ioremap(drhd->address, PAGE_SIZE);
     if ( !iommu->reg )
         return -ENOMEM;
-- 
1.7.10.4

[Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_SHIFT and PAGE_MASK instead of opencoded numbers.
 * a more descriptive error message for attempts to write a hypercall page at
   an unaligned frame address.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c |   38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..7aa4088 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,45 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         *val = 0;
-        break;
+        return 1;
     }
     default:
-        BUG();
+        return 0;
     }
-
-    return 1;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( val & PAGE_MASK )
         {
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "Expected aligned frame for writing hypercall page\n");
             return 0;
         }
 
@@ -671,14 +661,12 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
 
     default:
-        BUG();
+        return 0;
     }
-
-    return 1;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4

Re: [Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()

[Jan Beulich]

>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Coverity ID: 1054967
> 
> Coverity spotted that iommu->root_maddr was optionally allocated within the
> protection of the iommu->lock, but was referenced with the protection of the
> iommu->register_lock, and freed without any lock.
> 
> Luckily, the code as-is is not vulnerable to the potential risks identified.
> 
> However, the alloc_pgtable_maddr() is far more appropriately done in
> iommu_alloc(), removing a set of spinlock calls, and a possibility for the
> iommu setup to fail later than iommu_alloc() with an -ENOMEM.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> CC: Xiantao Zhang <xiantao.zhang@intel.com>
> ---
>  xen/drivers/passthrough/vtd/iommu.c |   19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c 
> b/xen/drivers/passthrough/vtd/iommu.c
> index 12e0165..2dbe97a 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -696,25 +696,9 @@ static void iommu_free_pagetable(u64 pt_maddr, int 
> level)
>  
>  static int iommu_set_root_entry(struct iommu *iommu)
>  {
> -    struct acpi_drhd_unit *drhd;
>      u32 sts;
>      unsigned long flags;
>  
> -    spin_lock(&iommu->lock);
> -
> -    if ( iommu->root_maddr == 0 )
> -    {
> -        drhd = iommu_to_drhd(iommu);
> -        iommu->root_maddr = alloc_pgtable_maddr(drhd, 1);
> -    }
> -
> -    if ( iommu->root_maddr == 0 )
> -    {
> -        spin_unlock(&iommu->lock);
> -        return -ENOMEM;
> -    }
> -
> -    spin_unlock(&iommu->lock);
>      spin_lock_irqsave(&iommu->register_lock, flags);
>      dmar_writeq(iommu->reg, DMAR_RTADDR_REG, iommu->root_maddr);
>  
> @@ -1144,6 +1128,9 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>      iommu->intel->drhd = drhd;
>      drhd->iommu = iommu;
>  
> +    if ( !(iommu->root_maddr = alloc_pgtable_maddr(drhd, 1)) )
> +        return -ENOMEM;
> +
>      iommu->reg = ioremap(drhd->address, PAGE_SIZE);
>      if ( !iommu->reg )
>          return -ENOMEM;
> -- 
> 1.7.10.4

Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Jan Beulich]

>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -595,55 +595,45 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>  
> +/* Returns 1 if handled, 0 if not and -Exx for error. */

This comment is not in line with all current uses of the function.
Either you fix the comment, or you fix the callers.

>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>  {
>      struct domain *d = current->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>  
> -    idx -= base;
> -    if ( idx > 0 )
> -        return 0;
> -
> -    switch ( idx )
> +    switch ( idx - base )
>      {
> -    case 0:
> +    case 0: /* Write hypercall page */

This comment looks more confusing that clarifying considering that
we're in "rdmsr_...".

>      {
>          *val = 0;
> -        break;
> +        return 1;
>      }
>      default:
> -        BUG();
> +        return 0;

In a situation like this I think it is better to not have a "default:" at
all, and instead have the "return" at the end of the function deal
with all cases not getting handled inside the switch statement.
But yes, this is a matter of taste.

>      }
> -
> -    return 1;
>  }
>  
> +/* Returns 1 if handled, 0 if not and -Exx for error. */
>  int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>  {
>      struct domain *d = current->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>  
> -    idx -= base;
> -    if ( idx > 0 )
> -        return 0;
> -
> -    switch ( idx )
> +    switch ( idx - base )
>      {
> -    case 0:
> +    case 0: /* Write hypercall page */
>      {
>          void *hypercall_page;
> -        unsigned long gmfn = val >> 12;
> -        unsigned int idx  = val & 0xfff;
> +        unsigned long gmfn = val >> PAGE_SHIFT;
>          struct page_info *page;
>          p2m_type_t t;
>  
> -        if ( idx > 0 )
> +        if ( val & PAGE_MASK )

Did you mean ~PAGE_MASK? And in the light of this - did you test
the change?

>          {
>              gdprintk(XENLOG_WARNING,
> -                     "Out of range index %u to MSR %08x\n",
> -                     idx, 0x40000000);
> +                     "Expected aligned frame for writing hypercall page\n");

I don't think that's the intention here. Instead the low bits are
specifying the n-th hypercall page, and hence talking about
alignment here seems wrong.

Jan

Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

On 07/10/13 11:17, Jan Beulich wrote:
>>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -595,55 +595,45 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
>>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>>  
>> +/* Returns 1 if handled, 0 if not and -Exx for error. */
> This comment is not in line with all current uses of the function.
> Either you fix the comment, or you fix the callers.

Hmm yes - the error case isn't dealt with properly.  I shall fix the
comment in preference to editing the callsites at the moment.

I have a separate proposal for a change in the way this msr handling
code works, and will fix this up then.

>
>>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>  
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page */
> This comment looks more confusing that clarifying considering that
> we're in "rdmsr_...".

Very true.  I shall adjust.

>
>>      {
>>          *val = 0;
>> -        break;
>> +        return 1;
>>      }
>>      default:
>> -        BUG();
>> +        return 0;
> In a situation like this I think it is better to not have a "default:" at
> all, and instead have the "return" at the end of the function deal
> with all cases not getting handled inside the switch statement.
> But yes, this is a matter of taste.
>
>>      }
>> -
>> -    return 1;
>>  }
>>  
>> +/* Returns 1 if handled, 0 if not and -Exx for error. */
>>  int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>  
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page */
>>      {
>>          void *hypercall_page;
>> -        unsigned long gmfn = val >> 12;
>> -        unsigned int idx  = val & 0xfff;
>> +        unsigned long gmfn = val >> PAGE_SHIFT;
>>          struct page_info *page;
>>          p2m_type_t t;
>>  
>> -        if ( idx > 0 )
>> +        if ( val & PAGE_MASK )
> Did you mean ~PAGE_MASK? And in the light of this - did you test
> the change?

I did indeed mean ~PAGE_MASK, but also had that in the version of the
code tested.  I think I lost that in a botched rebase, and shall try to
be more careful in the future.

>
>>          {
>>              gdprintk(XENLOG_WARNING,
>> -                     "Out of range index %u to MSR %08x\n",
>> -                     idx, 0x40000000);
>> +                     "Expected aligned frame for writing hypercall page\n");
> I don't think that's the intention here. Instead the low bits are
> specifying the n-th hypercall page, and hence talking about
> alignment here seems wrong.
>
> Jan
>

Is this documented anywhere?  The cpuid documentation describes how to
locate the base address.

Looking at things more closely, that does make sense. I did mistake it
for an alignment check, given unconditionally 1 hypercall page.

I will return it back to what it was intended, but without shadowing the
idx function parameter.

~Andrew

[Patch v2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_SHIFT instead of opencoded numbers.
 * a more descriptive error message for attempting to write invalid indicies
   for hypercall pages.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..8797e56 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/*
+ * Returns 0 if not handled, and non-0 for error. (The calling semantics are
+ * in need of some work)
+ */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
     {
         *val = 0;
-        break;
+        return 1;
     }
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( val & 0xfff )
         {
+            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "wrmsr hypercall page index %#x unsupported\n",
+                     val & 0xfff);
             return 0;
         }
 
@@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
-
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4

[Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_SHIFT instead of opencoded numbers.
 * a more descriptive error message for attempting to write invalid indicies
   for hypercall pages.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..3bb62f0 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/*
+ * Returns 0 if not handled, and non-0 for error. (The calling semantics are
+ * in need of some work)
+ */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
     {
         *val = 0;
-        break;
+        return 1;
     }
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( val & 0xfff )
         {
+            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "wrmsr hypercall page index %#lx unsupported\n",
+                     val & 0xfff);
             return 0;
         }
 
@@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
-
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4

Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 07 October 2013 13:01
> To: Xen-devel
> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
> Subject: [Xen-devel] [Patch v3] x86/traps: improvements to {rd,
> wr}msr_hypervisor_regs()
> 
> Coverity ID: 1055249 1055250
> 
> Coverity was complaining that the switch statments contained dead code in
> their default statements.  While this is quite minor, the code flow in
> wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
> fix.
> 
> Other improvements include:
>  * not shadowing the function parameter 'idx'.
>  * use of PAGE_SHIFT instead of opencoded numbers.
>  * a more descriptive error message for attempting to write invalid indicies
>    for hypercall pages.
> 
> There is no behavioural change as a result.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
>  1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 6c7bd99..3bb62f0 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,
> coprocessor_error)
>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
> 
> +/*
> + * Returns 0 if not handled, and non-0 for error. (The calling semantics are
> + * in need of some work)
> + */
>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>  {
>      struct domain *d = current->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> 
> -    idx -= base;
> -    if ( idx > 0 )
> -        return 0;
> -
> -    switch ( idx )
> +    switch ( idx - base )
>      {
> -    case 0:
> +    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
>      {
>          *val = 0;
> -        break;
> +        return 1;
>      }
> -    default:
> -        BUG();
>      }
> 
> -    return 1;
> +    return 0;
>  }
> 
> +/* Returns 1 if handled, 0 if not and -Exx for error. */
>  int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>  {
>      struct domain *d = current->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> 
> -    idx -= base;
> -    if ( idx > 0 )
> -        return 0;
> -
> -    switch ( idx )
> +    switch ( idx - base )
>      {
> -    case 0:
> +    case 0: /* Write hypercall page */
>      {
>          void *hypercall_page;
> -        unsigned long gmfn = val >> 12;
> -        unsigned int idx  = val & 0xfff;
> +        unsigned long gmfn = val >> PAGE_SHIFT;
>          struct page_info *page;
>          p2m_type_t t;
> 
> -        if ( idx > 0 )
> +        if ( val & 0xfff )

If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 0xfff?

>          {
> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */

And modify this comment accordingly?

  Paul

>              gdprintk(XENLOG_WARNING,
> -                     "Out of range index %u to MSR %08x\n",
> -                     idx, 0x40000000);
> +                     "wrmsr hypercall page index %#lx unsupported\n",
> +                     val & 0xfff);
>              return 0;
>          }
> 
> @@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t
> val)
>          unmap_domain_page(hypercall_page);
> 
>          put_page_and_type(page);
> -        break;
> +        return 1;
>      }
> -
> -    default:
> -        BUG();
>      }
> 
> -    return 1;
> +    return 0;
>  }
> 
>  int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

On 07/10/13 13:26, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 07 October 2013 13:01
>> To: Xen-devel
>> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
>> Subject: [Xen-devel] [Patch v3] x86/traps: improvements to {rd,
>> wr}msr_hypervisor_regs()
>>
>> Coverity ID: 1055249 1055250
>>
>> Coverity was complaining that the switch statments contained dead code in
>> their default statements.  While this is quite minor, the code flow in
>> wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
>> fix.
>>
>> Other improvements include:
>>  * not shadowing the function parameter 'idx'.
>>  * use of PAGE_SHIFT instead of opencoded numbers.
>>  * a more descriptive error message for attempting to write invalid indicies
>>    for hypercall pages.
>>
>> There is no behavioural change as a result.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
>>  1 file changed, 18 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 6c7bd99..3bb62f0 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,
>> coprocessor_error)
>>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>>
>> +/*
>> + * Returns 0 if not handled, and non-0 for error. (The calling semantics are
>> + * in need of some work)
>> + */
>>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
>>      {
>>          *val = 0;
>> -        break;
>> +        return 1;
>>      }
>> -    default:
>> -        BUG();
>>      }
>>
>> -    return 1;
>> +    return 0;
>>  }
>>
>> +/* Returns 1 if handled, 0 if not and -Exx for error. */
>>  int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page */
>>      {
>>          void *hypercall_page;
>> -        unsigned long gmfn = val >> 12;
>> -        unsigned int idx  = val & 0xfff;
>> +        unsigned long gmfn = val >> PAGE_SHIFT;
>>          struct page_info *page;
>>          p2m_type_t t;
>>
>> -        if ( idx > 0 )
>> +        if ( val & 0xfff )
> If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 0xfff?
>
>>          {
>> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
> And modify this comment accordingly?
>
>   Paul

I suppose.  The use of ~PAGE_MASK before was my mistaken impression that
this was an alignment check rather than a hypercall index check.

As this stuff is not documented anywhere I can find (other than by
reading the code), I am not sure it is caring too much about.  Ideally,
there would be somewhere XEN_MSR_HYPERCALL_PAGE_INDEX_MASK as 0xfff and
XEN_MSR_HYPERCALL_PAGE_MAX_INDEX, but as it is unlikely that Xen will
ever start using multiple hypercall pages, this sounds like overkill.

I am happy to implement it however people prefer, but would err on the
lazy side of leaving it alone if there is no overwhelming objection.

~Andrew

Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Jan Beulich]

>>> On 07.10.13 at 15:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> -    switch ( idx )
>>> +    switch ( idx - base )
>>>      {
>>> -    case 0:
>>> +    case 0: /* Write hypercall page */
>>>      {
>>>          void *hypercall_page;
>>> -        unsigned long gmfn = val >> 12;
>>> -        unsigned int idx  = val & 0xfff;
>>> +        unsigned long gmfn = val >> PAGE_SHIFT;
>>>          struct page_info *page;
>>>          p2m_type_t t;
>>>
>>> -        if ( idx > 0 )
>>> +        if ( val & 0xfff )
>> If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 0xfff?
>>
>>>          {
>>> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
>> And modify this comment accordingly?
> 
> I suppose.  The use of ~PAGE_MASK before was my mistaken impression that
> this was an alignment check rather than a hypercall index check.
> 
> As this stuff is not documented anywhere I can find (other than by
> reading the code), I am not sure it is caring too much about.  Ideally,
> there would be somewhere XEN_MSR_HYPERCALL_PAGE_INDEX_MASK as 0xfff and
> XEN_MSR_HYPERCALL_PAGE_MAX_INDEX, but as it is unlikely that Xen will
> ever start using multiple hypercall pages, this sounds like overkill.
> 
> I am happy to implement it however people prefer, but would err on the
> lazy side of leaving it alone if there is no overwhelming objection.

As Paul said - if you use PAGE_SHIFT, you also should replace 0xfff.

Jan

[Patch v4] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_{SHIFT,SIZE} instead of opencoded numbers.
 * a more descriptive error message for attempting to write invalid indicies
   for hypercall pages.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..2355531 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/*
+ * Returns 0 if not handled, and non-0 for error. (The calling semantics are
+ * in need of some work)
+ */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
     {
         *val = 0;
-        break;
+        return 1;
     }
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
+        unsigned int page_index = val & (PAGE_SIZE - 1);
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( page_index > 0 )
         {
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "wrmsr hypercall page index %#x unsupported\n",
+                     page_index);
             return 0;
         }
 
@@ -671,14 +666,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
-
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4

Re: [Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()

[Zhang, Xiantao]

Thanks, Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
Xiantao

-----Original Message-----
From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] 
Sent: Monday, October 7, 2013 5:49 PM
To: Xen-devel
Cc: Andrew Cooper; Keir Fraser; Jan Beulich; Zhang, Xiantao
Subject: [Xen-devel] [Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()

Coverity ID: 1054967

Coverity spotted that iommu->root_maddr was optionally allocated within the protection of the iommu->lock, but was referenced with the protection of the
iommu->register_lock, and freed without any lock.

Luckily, the code as-is is not vulnerable to the potential risks identified.

However, the alloc_pgtable_maddr() is far more appropriately done in iommu_alloc(), removing a set of spinlock calls, and a possibility for the iommu setup to fail later than iommu_alloc() with an -ENOMEM.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Xiantao Zhang <xiantao.zhang@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c |   19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 12e0165..2dbe97a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -696,25 +696,9 @@ static void iommu_free_pagetable(u64 pt_maddr, int level)
 
 static int iommu_set_root_entry(struct iommu *iommu)  {
-    struct acpi_drhd_unit *drhd;
     u32 sts;
     unsigned long flags;
 
-    spin_lock(&iommu->lock);
-
-    if ( iommu->root_maddr == 0 )
-    {
-        drhd = iommu_to_drhd(iommu);
-        iommu->root_maddr = alloc_pgtable_maddr(drhd, 1);
-    }
-
-    if ( iommu->root_maddr == 0 )
-    {
-        spin_unlock(&iommu->lock);
-        return -ENOMEM;
-    }
-
-    spin_unlock(&iommu->lock);
     spin_lock_irqsave(&iommu->register_lock, flags);
     dmar_writeq(iommu->reg, DMAR_RTADDR_REG, iommu->root_maddr);
 
@@ -1144,6 +1128,9 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
     iommu->intel->drhd = drhd;
     drhd->iommu = iommu;
 
+    if ( !(iommu->root_maddr = alloc_pgtable_maddr(drhd, 1)) )
+        return -ENOMEM;
+
     iommu->reg = ioremap(drhd->address, PAGE_SIZE);
     if ( !iommu->reg )
         return -ENOMEM;
--
1.7.10.4

Re: [Patch v4] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Jan Beulich]

>>> On 07.10.13 at 15:46, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>  
> +/*
> + * Returns 0 if not handled, and non-0 for error. (The calling semantics are
> + * in need of some work)
> + */
>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>  {

The comment here still isn't in line with the existing callers. Non-
zero means success afaict. There simply is no path resulting in
an error here so far.

> +    switch ( idx - base )
>      {
> -    case 0:
> +    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
>      {
>          *val = 0;
> -        break;
> +        return 1;

And the above means that there's no #GP being "handed back"
here either.

Jan

Re: [Patch v4] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

On 08/10/13 09:52, Jan Beulich wrote:
>>>> On 07.10.13 at 15:46, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -595,55 +595,50 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
>>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>>  
>> +/*
>> + * Returns 0 if not handled, and non-0 for error. (The calling semantics are
>> + * in need of some work)
>> + */
>>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>>  {
> The comment here still isn't in line with the existing callers. Non-
> zero means success afaict. There simply is no path resulting in
> an error here so far.
>
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page.  Reads are invalid. Hand a #GP back. */
>>      {
>>          *val = 0;
>> -        break;
>> +        return 1;
> And the above means that there's no #GP being "handed back"
> here either.
>
> Jan
>

/sigh - Quite right.  I misread the where the breaks were going in the
callers.

~Andrew

[Patch v5] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper]

Coverity ID: 1055249 1055250

Coverity was complaining that the switch statments contained dead code in
their default statements.  While this is quite minor, the code flow in
wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
fix.

Other improvements include:
 * not shadowing the function parameter 'idx'.
 * use of PAGE_{SHIFT,SIZE} instead of opencoded numbers.
 * a more descriptive error message for attempting to write invalid indicies
   for hypercall pages.

There is no behavioural change as a result.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/traps.c |   41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6c7bd99..47c71b7 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -595,55 +595,47 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
 DO_ERROR(       TRAP_alignment_check, alignment_check)
 DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
 
+/* Returns 0 if not handled, and non-0 for success. */
 int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page MSR.  Read as zero. */
     {
         *val = 0;
-        break;
+        return 1;
     }
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
+/* Returns 1 if handled, 0 if not and -Exx for error. */
 int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
 {
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
     uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
 
-    idx -= base;
-    if ( idx > 0 )
-        return 0;
-
-    switch ( idx )
+    switch ( idx - base )
     {
-    case 0:
+    case 0: /* Write hypercall page */
     {
         void *hypercall_page;
-        unsigned long gmfn = val >> 12;
-        unsigned int idx  = val & 0xfff;
+        unsigned long gmfn = val >> PAGE_SHIFT;
+        unsigned int page_index = val & (PAGE_SIZE - 1);
         struct page_info *page;
         p2m_type_t t;
 
-        if ( idx > 0 )
+        if ( page_index > 0 )
         {
             gdprintk(XENLOG_WARNING,
-                     "Out of range index %u to MSR %08x\n",
-                     idx, 0x40000000);
+                     "wrmsr hypercall page index %#x unsupported\n",
+                     page_index);
             return 0;
         }
 
@@ -671,14 +663,11 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        break;
+        return 1;
     }
-
-    default:
-        BUG();
     }
 
-    return 1;
+    return 0;
 }
 
 int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
-- 
1.7.10.4