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
Andrew Cooper
2013-Oct-07 09:48 UTC
[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
Andrew Cooper
2013-Oct-07 09:48 UTC
[Patch 2/2] 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 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
Jan Beulich
2013-Oct-07 10:03 UTC
Re: [Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()
>>> 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
Jan Beulich
2013-Oct-07 10:17 UTC
Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
>>> 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
Andrew Cooper
2013-Oct-07 10:51 UTC
Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
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
Andrew Cooper
2013-Oct-07 11:59 UTC
[Patch v2] 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..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
Andrew Cooper
2013-Oct-07 12:01 UTC
[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 )
{
+ /* 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
Paul Durrant
2013-Oct-07 12:26 UTC
Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
> -----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
Andrew Cooper
2013-Oct-07 13:15 UTC
Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
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? > > PaulI 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
Jan Beulich
2013-Oct-07 13:36 UTC
Re: [Patch v3] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
>>> 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
Andrew Cooper
2013-Oct-07 13:46 UTC
[Patch v4] 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,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
Zhang, Xiantao
2013-Oct-07 14:50 UTC
Re: [Patch 1/2] x86/vtd: Fix suspected data race condition in iommu_set_root_entry()
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
Jan Beulich
2013-Oct-08 08:52 UTC
Re: [Patch v4] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
>>> 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
Andrew Cooper
2013-Oct-08 09:32 UTC
Re: [Patch v4] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()
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
Andrew Cooper
2013-Oct-08 09:33 UTC
[Patch v5] 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,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