* [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:33 ` Tian, Kevin
2016-04-25 9:22 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
` (9 subsequent siblings)
10 siblings, 2 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Quan Xu, Andrew Cooper, dario.faggioli,
Jan Beulich, Feng Wu
The propagation value from IOMMU flush interfaces may be positive, which
indicates callers need to flush cache, not one of faliures.
when the propagation value is positive, this patch fixes this flush issue
as follows:
- call iommu_flush_write_buffer() to flush cache.
- return zero.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/drivers/passthrough/vtd/iommu.c | 94 ++++++++++++++++++++++++-------------
xen/include/asm-x86/iommu.h | 2 +-
2 files changed, 63 insertions(+), 33 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5ad25dc..50d98ac 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -558,14 +558,16 @@ static void iommu_flush_all(void)
}
}
-static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
- int dma_old_pte_present, unsigned int page_count)
+static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
+ int dma_old_pte_present,
+ unsigned int page_count)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
struct acpi_drhd_unit *drhd;
struct iommu *iommu;
int flush_dev_iotlb;
int iommu_domid;
+ int rc = 0;
/*
* No need pcideves_lock here because we have flush
@@ -584,29 +586,34 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
continue;
if ( page_count != 1 || gfn == INVALID_GFN )
- {
- if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
- 0, flush_dev_iotlb) )
- iommu_flush_write_buffer(iommu);
- }
+ rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
+ 0, flush_dev_iotlb);
else
+ rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
+ (paddr_t)gfn << PAGE_SHIFT_4K,
+ PAGE_ORDER_4K,
+ !dma_old_pte_present,
+ flush_dev_iotlb);
+
+ if ( rc > 0 )
{
- if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
- (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
- !dma_old_pte_present, flush_dev_iotlb) )
- iommu_flush_write_buffer(iommu);
- }
+ iommu_flush_write_buffer(iommu);
+ rc = 0;
+ } else if ( rc < 0 )
+ break;
}
+
+ return rc;
}
static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
{
- __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
+ iommu_flush_iotlb(d, gfn, 1, page_count);
}
static void intel_iommu_iotlb_flush_all(struct domain *d)
{
- __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
+ iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
}
/* clear one page's page table */
@@ -640,7 +647,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
if ( !this_cpu(iommu_dont_flush_iotlb) )
- __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
+ iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);
unmap_vtd_domain_page(page);
}
@@ -1281,6 +1288,7 @@ int domain_context_mapping_one(
u64 maddr, pgd_maddr;
u16 seg = iommu->intel->drhd->segment;
int agaw;
+ int rc;
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
@@ -1394,13 +1402,19 @@ int domain_context_mapping_one(
spin_unlock(&iommu->lock);
/* Context entry was previously non-present (with domid 0). */
- if ( iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
- DMA_CCMD_MASK_NOBIT, 1) )
- iommu_flush_write_buffer(iommu);
- else
+ rc = iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
+ DMA_CCMD_MASK_NOBIT, 1);
+
+ if ( !rc )
{
int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
- iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
+ rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
+ }
+
+ if ( rc > 0 )
+ {
+ iommu_flush_write_buffer(iommu);
+ rc = 0;
}
set_bit(iommu->index, &hd->arch.iommu_bitmap);
@@ -1410,7 +1424,7 @@ int domain_context_mapping_one(
if ( !seg )
me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
- return 0;
+ return rc;
}
static int domain_context_mapping(
@@ -1505,6 +1519,7 @@ int domain_context_unmap_one(
struct context_entry *context, *context_entries;
u64 maddr;
int iommu_domid;
+ int rc;
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
@@ -1532,14 +1547,20 @@ int domain_context_unmap_one(
return -EINVAL;
}
- if ( iommu_flush_context_device(iommu, iommu_domid,
+ rc = iommu_flush_context_device(iommu, iommu_domid,
(((u16)bus) << 8) | devfn,
- DMA_CCMD_MASK_NOBIT, 0) )
- iommu_flush_write_buffer(iommu);
- else
+ DMA_CCMD_MASK_NOBIT, 0);
+
+ if ( !rc )
{
int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
- iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb);
+ rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb);
+ }
+
+ if ( rc > 0 )
+ {
+ iommu_flush_write_buffer(iommu);
+ rc = 0;
}
spin_unlock(&iommu->lock);
@@ -1548,7 +1569,7 @@ int domain_context_unmap_one(
if ( !iommu->intel->drhd->segment )
me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
- return 0;
+ return rc;
}
static int domain_context_unmap(
@@ -1738,7 +1759,7 @@ static int intel_iommu_map_page(
unmap_vtd_domain_page(page);
if ( !this_cpu(iommu_dont_flush_iotlb) )
- __intel_iommu_iotlb_flush(d, gfn, dma_pte_present(old), 1);
+ iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
return 0;
}
@@ -1754,14 +1775,15 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
return 0;
}
-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
- int order, int present)
+int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
+ int order, int present)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu = NULL;
struct hvm_iommu *hd = domain_hvm_iommu(d);
int flush_dev_iotlb;
int iommu_domid;
+ int rc = 0;
iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
@@ -1775,11 +1797,19 @@ void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
iommu_domid= domain_iommu_domid(d, iommu);
if ( iommu_domid == -1 )
continue;
- if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
+
+ rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
(paddr_t)gfn << PAGE_SHIFT_4K,
- order, !present, flush_dev_iotlb) )
+ order, !present, flush_dev_iotlb);
+ if ( rc > 0 )
+ {
iommu_flush_write_buffer(iommu);
+ rc = 0;
+ } else if ( rc < 0 )
+ break;
}
+
+ return rc;
}
static int __init vtd_ept_page_compatible(struct iommu *iommu)
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index f22b3a5..790701e 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -26,7 +26,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
/* While VT-d specific, this must get declared in a generic header. */
int adjust_vtd_irq_affinities(void);
-void iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present);
+int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, int present);
bool_t iommu_supports_eim(void);
int iommu_enable_x2apic_IR(void);
void iommu_disable_x2apic_IR(void);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-18 14:00 ` [PATCH v2 01/11] vt-d: fix the IOMMU flush issue Quan Xu
@ 2016-04-19 6:33 ` Tian, Kevin
2016-04-19 8:33 ` Xu, Quan
2016-04-25 9:22 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:33 UTC (permalink / raw)
To: Xu, Quan, xen-devel
Cc: Keir Fraser, dario.faggioli, Wu, Feng, Jan Beulich, Andrew Cooper
> From: Xu, Quan
> Sent: Monday, April 18, 2016 10:00 PM
>
> The propagation value from IOMMU flush interfaces may be positive, which
> indicates callers need to flush cache, not one of faliures.
>
> when the propagation value is positive, this patch fixes this flush issue
> as follows:
> - call iommu_flush_write_buffer() to flush cache.
> - return zero.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> xen/drivers/passthrough/vtd/iommu.c | 94
> ++++++++++++++++++++++++-------------
> xen/include/asm-x86/iommu.h | 2 +-
> 2 files changed, 63 insertions(+), 33 deletions(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 5ad25dc..50d98ac 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> }
> }
>
> -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> - int dma_old_pte_present, unsigned int page_count)
> +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
any reason for the renaming here?
> + int dma_old_pte_present,
> + unsigned int page_count)
> {
> struct hvm_iommu *hd = domain_hvm_iommu(d);
> struct acpi_drhd_unit *drhd;
> struct iommu *iommu;
> int flush_dev_iotlb;
> int iommu_domid;
> + int rc = 0;
>
> /*
> * No need pcideves_lock here because we have flush
> @@ -584,29 +586,34 @@ static void __intel_iommu_iotlb_flush(struct domain *d,
> unsigned long gfn,
> continue;
>
> if ( page_count != 1 || gfn == INVALID_GFN )
> - {
> - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
> - 0, flush_dev_iotlb) )
> - iommu_flush_write_buffer(iommu);
> - }
> + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
> + 0, flush_dev_iotlb);
> else
> + rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
> + (paddr_t)gfn << PAGE_SHIFT_4K,
> + PAGE_ORDER_4K,
> + !dma_old_pte_present,
> + flush_dev_iotlb);
> +
> + if ( rc > 0 )
> {
> - if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
> - (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
> - !dma_old_pte_present, flush_dev_iotlb) )
> - iommu_flush_write_buffer(iommu);
> - }
> + iommu_flush_write_buffer(iommu);
> + rc = 0;
> + } else if ( rc < 0 )
> + break;
> }
> +
> + return rc;
> }
>
> static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int
> page_count)
> {
> - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> + iommu_flush_iotlb(d, gfn, 1, page_count);
> }
>
> static void intel_iommu_iotlb_flush_all(struct domain *d)
> {
> - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
> + iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> }
Do you plan to change return value of above 2 functions in another
patch, or will keep it current form w/o propagation?
>
> /* clear one page's page table */
> @@ -640,7 +647,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
> iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
>
> if ( !this_cpu(iommu_dont_flush_iotlb) )
> - __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
> + iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);
>
> unmap_vtd_domain_page(page);
> }
> @@ -1281,6 +1288,7 @@ int domain_context_mapping_one(
> u64 maddr, pgd_maddr;
> u16 seg = iommu->intel->drhd->segment;
> int agaw;
> + int rc;
>
> ASSERT(pcidevs_locked());
> spin_lock(&iommu->lock);
> @@ -1394,13 +1402,19 @@ int domain_context_mapping_one(
> spin_unlock(&iommu->lock);
>
> /* Context entry was previously non-present (with domid 0). */
> - if ( iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> - DMA_CCMD_MASK_NOBIT, 1) )
> - iommu_flush_write_buffer(iommu);
> - else
> + rc = iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> + DMA_CCMD_MASK_NOBIT, 1);
> +
> + if ( !rc )
> {
> int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> - iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> + rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> + }
> +
> + if ( rc > 0 )
> + {
> + iommu_flush_write_buffer(iommu);
> + rc = 0;
what about 'rc>0' in iommu_flush_context_device while "rc<0"
in iommu_flush_iotlb_dsi, which will leave write buffer not flushed?
> }
>
> set_bit(iommu->index, &hd->arch.iommu_bitmap);
> @@ -1410,7 +1424,7 @@ int domain_context_mapping_one(
> if ( !seg )
> me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
>
> - return 0;
> + return rc;
> }
>
> static int domain_context_mapping(
> @@ -1505,6 +1519,7 @@ int domain_context_unmap_one(
> struct context_entry *context, *context_entries;
> u64 maddr;
> int iommu_domid;
> + int rc;
>
> ASSERT(pcidevs_locked());
> spin_lock(&iommu->lock);
> @@ -1532,14 +1547,20 @@ int domain_context_unmap_one(
> return -EINVAL;
> }
>
> - if ( iommu_flush_context_device(iommu, iommu_domid,
> + rc = iommu_flush_context_device(iommu, iommu_domid,
> (((u16)bus) << 8) | devfn,
> - DMA_CCMD_MASK_NOBIT, 0) )
> - iommu_flush_write_buffer(iommu);
> - else
> + DMA_CCMD_MASK_NOBIT, 0);
> +
> + if ( !rc )
> {
> int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> - iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb);
> + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid, 0, flush_dev_iotlb);
> + }
> +
> + if ( rc > 0 )
> + {
> + iommu_flush_write_buffer(iommu);
> + rc = 0;
> }
ditto
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-19 6:33 ` Tian, Kevin
@ 2016-04-19 8:33 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-19 8:33 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Keir Fraser, dario.faggioli, Wu, Feng, Jan Beulich, Andrew Cooper
On April 19, 2016 2:33pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > The propagation value from IOMMU flush interfaces may be positive,
> > which indicates callers need to flush cache, not one of faliures.
> >
> > when the propagation value is positive, this patch fixes this flush
> > issue as follows:
> > - call iommu_flush_write_buffer() to flush cache.
> > - return zero.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > xen/drivers/passthrough/vtd/iommu.c | 94
> > ++++++++++++++++++++++++-------------
> > xen/include/asm-x86/iommu.h | 2 +-
> > 2 files changed, 63 insertions(+), 33 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 5ad25dc..50d98ac 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> > }
> > }
> >
> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> > - int dma_old_pte_present, unsigned int page_count)
> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
>
> any reason for the renaming here?
>
As Jan mentioned,
"this would be a good opportunity to also drop the stray double underscores: You need to touch all callers anyway."
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg02456.html
think twice, I dropped the stray double underscores.
both intel_iommu_iotlb_flush and iommu_iotlb_flush were already in use,
So I tried to rename it to ' iommu_flush_iotlb'.
> > + int dma_old_pte_present,
> > + unsigned int page_count)
> > {
> > struct hvm_iommu *hd = domain_hvm_iommu(d);
> > struct acpi_drhd_unit *drhd;
> > struct iommu *iommu;
> > int flush_dev_iotlb;
> > int iommu_domid;
> > + int rc = 0;
> >
> > /*
> > * No need pcideves_lock here because we have flush @@ -584,29
> > +586,34 @@ static void __intel_iommu_iotlb_flush(struct domain *d,
> > unsigned long gfn,
> > continue;
> >
> > if ( page_count != 1 || gfn == INVALID_GFN )
> > - {
> > - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
> > - 0, flush_dev_iotlb) )
> > - iommu_flush_write_buffer(iommu);
> > - }
> > + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
> > + 0, flush_dev_iotlb);
> > else
> > + rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
> > + (paddr_t)gfn <<
> PAGE_SHIFT_4K,
> > + PAGE_ORDER_4K,
> > + !dma_old_pte_present,
> > + flush_dev_iotlb);
> > +
> > + if ( rc > 0 )
> > {
> > - if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
> > - (paddr_t)gfn << PAGE_SHIFT_4K,
> PAGE_ORDER_4K,
> > - !dma_old_pte_present, flush_dev_iotlb) )
> > - iommu_flush_write_buffer(iommu);
> > - }
> > + iommu_flush_write_buffer(iommu);
> > + rc = 0;
> > + } else if ( rc < 0 )
> > + break;
> > }
> > +
> > + return rc;
> > }
> >
> > static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > gfn, unsigned int
> > page_count)
> > {
> > - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> > + iommu_flush_iotlb(d, gfn, 1, page_count);
> > }
> >
> > static void intel_iommu_iotlb_flush_all(struct domain *d) {
> > - __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
> > + iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> > }
>
> Do you plan to change return value of above 2 functions in another patch, or
> will keep it current form w/o propagation?
>
Change return value of above 2 functions in another patches.
> >
> > /* clear one page's page table */
> > @@ -640,7 +647,7 @@ static void dma_pte_clear_one(struct domain
> *domain, u64 addr)
> > iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
> >
> > if ( !this_cpu(iommu_dont_flush_iotlb) )
> > - __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
> > + iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);
> >
> > unmap_vtd_domain_page(page);
> > }
> > @@ -1281,6 +1288,7 @@ int domain_context_mapping_one(
> > u64 maddr, pgd_maddr;
> > u16 seg = iommu->intel->drhd->segment;
> > int agaw;
> > + int rc;
> >
> > ASSERT(pcidevs_locked());
> > spin_lock(&iommu->lock);
> > @@ -1394,13 +1402,19 @@ int domain_context_mapping_one(
> > spin_unlock(&iommu->lock);
> >
> > /* Context entry was previously non-present (with domid 0). */
> > - if ( iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> > - DMA_CCMD_MASK_NOBIT, 1) )
> > - iommu_flush_write_buffer(iommu);
> > - else
> > + rc = iommu_flush_context_device(iommu, 0, (((u16)bus) << 8) | devfn,
> > + DMA_CCMD_MASK_NOBIT, 1);
> > +
> > + if ( !rc )
> > {
> > int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> > - iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> > + rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> > + }
> > +
> > + if ( rc > 0 )
> > + {
> > + iommu_flush_write_buffer(iommu);
> > + rc = 0;
>
> what about 'rc>0' in iommu_flush_context_device while "rc<0"
> in iommu_flush_iotlb_dsi, which will leave write buffer not flushed?
That's true, but IMO it is impossible.
IOW, if 'rc>0' is in iommu_flush_context_device, the rc should be "rc>0" in iommu_flush_iotlb_dsi too.
As the flush_non_present_entry parameter is '1' to both, and read the same register 'iommu->cap' at the beginning of the functions.
To be honest, this modification is an aggressive approach and looks good to me, but I am open for any other suggestions.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-18 14:00 ` [PATCH v2 01/11] vt-d: fix the IOMMU flush issue Quan Xu
2016-04-19 6:33 ` Tian, Kevin
@ 2016-04-25 9:22 ` Jan Beulich
2016-04-26 1:17 ` Xu, Quan
2016-04-26 2:18 ` Xu, Quan
1 sibling, 2 replies; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 9:22 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Feng Wu
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> }
> }
>
> -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> - int dma_old_pte_present, unsigned int page_count)
> +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> + int dma_old_pte_present,
With the rename, please also make this boolean parameter bool_t.
> + unsigned int page_count)
> {
> struct hvm_iommu *hd = domain_hvm_iommu(d);
> struct acpi_drhd_unit *drhd;
> struct iommu *iommu;
> int flush_dev_iotlb;
> int iommu_domid;
> + int rc = 0;
Pointless initializer.
> @@ -584,29 +586,34 @@ static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> continue;
>
> if ( page_count != 1 || gfn == INVALID_GFN )
> - {
> - if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
> - 0, flush_dev_iotlb) )
> - iommu_flush_write_buffer(iommu);
> - }
> + rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
> + 0, flush_dev_iotlb);
> else
> + rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
> + (paddr_t)gfn << PAGE_SHIFT_4K,
> + PAGE_ORDER_4K,
> + !dma_old_pte_present,
> + flush_dev_iotlb);
> +
> + if ( rc > 0 )
> {
> - if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
> - (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
> - !dma_old_pte_present, flush_dev_iotlb) )
> - iommu_flush_write_buffer(iommu);
> - }
> + iommu_flush_write_buffer(iommu);
> + rc = 0;
> + } else if ( rc < 0 )
Coding style.
> + break;
I thought we had agreed on best effort flushing when an error
occurs. That means you shouldn't break out of the loop here, but
accumulate errors. (Breaking out of the loop would be okay if it
was conditional upon the domain having got crashed.)
(The same issue exists again near the end of the patch).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-25 9:22 ` Jan Beulich
@ 2016-04-26 1:17 ` Xu, Quan
2016-04-26 2:18 ` Xu, Quan
1 sibling, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 1:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Wu, Feng
On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>
> I thought we had agreed on best effort flushing when an error occurs. That
> means you shouldn't break out of the loop here, but accumulate errors.
> (Breaking out of the loop would be okay if it was conditional upon the domain
> having got crashed.)
>
I will follow it for coming version.
I indeed agreed to that single point, however, I wondered whether I could extend it as a pattern for this series or not.
Now I am clear.
Quan
> (The same issue exists again near the end of the patch).
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-25 9:22 ` Jan Beulich
2016-04-26 1:17 ` Xu, Quan
@ 2016-04-26 2:18 ` Xu, Quan
2016-04-26 9:10 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 2:18 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Wu, Feng
On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> > }
> > }
> >
> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> > - int dma_old_pte_present, unsigned int page_count)
> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> > + int dma_old_pte_present,
> > + unsigned int page_count)
> > {
> > struct hvm_iommu *hd = domain_hvm_iommu(d);
> > struct acpi_drhd_unit *drhd;
> > struct iommu *iommu;
> > int flush_dev_iotlb;
> > int iommu_domid;
> > + int rc = 0;
>
> Pointless initializer.
>
I am afraid not.
In case I don't initialize 'rc', the compiler prints out " error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]".
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-26 2:18 ` Xu, Quan
@ 2016-04-26 9:10 ` Jan Beulich
2016-04-26 10:15 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-26 9:10 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Feng Wu
>>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote:
> On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
>> > }
>> > }
>> >
>> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn,
>> > - int dma_old_pte_present, unsigned int page_count)
>> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
>> > + int dma_old_pte_present,
>> > + unsigned int page_count)
>> > {
>> > struct hvm_iommu *hd = domain_hvm_iommu(d);
>> > struct acpi_drhd_unit *drhd;
>> > struct iommu *iommu;
>> > int flush_dev_iotlb;
>> > int iommu_domid;
>> > + int rc = 0;
>>
>> Pointless initializer.
>
> I am afraid not.
> In case I don't initialize 'rc', the compiler prints out " error: 'rc' may
> be used uninitialized in this function [-Werror=maybe-uninitialized]".
Looking at the patch again I can't see why that would be the case.
Are you certain this isn't a result of subsequent patches, IOW did
you try this with just this one patch applied? rc gets initialized in
both the if() and the else branches, and there's no label allowing
that initialization to be bypassed...
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-26 9:10 ` Jan Beulich
@ 2016-04-26 10:15 ` Xu, Quan
2016-04-26 10:52 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 10:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Wu, Feng
On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote:
> > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> >> > }
> >> > }
> >> >
> >> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long
> gfn,
> >> > - int dma_old_pte_present, unsigned int page_count)
> >> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> >> > + int dma_old_pte_present,
> >> > + unsigned int page_count)
> >> > {
> >> > struct hvm_iommu *hd = domain_hvm_iommu(d);
> >> > struct acpi_drhd_unit *drhd;
> >> > struct iommu *iommu;
> >> > int flush_dev_iotlb;
> >> > int iommu_domid;
> >> > + int rc = 0;
> >>
> >> Pointless initializer.
> >
> > I am afraid not.
> > In case I don't initialize 'rc', the compiler prints out " error:
> > 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]".
>
> Looking at the patch again I can't see why that would be the case.
> Are you certain this isn't a result of subsequent patches, IOW did you try this
> with just this one patch applied?
Yes, I test it with this only patch again.
The same result.
> rc gets initialized in both the if() and the else
> branches, and there's no label allowing that initialization to be bypassed...
look at this function again,
>> func
iommu_flush_iotlb()
{
int rc;
for_each_drhd_unit ( drhd )
{
if ()
rc ..
else
rc ..
}
return rc;
}
<< func
The case drhd is NULL, the rc may be not initialized.
Development ENV:
Ubuntu 14.04.1,
GCC 4.8
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-26 10:15 ` Xu, Quan
@ 2016-04-26 10:52 ` Jan Beulich
2016-04-26 10:58 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-26 10:52 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Feng Wu
>>> On 26.04.16 at 12:15, <quan.xu@intel.com> wrote:
> On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote:
>> > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
>> >> > }
>> >> > }
>> >> >
>> >> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> gfn,
>> >> > - int dma_old_pte_present, unsigned int page_count)
>> >> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
>> >> > + int dma_old_pte_present,
>> >> > + unsigned int page_count)
>> >> > {
>> >> > struct hvm_iommu *hd = domain_hvm_iommu(d);
>> >> > struct acpi_drhd_unit *drhd;
>> >> > struct iommu *iommu;
>> >> > int flush_dev_iotlb;
>> >> > int iommu_domid;
>> >> > + int rc = 0;
>> >>
>> >> Pointless initializer.
>> >
>> > I am afraid not.
>> > In case I don't initialize 'rc', the compiler prints out " error:
>> > 'rc' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]".
>>
>> Looking at the patch again I can't see why that would be the case.
>> Are you certain this isn't a result of subsequent patches, IOW did you try
> this
>> with just this one patch applied?
>
> Yes, I test it with this only patch again.
>
> The same result.
>
>> rc gets initialized in both the if() and the else
>> branches, and there's no label allowing that initialization to be
> bypassed...
>
> look at this function again,
>
>>> func
> iommu_flush_iotlb()
> {
> int rc;
>
> for_each_drhd_unit ( drhd )
> {
> if ()
> rc ..
> else
> rc ..
> }
>
> return rc;
> }
Ah, I'm sorry - the patch context didn't make it obvious this is in
a loop.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 01/11] vt-d: fix the IOMMU flush issue
2016-04-26 10:52 ` Jan Beulich
@ 2016-04-26 10:58 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 10:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Wu, Feng
On April 26, 2016 6:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 26.04.16 at 12:15, <quan.xu@intel.com> wrote:
> > On April 26, 2016 5:11 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 26.04.16 at 04:18, <quan.xu@intel.com> wrote:
> >> > On April 25, 2016 5:22 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> > @@ -558,14 +558,16 @@ static void iommu_flush_all(void)
> >> >> > }
> >> >> > }
> >> >> >
> >> >> > -static void __intel_iommu_iotlb_flush(struct domain *d,
> >> >> > unsigned long
> >> gfn,
> >> >> > - int dma_old_pte_present, unsigned int page_count)
> >> >> > +static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> >> >> > + int dma_old_pte_present,
> >> >> > + unsigned int page_count)
> >> >> > {
> >> >> > struct hvm_iommu *hd = domain_hvm_iommu(d);
> >> >> > struct acpi_drhd_unit *drhd;
> >> >> > struct iommu *iommu;
> >> >> > int flush_dev_iotlb;
> >> >> > int iommu_domid;
> >> >> > + int rc = 0;
> >> >>
> >> >> Pointless initializer.
> >> >
> >> > I am afraid not.
> >> > In case I don't initialize 'rc', the compiler prints out " error:
> >> > 'rc' may be used uninitialized in this function
> > [-Werror=maybe-uninitialized]".
> >>
> >> Looking at the patch again I can't see why that would be the case.
> >> Are you certain this isn't a result of subsequent patches, IOW did
> >> you try
> > this
> >> with just this one patch applied?
> >
> > Yes, I test it with this only patch again.
> >
> > The same result.
> >
> >> rc gets initialized in both the if() and the else branches, and
> >> there's no label allowing that initialization to be
> > bypassed...
> >
> > look at this function again,
> >
> >>> func
> > iommu_flush_iotlb()
> > {
> > int rc;
> >
> > for_each_drhd_unit ( drhd )
> > {
> > if ()
> > rc ..
> > else
> > rc ..
> > }
> >
> > return rc;
> > }
>
> Ah, I'm sorry - the patch context didn't make it obvious this is in a loop.
>
Never mind. It is _not_ quite as obvious, but compiler helps us.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
2016-04-18 14:00 ` [PATCH v2 01/11] vt-d: fix the IOMMU flush issue Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:36 ` Tian, Kevin
2016-04-25 9:26 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
` (8 subsequent siblings)
10 siblings, 2 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Jan Beulich, Quan Xu
Now IOMMU mapping and unmapping failures are treated as a fatal to
the domain (with the exception of the hardware domain).
If IOMMU mapping and unmapping failed, crash the domain (with the
exception of the hardware domain) and propagate the error up to the
call trees.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/passthrough/iommu.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index b64676f..850101b 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
+ int rc;
if ( !iommu_enabled || !hd->platform_ops )
return 0;
- return hd->platform_ops->map_page(d, gfn, mfn, flags);
+ rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+
+ if ( rc && !is_hardware_domain(d) )
+ domain_crash(d);
+
+ return rc;
}
int iommu_unmap_page(struct domain *d, unsigned long gfn)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
+ int rc;
if ( !iommu_enabled || !hd->platform_ops )
return 0;
- return hd->platform_ops->unmap_page(d, gfn);
+ rc = hd->platform_ops->unmap_page(d, gfn);
+
+ if ( rc && !is_hardware_domain(d) )
+ domain_crash(d);
+
+ return rc;
}
static void iommu_free_pagetables(unsigned long unused)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-18 14:00 ` [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
@ 2016-04-19 6:36 ` Tian, Kevin
2016-04-19 6:40 ` Xu, Quan
2016-04-25 9:26 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:36 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Xu, Quan, Jan Beulich
> From: Quan Xu
> Sent: Monday, April 18, 2016 10:00 PM
>
> Now IOMMU mapping and unmapping failures are treated as a fatal to
> the domain (with the exception of the hardware domain).
'Now' is more about eixsting state, while it's actually what you want
to change towards. Better directly say "Treat IOMMU mapping...".
>
> If IOMMU mapping and unmapping failed, crash the domain (with the
> exception of the hardware domain) and propagate the error up to the
> call trees.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Jan Beulich <jbeulich@suse.com>
Otherwise looks OK to me.
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-19 6:36 ` Tian, Kevin
@ 2016-04-19 6:40 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-19 6:40 UTC (permalink / raw)
To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Jan Beulich
On April 19, 2016 2:37pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Quan Xu
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > Now IOMMU mapping and unmapping failures are treated as a fatal to the
> > domain (with the exception of the hardware domain).
>
> 'Now' is more about eixsting state, while it's actually what you want to change
> towards. Better directly say "Treat IOMMU mapping...".
>
Agreed. I'll modify it in next v3. Thanks for your review!!
Quan
> >
> > If IOMMU mapping and unmapping failed, crash the domain (with the
> > exception of the hardware domain) and propagate the error up to the
> > call trees.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Jan Beulich <jbeulich@suse.com>
>
> Otherwise looks OK to me.
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-18 14:00 ` [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
2016-04-19 6:36 ` Tian, Kevin
@ 2016-04-25 9:26 ` Jan Beulich
2016-04-27 14:26 ` Xu, Quan
1 sibling, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 9:26 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
> unsigned int flags)
> {
> struct hvm_iommu *hd = domain_hvm_iommu(d);
> + int rc;
>
> if ( !iommu_enabled || !hd->platform_ops )
> return 0;
>
> - return hd->platform_ops->map_page(d, gfn, mfn, flags);
> + rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +
> + if ( rc && !is_hardware_domain(d) )
> + domain_crash(d);
> +
> + return rc;
> }
As said before - letting this go completely silently for the hardware
domain is bad. At least the first instance of such an event needs a
message to be logged. Advanced variants where a message gets
logged once in a while if the issue re-occurs would be nice, but
aren't strictly necessary imo. And note that even logging all
occurrences would not be a security issue, but just a usability one
(but I still recommend against this).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-25 9:26 ` Jan Beulich
@ 2016-04-27 14:26 ` Xu, Quan
2016-04-27 15:02 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-27 14:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, xen-devel
On April 25, 2016 5:27 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d,
> unsigned long gfn, unsigned long mfn,
> > unsigned int flags) {
> > struct hvm_iommu *hd = domain_hvm_iommu(d);
> > + int rc;
> >
> > if ( !iommu_enabled || !hd->platform_ops )
> > return 0;
> >
> > - return hd->platform_ops->map_page(d, gfn, mfn, flags);
> > + rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> > +
> > + if ( rc && !is_hardware_domain(d) )
> > + domain_crash(d);
> > +
> > + return rc;
> > }
>
> As said before - letting this go completely silently for the hardware domain is
> bad. At least the first instance of such an event needs a message to be logged.
> Advanced variants where a message gets logged once in a while if the issue re-
> occurs would be nice, but aren't strictly necessary imo. And note that even
> logging all occurrences would not be a security issue, but just a usability one
> (but I still recommend against this).
>
IMO the IOMMU map/unmap failures are the small probability events. I think the log message wouldn't
spam log message.
I'd like modify it as following:
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -243,11 +243,24 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
+ int rc;
if ( !iommu_enabled || !hd->platform_ops )
return 0;
- return hd->platform_ops->map_page(d, gfn, mfn, flags);
+ rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
+
+ if ( rc )
+ {
+ printk(XENLOG_ERR
+ "iommu_map_page: IOMMU mapping gfn %#lx mfn %#lx failed for dom%d.",
+ gfn, mfn, d->domain_id);
+
+ if ( !is_hardware_domain(d) )
+ domain_crash(d);
+ }
+
+ return rc;
}
int iommu_unmap_page(struct domain *d, unsigned long gfn)
btw, in case I have print-out for mapping failure, I think it is no need to print out an extra message for the hardware domain.
If this is still not what you want, I'll continue to enhance it. Also I am not good at description, so I try to send out code modification directly.
Ditto for iommu_unmap_page().
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-27 14:26 ` Xu, Quan
@ 2016-04-27 15:02 ` Jan Beulich
2016-04-28 1:23 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-27 15:02 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, xen-devel
>>> On 27.04.16 at 16:26, <quan.xu@intel.com> wrote:
> On April 25, 2016 5:27 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -243,21 +243,33 @@ int iommu_map_page(struct domain *d,
>> unsigned long gfn, unsigned long mfn,
>> > unsigned int flags) {
>> > struct hvm_iommu *hd = domain_hvm_iommu(d);
>> > + int rc;
>> >
>> > if ( !iommu_enabled || !hd->platform_ops )
>> > return 0;
>> >
>> > - return hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > + rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > +
>> > + if ( rc && !is_hardware_domain(d) )
>> > + domain_crash(d);
>> > +
>> > + return rc;
>> > }
>>
>> As said before - letting this go completely silently for the hardware domain
> is
>> bad. At least the first instance of such an event needs a message to be
> logged.
>> Advanced variants where a message gets logged once in a while if the issue
> re-
>> occurs would be nice, but aren't strictly necessary imo. And note that even
>> logging all occurrences would not be a security issue, but just a usability
> one
>> (but I still recommend against this).
>>
>
> IMO the IOMMU map/unmap failures are the small probability events. I think
> the log message wouldn't
> spam log message.
That's when things work right. The case to consider is when something
goes wrong, leading to a myriad of failures. To see what went wrong
first it is necessary to not write overly many messages when many
failures occur in close succession.
> I'd like modify it as following:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -243,11 +243,24 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
> unsigned int flags)
> {
> struct hvm_iommu *hd = domain_hvm_iommu(d);
> + int rc;
>
> if ( !iommu_enabled || !hd->platform_ops )
> return 0;
>
> - return hd->platform_ops->map_page(d, gfn, mfn, flags);
> + rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
> +
> + if ( rc )
> + {
> + printk(XENLOG_ERR
> + "iommu_map_page: IOMMU mapping gfn %#lx mfn %#lx failed for dom%d.",
> + gfn, mfn, d->domain_id);
> +
> + if ( !is_hardware_domain(d) )
> + domain_crash(d);
> + }
> +
> + return rc;
> }
That's okay as long as no code path would ever trigger hundreds
of these messages before getting back to the pointer where the
now dead domain gets unscheduled. And note that this can't just
be "I don't think this can happen", but it needs to be guaranteed,
or else you introduce a security issue.
> btw, in case I have print-out for mapping failure, I think it is no need
> to print out an extra message for the hardware domain.
> If this is still not what you want, I'll continue to enhance it. Also I am
> not good at description, so I try to send out code modification directly.
I.e. I continue to think that
if ( is_hardware_domain() )
printk();
else
domain_crash();
is the better approach (and still subject to preventing spamming
the log as per the first part of my reply).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures
2016-04-27 15:02 ` Jan Beulich
@ 2016-04-28 1:23 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-28 1:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, xen-devel
On April 27, 2016 11:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.04.16 at 16:26, <quan.xu@intel.com> wrote:
> > On April 25, 2016 5:27 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> I.e. I continue to think that
>
> if ( is_hardware_domain() )
> printk();
> else
> domain_crash();
>
> is the better approach (and still subject to preventing spamming the log as per
> the first part of my reply).
>
agreed. I will follow this approach. Thanks.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
2016-04-18 14:00 ` [PATCH v2 01/11] vt-d: fix the IOMMU flush issue Quan Xu
2016-04-18 14:00 ` [PATCH v2 02/11] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:44 ` Tian, Kevin
` (2 more replies)
2016-04-18 14:00 ` [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping Quan Xu
` (7 subsequent siblings)
10 siblings, 3 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, dario.faggioli, Jun Nakajima, Quan Xu
If IOMMU mapping and unmapping failed, the domain (with the exception of
the hardware domain) is crashed, treated as a fatal error. Rollback can
be lighter weight.
For the hardware domain, we do things on a best effort basis. When rollback
is not feasible (in early initialization phase or trade-off of complexity),
at least, unmap upon maps error or then throw out error message.
IOMMU unmapping should perhaps continue despite an error, in an attempt to
do best effort cleanup.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/mm.c | 13 ++++++++-----
xen/arch/x86/mm/p2m-ept.c | 25 ++++++++++++++++++++++---
xen/arch/x86/mm/p2m-pt.c | 22 ++++++++++++++++++----
xen/arch/x86/mm/p2m.c | 11 +++++++++--
xen/drivers/passthrough/iommu.c | 14 +++++++++++++-
5 files changed, 70 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c997b53..5c4fb58 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type,
int preemptible)
{
unsigned long nx, x, y = page->u.inuse.type_info;
- int rc = 0;
+ int rc = 0, ret = 0;
ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
@@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
{
if ( (x & PGT_type_mask) == PGT_writable_page )
- iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
+ ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
else if ( type == PGT_writable_page )
- iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
- page_to_mfn(page),
- IOMMUF_readable|IOMMUF_writable);
+ ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
+ page_to_mfn(page),
+ IOMMUF_readable|IOMMUF_writable);
}
}
@@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
if ( (x & PGT_partial) && !(nx & PGT_partial) )
put_page(page);
+ if ( unlikely(ret) )
+ rc = ret;
+
return rc;
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 3cb6868..22c8d17 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
ept_entry_t *table, *ept_entry = NULL;
unsigned long gfn_remainder = gfn;
unsigned int i, target = order / EPT_TABLE_ORDER;
- int ret, rc = 0;
+ int ret, err = 0, rc = 0;
bool_t direct_mmio = (p2mt == p2m_mmio_direct);
uint8_t ipat = 0;
bool_t need_modify_vtd_table = 1;
@@ -830,10 +830,26 @@ out:
{
if ( iommu_flags )
for ( i = 0; i < (1 << order); i++ )
- iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
+ {
+ ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
+
+ if ( unlikely(ret) )
+ {
+ while (i)
+ iommu_unmap_page(d, gfn + --i);
+
+ err = ret;
+ break;
+ }
+ }
else
for ( i = 0; i < (1 << order); i++ )
- iommu_unmap_page(d, gfn + i);
+ {
+ ret = iommu_unmap_page(d, gfn + i);
+
+ if ( unlikely(ret) )
+ err = ret;
+ }
}
}
@@ -849,6 +865,9 @@ out:
if ( rc == 0 && p2m_is_hostp2m(p2m) )
p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
+ if ( unlikely(err) )
+ rc = err;
+
return rc;
}
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 3d80612..db4257a 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -498,7 +498,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
l1_pgentry_t intermediate_entry = l1e_empty();
l2_pgentry_t l2e_content;
l3_pgentry_t l3e_content;
- int rc;
+ int rc, ret;
unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
/*
* old_mfn and iommu_old_flags control possible flush/update needs on the
@@ -680,11 +680,25 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
}
else if ( iommu_pte_flags )
for ( i = 0; i < (1UL << page_order); i++ )
- iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
- iommu_pte_flags);
+ {
+ ret = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
+ iommu_pte_flags);
+ if ( unlikely(ret) )
+ {
+ while (i)
+ iommu_unmap_page(p2m->domain, gfn + --i);
+
+ rc = ret;
+ }
+ }
else
for ( i = 0; i < (1UL << page_order); i++ )
- iommu_unmap_page(p2m->domain, gfn + i);
+ {
+ ret = iommu_unmap_page(p2m->domain, gfn + i);
+
+ if ( unlikely(ret) )
+ rc = ret;
+ }
}
/*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b3fce1b..98450a4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -610,13 +610,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
mfn_t mfn_return;
p2m_type_t t;
p2m_access_t a;
+ int ret = 0, rc;
if ( !paging_mode_translate(p2m->domain) )
{
if ( need_iommu(p2m->domain) )
for ( i = 0; i < (1 << page_order); i++ )
- iommu_unmap_page(p2m->domain, mfn + i);
- return 0;
+ {
+ rc = iommu_unmap_page(p2m->domain, mfn + i);
+
+ if ( unlikely(rc) )
+ ret = rc;
+ }
+
+ return ret;
}
ASSERT(gfn_locked_by_me(p2m, gfn));
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 850101b..c351209 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -172,6 +172,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
{
struct page_info *page;
unsigned int i = 0;
+ int ret, rc = 0;
+
page_list_for_each ( page, &d->page_list )
{
unsigned long mfn = page_to_mfn(page);
@@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
((page->u.inuse.type_info & PGT_type_mask)
== PGT_writable_page) )
mapping |= IOMMUF_writable;
- hd->platform_ops->map_page(d, gfn, mfn, mapping);
+
+ ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
+
+ if ( unlikely(ret) )
+ rc = ret;
+
if ( !(i++ & 0xfffff) )
process_pending_softirqs();
}
+
+ if ( rc )
+ printk(XENLOG_G_ERR
+ "dom%d: IOMMU mapping is failed for hardware domain.",
+ d->domain_id);
}
return hd->platform_ops->hwdom_init(d);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-18 14:00 ` [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
@ 2016-04-19 6:44 ` Tian, Kevin
2016-04-20 5:27 ` Xu, Quan
2016-04-25 9:50 ` Jan Beulich
2016-04-27 15:48 ` George Dunlap
2 siblings, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:44 UTC (permalink / raw)
To: Xu, Quan, xen-devel
Cc: Keir Fraser, Jan Beulich, George Dunlap, Andrew Cooper,
dario.faggioli, Nakajima, Jun
> From: Xu, Quan
> Sent: Monday, April 18, 2016 10:00 PM
>
> If IOMMU mapping and unmapping failed, the domain (with the exception of
> the hardware domain) is crashed, treated as a fatal error. Rollback can
> be lighter weight.
What do you mean by 'lighter weight"? Please clarify it.
>
> For the hardware domain, we do things on a best effort basis. When rollback
> is not feasible (in early initialization phase or trade-off of complexity),
> at least, unmap upon maps error or then throw out error message.
remove 'or'. Based on next sentence, is above specific for IOMMU mapping?
>
> IOMMU unmapping should perhaps continue despite an error, in an attempt to
> do best effort cleanup.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> xen/arch/x86/mm.c | 13 ++++++++-----
> xen/arch/x86/mm/p2m-ept.c | 25 ++++++++++++++++++++++---
> xen/arch/x86/mm/p2m-pt.c | 22 ++++++++++++++++++----
> xen/arch/x86/mm/p2m.c | 11 +++++++++--
> xen/drivers/passthrough/iommu.c | 14 +++++++++++++-
> 5 files changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index c997b53..5c4fb58 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned
> long type,
> int preemptible)
> {
> unsigned long nx, x, y = page->u.inuse.type_info;
> - int rc = 0;
> + int rc = 0, ret = 0;
>
> ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>
> @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned
> long type,
> if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> {
> if ( (x & PGT_type_mask) == PGT_writable_page )
> - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> + ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> else if ( type == PGT_writable_page )
> - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> - page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> + page_to_mfn(page),
> + IOMMUF_readable|IOMMUF_writable);
> }
> }
>
> @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info *page, unsigned
> long type,
> if ( (x & PGT_partial) && !(nx & PGT_partial) )
> put_page(page);
>
> + if ( unlikely(ret) )
> + rc = ret;
> +
> return rc;
> }
>
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 3cb6868..22c8d17 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn,
> mfn_t mfn,
> ept_entry_t *table, *ept_entry = NULL;
> unsigned long gfn_remainder = gfn;
> unsigned int i, target = order / EPT_TABLE_ORDER;
> - int ret, rc = 0;
> + int ret, err = 0, rc = 0;
> bool_t direct_mmio = (p2mt == p2m_mmio_direct);
> uint8_t ipat = 0;
> bool_t need_modify_vtd_table = 1;
> @@ -830,10 +830,26 @@ out:
> {
> if ( iommu_flags )
> for ( i = 0; i < (1 << order); i++ )
> - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> + {
> + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> +
> + if ( unlikely(ret) )
> + {
> + while (i)
> + iommu_unmap_page(d, gfn + --i);
How about below?
while (i-- >= 0)
iommu_unmap_page(d, gfn + i);
> +
> + err = ret;
> + break;
> + }
> + }
> else
> for ( i = 0; i < (1 << order); i++ )
> - iommu_unmap_page(d, gfn + i);
> + {
> + ret = iommu_unmap_page(d, gfn + i);
> +
> + if ( unlikely(ret) )
> + err = ret;
> + }
> }
> }
>
> @@ -849,6 +865,9 @@ out:
> if ( rc == 0 && p2m_is_hostp2m(p2m) )
> p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
>
> + if ( unlikely(err) )
> + rc = err;
> +
not sure we need three return values here: rc, ret, err...
> return rc;
> }
>
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 3d80612..db4257a 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -498,7 +498,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn,
> mfn_t mfn,
> l1_pgentry_t intermediate_entry = l1e_empty();
> l2_pgentry_t l2e_content;
> l3_pgentry_t l3e_content;
> - int rc;
> + int rc, ret;
> unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> /*
> * old_mfn and iommu_old_flags control possible flush/update needs on the
> @@ -680,11 +680,25 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long
> gfn, mfn_t mfn,
> }
> else if ( iommu_pte_flags )
> for ( i = 0; i < (1UL << page_order); i++ )
> - iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> - iommu_pte_flags);
> + {
> + ret = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> + iommu_pte_flags);
> + if ( unlikely(ret) )
> + {
> + while (i)
> + iommu_unmap_page(p2m->domain, gfn + --i);
> +
> + rc = ret;
> + }
> + }
> else
> for ( i = 0; i < (1UL << page_order); i++ )
> - iommu_unmap_page(p2m->domain, gfn + i);
> + {
> + ret = iommu_unmap_page(p2m->domain, gfn + i);
> +
> + if ( unlikely(ret) )
> + rc = ret;
> + }
> }
>
> /*
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b3fce1b..98450a4 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -610,13 +610,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long
> gfn, unsigned long mfn,
> mfn_t mfn_return;
> p2m_type_t t;
> p2m_access_t a;
> + int ret = 0, rc;
>
> if ( !paging_mode_translate(p2m->domain) )
> {
> if ( need_iommu(p2m->domain) )
> for ( i = 0; i < (1 << page_order); i++ )
> - iommu_unmap_page(p2m->domain, mfn + i);
> - return 0;
> + {
> + rc = iommu_unmap_page(p2m->domain, mfn + i);
> +
> + if ( unlikely(rc) )
> + ret = rc;
> + }
> +
> + return ret;
> }
>
> ASSERT(gfn_locked_by_me(p2m, gfn));
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 850101b..c351209 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -172,6 +172,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> {
> struct page_info *page;
> unsigned int i = 0;
> + int ret, rc = 0;
> +
> page_list_for_each ( page, &d->page_list )
> {
> unsigned long mfn = page_to_mfn(page);
> @@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> ((page->u.inuse.type_info & PGT_type_mask)
> == PGT_writable_page) )
> mapping |= IOMMUF_writable;
> - hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +
> + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +
> + if ( unlikely(ret) )
> + rc = ret;
> +
> if ( !(i++ & 0xfffff) )
> process_pending_softirqs();
> }
> +
> + if ( rc )
> + printk(XENLOG_G_ERR
> + "dom%d: IOMMU mapping is failed for hardware domain.",
> + d->domain_id);
> }
>
> return hd->platform_ops->hwdom_init(d);
> --
> 1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-19 6:44 ` Tian, Kevin
@ 2016-04-20 5:27 ` Xu, Quan
2016-04-20 5:44 ` Tian, Kevin
2016-04-20 6:11 ` Jan Beulich
0 siblings, 2 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-20 5:27 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Keir Fraser, Jan Beulich, George Dunlap, Andrew Cooper,
dario.faggioli, Nakajima, Jun
On April 19, 2016 2:44pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > If IOMMU mapping and unmapping failed, the domain (with the exception
> > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > can be lighter weight.
>
> What do you mean by 'lighter weight"? Please clarify it.
>
> >
> > For the hardware domain, we do things on a best effort basis. When
> > rollback is not feasible (in early initialization phase or trade-off
> > of complexity), at least, unmap upon maps error or then throw out error
> message.
>
> remove 'or'. Based on next sentence, is above specific for IOMMU mapping?
>
> >
> > IOMMU unmapping should perhaps continue despite an error, in an
> > attempt to do best effort cleanup.
> >
Could I enhance the commit log as below:
"""
If IOMMU mapping and unmapping failed, the domain (with the exception of the hardware domain) is crashed,
treated as a fatal error. Rollback can be lighter weight (at least errors need to be propagated).
IOMMU mapping breaks for an error, unmapping upon maps, throwing out error message and then reporting
the error up the call trees. When rollback is not feasible (in early initialization phase or trade-off of complexity)
for the hardware domain, we do things on a best effort basis, only throwing out error message.
IOMMU unmapping should perhaps continue despite an error, in an attempt to do best effort cleanup.
"""
I am still not sure whether we really need throw out error message for each IOMMU mapping or not.
If yes, I will throw out error message for each IOMMU mapping in next v3.
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Keir Fraser <keir@xen.org>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
> > CC: Jun Nakajima <jun.nakajima@intel.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: George Dunlap <george.dunlap@eu.citrix.com>
> > ---
> > xen/arch/x86/mm.c | 13 ++++++++-----
> > xen/arch/x86/mm/p2m-ept.c | 25 ++++++++++++++++++++++---
> > xen/arch/x86/mm/p2m-pt.c | 22 ++++++++++++++++++----
> > xen/arch/x86/mm/p2m.c | 11 +++++++++--
> > xen/drivers/passthrough/iommu.c | 14 +++++++++++++-
> > 5 files changed, 70 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index
> > c997b53..5c4fb58 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> > int preemptible) {
> > unsigned long nx, x, y = page->u.inuse.type_info;
> > - int rc = 0;
> > + int rc = 0, ret = 0;
> >
> > ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >
> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> > {
> > if ( (x & PGT_type_mask) == PGT_writable_page )
> > - iommu_unmap_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)));
> > + ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> > else if ( type == PGT_writable_page )
> > - iommu_map_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)),
> > - page_to_mfn(page),
> > -
> IOMMUF_readable|IOMMUF_writable);
> > + ret = iommu_map_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)),
> > + page_to_mfn(page),
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> > }
> > }
> >
> > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> > if ( (x & PGT_partial) && !(nx & PGT_partial) )
> > put_page(page);
> >
> > + if ( unlikely(ret) )
> > + rc = ret;
> > +
> > return rc;
> > }
> >
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index 3cb6868..22c8d17 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned
> > long gfn, mfn_t mfn,
> > ept_entry_t *table, *ept_entry = NULL;
> > unsigned long gfn_remainder = gfn;
> > unsigned int i, target = order / EPT_TABLE_ORDER;
> > - int ret, rc = 0;
> > + int ret, err = 0, rc = 0;
> > bool_t direct_mmio = (p2mt == p2m_mmio_direct);
> > uint8_t ipat = 0;
> > bool_t need_modify_vtd_table = 1; @@ -830,10 +830,26 @@ out:
> > {
> > if ( iommu_flags )
> > for ( i = 0; i < (1 << order); i++ )
> > - iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> iommu_flags);
> > + {
> > + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > + iommu_flags);
> > +
> > + if ( unlikely(ret) )
> > + {
> > + while (i)
> > + iommu_unmap_page(d, gfn + --i);
>
> How about below?
>
> while (i-- >= 0)
> iommu_unmap_page(d, gfn + i);
>
this modification is based on discussion rooted at http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
wait for Jan's decision.
> > +
> > + err = ret;
> > + break;
> > + }
> > + }
> > else
> > for ( i = 0; i < (1 << order); i++ )
> > - iommu_unmap_page(d, gfn + i);
> > + {
> > + ret = iommu_unmap_page(d, gfn + i);
> > +
> > + if ( unlikely(ret) )
> > + err = ret;
> > + }
> > }
> > }
> >
> > @@ -849,6 +865,9 @@ out:
> > if ( rc == 0 && p2m_is_hostp2m(p2m) )
> > p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt,
> > p2ma);
> >
> > + if ( unlikely(err) )
> > + rc = err;
> > +
>
> not sure we need three return values here: rc, ret, err...
>
I am afraid that we need three return values here.
As similar as George mentioned, we need to make sure that the altp2m gets updated when the hostp2m is updated, even if the IOMMU mapping or unmapping fail.
if altp2m is enabled, rc cannot be non-zero here due to IOMMU mapping or unmapping, so I need another return value.
Quan
> > return rc;
> > }
> >
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index
> > 3d80612..db4257a 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -498,7 +498,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> unsigned
> > long gfn, mfn_t mfn,
> > l1_pgentry_t intermediate_entry = l1e_empty();
> > l2_pgentry_t l2e_content;
> > l3_pgentry_t l3e_content;
> > - int rc;
> > + int rc, ret;
> > unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> > /*
> > * old_mfn and iommu_old_flags control possible flush/update
> > needs on the @@ -680,11 +680,25 @@ p2m_pt_set_entry(struct
> p2m_domain
> > *p2m, unsigned long gfn, mfn_t mfn,
> > }
> > else if ( iommu_pte_flags )
> > for ( i = 0; i < (1UL << page_order); i++ )
> > - iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> > - iommu_pte_flags);
> > + {
> > + ret = iommu_map_page(p2m->domain, gfn + i,
> mfn_x(mfn) + i,
> > + iommu_pte_flags);
> > + if ( unlikely(ret) )
> > + {
> > + while (i)
> > + iommu_unmap_page(p2m->domain, gfn + --i);
> > +
> > + rc = ret;
> > + }
> > + }
> > else
> > for ( i = 0; i < (1UL << page_order); i++ )
> > - iommu_unmap_page(p2m->domain, gfn + i);
> > + {
> > + ret = iommu_unmap_page(p2m->domain, gfn + i);
> > +
> > + if ( unlikely(ret) )
> > + rc = ret;
> > + }
> > }
> >
> > /*
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index
> > b3fce1b..98450a4 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -610,13 +610,20 @@ p2m_remove_page(struct p2m_domain *p2m,
> unsigned
> > long gfn, unsigned long mfn,
> > mfn_t mfn_return;
> > p2m_type_t t;
> > p2m_access_t a;
> > + int ret = 0, rc;
> >
> > if ( !paging_mode_translate(p2m->domain) )
> > {
> > if ( need_iommu(p2m->domain) )
> > for ( i = 0; i < (1 << page_order); i++ )
> > - iommu_unmap_page(p2m->domain, mfn + i);
> > - return 0;
> > + {
> > + rc = iommu_unmap_page(p2m->domain, mfn + i);
> > +
> > + if ( unlikely(rc) )
> > + ret = rc;
> > + }
> > +
> > + return ret;
> > }
> >
> > ASSERT(gfn_locked_by_me(p2m, gfn)); diff --git
> > a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index 850101b..c351209 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -172,6 +172,8 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> > {
> > struct page_info *page;
> > unsigned int i = 0;
> > + int ret, rc = 0;
> > +
> > page_list_for_each ( page, &d->page_list )
> > {
> > unsigned long mfn = page_to_mfn(page); @@ -182,10
> +184,20
> > @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> > ((page->u.inuse.type_info & PGT_type_mask)
> > == PGT_writable_page) )
> > mapping |= IOMMUF_writable;
> > - hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > + if ( unlikely(ret) )
> > + rc = ret;
> > +
> > if ( !(i++ & 0xfffff) )
> > process_pending_softirqs();
> > }
> > +
> > + if ( rc )
> > + printk(XENLOG_G_ERR
> > + "dom%d: IOMMU mapping is failed for hardware
> domain.",
> > + d->domain_id);
> > }
> >
> > return hd->platform_ops->hwdom_init(d);
> > --
> > 1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-20 5:27 ` Xu, Quan
@ 2016-04-20 5:44 ` Tian, Kevin
2016-04-20 6:11 ` Jan Beulich
1 sibling, 0 replies; 78+ messages in thread
From: Tian, Kevin @ 2016-04-20 5:44 UTC (permalink / raw)
To: Xu, Quan, xen-devel
Cc: Keir Fraser, Jan Beulich, George Dunlap, Andrew Cooper,
dario.faggioli, Nakajima, Jun
> From: Xu, Quan
> Sent: Wednesday, April 20, 2016 1:27 PM
>
> On April 19, 2016 2:44pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > > From: Xu, Quan
> > > Sent: Monday, April 18, 2016 10:00 PM
> > >
> > > If IOMMU mapping and unmapping failed, the domain (with the exception
> > > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > > can be lighter weight.
> >
> > What do you mean by 'lighter weight"? Please clarify it.
> >
> > >
> > > For the hardware domain, we do things on a best effort basis. When
> > > rollback is not feasible (in early initialization phase or trade-off
> > > of complexity), at least, unmap upon maps error or then throw out error
> > message.
> >
> > remove 'or'. Based on next sentence, is above specific for IOMMU mapping?
> >
> > >
> > > IOMMU unmapping should perhaps continue despite an error, in an
> > > attempt to do best effort cleanup.
> > >
>
>
>
> Could I enhance the commit log as below:
> """
> If IOMMU mapping and unmapping failed, the domain (with the exception of the hardware
> domain) is crashed,
> treated as a fatal error. Rollback can be lighter weight (at least errors need to be
> propagated).
Still not clear about 'lighter weight'. Then what is the 'heavier weight' side then?
Isn't "at least errors need to be propagated" exactly what 'rollback' means?
>
> IOMMU mapping breaks for an error, unmapping upon maps, throwing out error message
> and then reporting
> the error up the call trees. When rollback is not feasible (in early initialization phase or
> trade-off of complexity)
> for the hardware domain, we do things on a best effort basis, only throwing out error
> message.
If above elaborates what earlier 'lighter weight" means, then please just
say a best-effort rollack.
>
> IOMMU unmapping should perhaps continue despite an error, in an attempt to do best
> effort cleanup.
> """
>
>
> I am still not sure whether we really need throw out error message for each IOMMU
> mapping or not.
> If yes, I will throw out error message for each IOMMU mapping in next v3.
I'm not sure about your question here. Have to judge based on specific case.
> > > {
> > > if ( iommu_flags )
> > > for ( i = 0; i < (1 << order); i++ )
> > > - iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > iommu_flags);
> > > + {
> > > + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > > + iommu_flags);
> > > +
> > > + if ( unlikely(ret) )
> > > + {
> > > + while (i)
> > > + iommu_unmap_page(d, gfn + --i);
> >
> > How about below?
> >
> > while (i-- >= 0)
> > iommu_unmap_page(d, gfn + i);
> >
>
> this modification is based on discussion rooted at
> http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
> wait for Jan's decision.
Either way is OK. 'gfn + --I' just looks not very readable to me.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-20 5:27 ` Xu, Quan
2016-04-20 5:44 ` Tian, Kevin
@ 2016-04-20 6:11 ` Jan Beulich
2016-04-20 6:26 ` Xu, Quan
1 sibling, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-20 6:11 UTC (permalink / raw)
To: kevin.tian, quan.xu, xen-devel
Cc: george.dunlap, andrew.cooper3, dario.faggioli, keir, jun.nakajima
>>> "Xu, Quan" <quan.xu@intel.com> 04/20/16 7:29 AM >>>
>I am still not sure whether we really need throw out error message for each IOMMU mapping or not.
>If yes, I will throw out error message for each IOMMU mapping in next v3.
Ideally not, if it's a batch that it failing, The question just is whether at the point you
issue the error message you can know another got already emitted. In no case
must this lead to spamming of the console originating from an unprivileged domain.
>> > + if ( unlikely(ret) )
>> > + {
>> > + while (i)
>> > + iommu_unmap_page(d, gfn + --i);
>>
>> How about below?
>>
>> while (i-- >= 0)
>> iommu_unmap_page(d, gfn + i);
>
>this modification is based on discussion rooted at http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
>wait for Jan's decision.
But did you really _follow_ that discussion? The adjustment done by that patch
was specifically not deemed good, so the shape Kevin suggests is in line with
the outcome of that discussion (except that I'd suggest omitting the ">= 0", the
more that i at least ought to be unsigned here).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-20 6:11 ` Jan Beulich
@ 2016-04-20 6:26 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-20 6:26 UTC (permalink / raw)
To: Jan Beulich, Tian, Kevin, xen-devel
Cc: george.dunlap, andrew.cooper3, dario.faggioli, keir, Nakajima, Jun
On April 20, 2016 2:12pm, <jbeulich@suse.com > wrote:
> >>> "Xu, Quan" <quan.xu@intel.com> 04/20/16 7:29 AM >>>
> Ideally not, if it's a batch that it failing, The question just is whether at the point
> you issue the error message you can know another got already emitted. In no
> case must this lead to spamming of the console originating from an unprivileged
> domain.
>
> >> > + if ( unlikely(ret) )
> >> > + {
> >> > + while (i)
> >> > + iommu_unmap_page(d, gfn + --i);
> >>
> >> How about below?
> >>
> >> while (i-- >= 0)
> >> iommu_unmap_page(d, gfn + i);
> >
> >this modification is based on discussion rooted at
> >http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.ht
> >ml
> >wait for Jan's decision.
>
> But did you really _follow_ that discussion? The adjustment done by that patch
> was specifically not deemed good, so the shape Kevin suggests is in line with the
> outcome of that discussion (except that I'd suggest omitting the ">= 0", the
> more that i at least ought to be unsigned here).
>
Based on your suggestions, I will fix it as below:
+while ( i-- )
+ iommu_unmap_page(d, gfn + i);
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-18 14:00 ` [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
2016-04-19 6:44 ` Tian, Kevin
@ 2016-04-25 9:50 ` Jan Beulich
2016-04-27 8:49 ` Xu, Quan
2016-04-27 15:48 ` George Dunlap
2 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 9:50 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> int preemptible)
> {
> unsigned long nx, x, y = page->u.inuse.type_info;
> - int rc = 0;
> + int rc = 0, ret = 0;
>
> ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>
> @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> {
> if ( (x & PGT_type_mask) == PGT_writable_page )
> - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> + ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> else if ( type == PGT_writable_page )
> - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> - page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> + page_to_mfn(page),
> + IOMMUF_readable|IOMMUF_writable);
> }
> }
>
> @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> if ( (x & PGT_partial) && !(nx & PGT_partial) )
> put_page(page);
>
> + if ( unlikely(ret) )
> + rc = ret;
Please don't overwrite the more relevant "rc" value in situations like
this in case that is already non-zero. In this specific case you can
actually get away without introducing a second error code variable,
since the only place rc gets altered is between the two hunks above,
and overwriting the rc value from map/unmap is then exactly what
we want (but I'd much appreciate if you added a comment to this
effect).
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> ept_entry_t *table, *ept_entry = NULL;
> unsigned long gfn_remainder = gfn;
> unsigned int i, target = order / EPT_TABLE_ORDER;
> - int ret, rc = 0;
> + int ret, err = 0, rc = 0;
Just like Kevin, and despite your reply to him I do not see the need
fir the 3rd error indicator variable here:
> @@ -830,10 +830,26 @@ out:
> {
> if ( iommu_flags )
> for ( i = 0; i < (1 << order); i++ )
> - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> + {
> + ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> +
> + if ( unlikely(ret) )
> + {
> + while (i)
while ( i-- )
> + iommu_unmap_page(d, gfn + --i);
> +
> + err = ret;
> + break;
> + }
> + }
> else
> for ( i = 0; i < (1 << order); i++ )
> - iommu_unmap_page(d, gfn + i);
> + {
> + ret = iommu_unmap_page(d, gfn + i);
> +
> + if ( unlikely(ret) )
> + err = ret;
> + }
> }
You can simply utilize that rc is zero upon entering this if(), clearing
it back to zero here.
> @@ -849,6 +865,9 @@ out:
> if ( rc == 0 && p2m_is_hostp2m(p2m) )
> p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
>
> + if ( unlikely(err) )
> + rc = err;
Same comment as for mm.c (and also for mm/p2m-pt.c and
mm/p2m.c below, albeit the impact there is limited - the only
difference is whether to return first or last error encountered, but
I'd still prefer the first one to be used, as for a crashed domain later
errors may end up being kind of meaningless wrt what caused the
domain to get crashed).
> @@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> ((page->u.inuse.type_info & PGT_type_mask)
> == PGT_writable_page) )
> mapping |= IOMMUF_writable;
> - hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +
> + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +
> + if ( unlikely(ret) )
> + rc = ret;
> +
> if ( !(i++ & 0xfffff) )
> process_pending_softirqs();
> }
> +
> + if ( rc )
> + printk(XENLOG_G_ERR
> + "dom%d: IOMMU mapping is failed for hardware domain.",
> + d->domain_id);
Leaving the admin / developer with no indication of at least one
specific case of a page where a failure occurred. As said in various
places before: Log messages should provide useful information for
at least getting started at investigating the issue, without first of
all having to further instrument the code.
In any event the "is" should be dropped from the text.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-25 9:50 ` Jan Beulich
@ 2016-04-27 8:49 ` Xu, Quan
2016-04-27 9:36 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-27 8:49 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Nakajima, Jun
On April 25, 2016 5:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> > int preemptible) {
> > unsigned long nx, x, y = page->u.inuse.type_info;
> > - int rc = 0;
> > + int rc = 0, ret = 0;
> >
> > ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >
> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> > {
> > if ( (x & PGT_type_mask) == PGT_writable_page )
> > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> > + ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> > else if ( type == PGT_writable_page )
> > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > - page_to_mfn(page),
> > - IOMMUF_readable|IOMMUF_writable);
> > + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > + page_to_mfn(page),
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> > }
> > }
> >
> > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> > if ( (x & PGT_partial) && !(nx & PGT_partial) )
> > put_page(page);
> >
> > + if ( unlikely(ret) )
> > + rc = ret;
>
> Please don't overwrite the more relevant "rc" value in situations like this in
> case that is already non-zero. In this specific case you can actually get away
> without introducing a second error code variable, since the only place rc gets
> altered is between the two hunks above, and overwriting the rc value from
> map/unmap is then exactly what we want (but I'd much appreciate if you
> added a comment to this effect).
>
Make sense.
I hope I have got your point.. then, I will modify it as below:
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
{
if ( (x & PGT_type_mask) == PGT_writable_page )
- iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
+ rc = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
else if ( type == PGT_writable_page )
- iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
- page_to_mfn(page),
- IOMMUF_readable|IOMMUF_writable);
+ rc = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
+ page_to_mfn(page),
+ IOMMUF_readable|IOMMUF_writable);
}
}
@@ -2593,6 +2593,8 @@ static int __get_page_type(struct page_info *page, unsigned long type,
page->nr_validated_ptes = 0;
page->partial_pte = 0;
}
+
+ /* Overwrite the rc value from IOMMU map and unmap. */
rc = alloc_page_type(page, type, preemptible);
}
btw, I am clumsy at comment, I'd be pleased if you could help me enhance it.
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned
> long gfn, mfn_t mfn,
> > ept_entry_t *table, *ept_entry = NULL;
> > unsigned long gfn_remainder = gfn;
> > unsigned int i, target = order / EPT_TABLE_ORDER;
> > - int ret, rc = 0;
> > + int ret, err = 0, rc = 0;
>
> Just like Kevin, and despite your reply to him I do not see the need fir the 3rd
> error indicator variable here:
>
Agreed.
> > @@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> > ((page->u.inuse.type_info & PGT_type_mask)
> > == PGT_writable_page) )
> > mapping |= IOMMUF_writable;
> > - hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > + if ( unlikely(ret) )
> > + rc = ret;
> > +
> > if ( !(i++ & 0xfffff) )
> > process_pending_softirqs();
> > }
> > +
> > + if ( rc )
> > + printk(XENLOG_G_ERR
> > + "dom%d: IOMMU mapping is failed for hardware domain.",
> > + d->domain_id);
>
> Leaving the admin / developer with no indication of at least one specific case
> of a page where a failure occurred. As said in various places before: Log
> messages should provide useful information for at least getting started at
> investigating the issue, without first of all having to further instrument the
> code.
>
> In any event the "is" should be dropped from the text.
>
What about:
+ if ( rc )
+ printk(XENLOG_WARNING
+ "iommu_hwdom_init: IOMMU mapping failed for dom%d.",
+ d->domain_id);
If this is still not what you want, could you help me enhance it and I can
follow it as a pattern. Sorry to bother you with such a case.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-27 8:49 ` Xu, Quan
@ 2016-04-27 9:36 ` Jan Beulich
2016-04-27 13:10 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-27 9:36 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 27.04.16 at 10:49, <quan.xu@intel.com> wrote:
> On April 25, 2016 5:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info
>> *page, unsigned long type,
>> > int preemptible) {
>> > unsigned long nx, x, y = page->u.inuse.type_info;
>> > - int rc = 0;
>> > + int rc = 0, ret = 0;
>> >
>> > ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>> >
>> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
>> *page, unsigned long type,
>> > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>> > {
>> > if ( (x & PGT_type_mask) == PGT_writable_page )
>> > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
>> > + ret = iommu_unmap_page(d, mfn_to_gmfn(d,
>> > + page_to_mfn(page)));
>> > else if ( type == PGT_writable_page )
>> > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
>> > - page_to_mfn(page),
>> > - IOMMUF_readable|IOMMUF_writable);
>> > + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
>> > + page_to_mfn(page),
>> > +
>> > + IOMMUF_readable|IOMMUF_writable);
>> > }
>> > }
>> >
>> > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info
>> *page, unsigned long type,
>> > if ( (x & PGT_partial) && !(nx & PGT_partial) )
>> > put_page(page);
>> >
>> > + if ( unlikely(ret) )
>> > + rc = ret;
>>
>> Please don't overwrite the more relevant "rc" value in situations like this
> in
>> case that is already non-zero. In this specific case you can actually get
> away
>> without introducing a second error code variable, since the only place rc
> gets
>> altered is between the two hunks above, and overwriting the rc value from
>> map/unmap is then exactly what we want (but I'd much appreciate if you
>> added a comment to this effect).
>>
>
>
> Make sense.
> I hope I have got your point.. then, I will modify it as below:
But I screwed it up. Looking at it as you have it:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page,
> unsigned long type,
> if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> {
> if ( (x & PGT_type_mask) == PGT_writable_page )
> - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> + rc = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> else if ( type == PGT_writable_page )
> - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> - page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + rc = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> + page_to_mfn(page),
> + IOMMUF_readable|IOMMUF_writable);
> }
> }
>
> @@ -2593,6 +2593,8 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> page->nr_validated_ptes = 0;
> page->partial_pte = 0;
> }
> +
> + /* Overwrite the rc value from IOMMU map and unmap. */
> rc = alloc_page_type(page, type, preemptible);
> }
... it is clear that this is wrong: You'd now also discard an error from
the map/unmap with possible success coming back here. I.e. you
want to overwrite the earlier rc only when you get a non-zero value
back here.
> btw, I am clumsy at comment, I'd be pleased if you could help me enhance it.
With the above, the comment is not needed anymore.
> What about:
>
> + if ( rc )
> + printk(XENLOG_WARNING
> + "iommu_hwdom_init: IOMMU mapping failed for dom%d.",
> + d->domain_id);
>
> If this is still not what you want, could you help me enhance it and I can
> follow it as a pattern. Sorry to bother you with such a case.
Looks okay.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-27 9:36 ` Jan Beulich
@ 2016-04-27 13:10 ` Xu, Quan
2016-04-27 14:06 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-27 13:10 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Nakajima, Jun
On April 27, 2016 5:37 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.04.16 at 10:49, <quan.xu@intel.com> wrote:
> > On April 25, 2016 5:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> > --- a/xen/arch/x86/mm.c
> >> > +++ b/xen/arch/x86/mm.c
> >> > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info
> >> *page, unsigned long type,
> >> > int preemptible) {
> >> > unsigned long nx, x, y = page->u.inuse.type_info;
> >> > - int rc = 0;
> >> > + int rc = 0, ret = 0;
> >> >
> >> > ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >> >
> >> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
> >> *page, unsigned long type,
> >> > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >> > {
> >> > if ( (x & PGT_type_mask) == PGT_writable_page )
> >> > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> >> > + ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> >> > + page_to_mfn(page)));
> >> > else if ( type == PGT_writable_page )
> >> > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> >> > - page_to_mfn(page),
> >> > - IOMMUF_readable|IOMMUF_writable);
> >> > + ret = iommu_map_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)),
> >> > + page_to_mfn(page),
> >> > +
> >> > + IOMMUF_readable|IOMMUF_writable);
> >> > }
> >> > }
> >> >
> >> > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info
> >> *page, unsigned long type,
> >> > if ( (x & PGT_partial) && !(nx & PGT_partial) )
> >> > put_page(page);
> >> >
> >> > + if ( unlikely(ret) )
> >> > + rc = ret;
> >>
> >> Please don't overwrite the more relevant "rc" value in situations
> >> like this
> > in
> >> case that is already non-zero. In this specific case you can actually
> >> get
> > away
> >> without introducing a second error code variable, since the only
> >> place rc
> > gets
> >> altered is between the two hunks above, and overwriting the rc value
> >> from map/unmap is then exactly what we want (but I'd much appreciate
> >> if you added a comment to this effect).
> >>
> >
> >
> > Make sense.
> > I hope I have got your point.. then, I will modify it as below:
>
> But I screwed it up. Looking at it as you have it:
>
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> > {
> > if ( (x & PGT_type_mask) == PGT_writable_page )
> > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> > + rc = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> > else if ( type == PGT_writable_page )
> > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > - page_to_mfn(page),
> > - IOMMUF_readable|IOMMUF_writable);
> > + rc = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > + page_to_mfn(page),
> > + IOMMUF_readable|IOMMUF_writable);
> > }
> > }
> >
> > @@ -2593,6 +2593,8 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> > page->nr_validated_ptes = 0;
> > page->partial_pte = 0;
> > }
> > +
> > + /* Overwrite the rc value from IOMMU map and unmap. */
> > rc = alloc_page_type(page, type, preemptible);
> > }
>
> ... it is clear that this is wrong: You'd now also discard an error from the
> map/unmap with possible success coming back here. I.e. you want to
> overwrite the earlier rc only when you get a non-zero value back here.
>
A little bit confused.
I need to introduce a second error code variable for this point, right?
Also I shouldn't overwrite the relevant 'rc' value in case that is already non-zero.
I want to send out my modification for this point, making sure we are on the same page, even I may be still wrong.
Then this code is:
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type,
int preemptible)
{
unsigned long nx, x, y = page->u.inuse.type_info;
- int rc = 0;
+ int rc = 0, ret = 0;
ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
@@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
{
if ( (x & PGT_type_mask) == PGT_writable_page )
- iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
+ ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
else if ( type == PGT_writable_page )
- iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
- page_to_mfn(page),
- IOMMUF_readable|IOMMUF_writable);
+ ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
+ page_to_mfn(page),
+ IOMMUF_readable|IOMMUF_writable);
}
}
@@ -2594,6 +2594,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
page->partial_pte = 0;
}
rc = alloc_page_type(page, type, preemptible);
+
+ if ( !rc )
+ rc = ret;
}
if ( (x & PGT_partial) && !(nx & PGT_partial) )
> > btw, I am clumsy at comment, I'd be pleased if you could help me enhance it.
>
> With the above, the comment is not needed anymore.
>
I will drop this comment.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-27 13:10 ` Xu, Quan
@ 2016-04-27 14:06 ` Jan Beulich
0 siblings, 0 replies; 78+ messages in thread
From: Jan Beulich @ 2016-04-27 14:06 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 27.04.16 at 15:10, <quan.xu@intel.com> wrote:
> Then this code is:
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> int preemptible)
> {
> unsigned long nx, x, y = page->u.inuse.type_info;
> - int rc = 0;
> + int rc = 0, ret = 0;
>
> ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>
> @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> {
> if ( (x & PGT_type_mask) == PGT_writable_page )
> - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> + ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> else if ( type == PGT_writable_page )
> - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> - page_to_mfn(page),
> - IOMMUF_readable|IOMMUF_writable);
> + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> + page_to_mfn(page),
> + IOMMUF_readable|IOMMUF_writable);
> }
> }
>
> @@ -2594,6 +2594,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
> page->partial_pte = 0;
> }
> rc = alloc_page_type(page, type, preemptible);
> +
> + if ( !rc )
> + rc = ret;
> }
>
> if ( (x & PGT_partial) && !(nx & PGT_partial) )
Looks okay now.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-18 14:00 ` [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
2016-04-19 6:44 ` Tian, Kevin
2016-04-25 9:50 ` Jan Beulich
@ 2016-04-27 15:48 ` George Dunlap
2016-04-28 1:18 ` Xu, Quan
2 siblings, 1 reply; 78+ messages in thread
From: George Dunlap @ 2016-04-27 15:48 UTC (permalink / raw)
To: Quan Xu, xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, dario.faggioli, Jun Nakajima
On 18/04/16 15:00, Quan Xu wrote:
> If IOMMU mapping and unmapping failed, the domain (with the exception of
> the hardware domain) is crashed, treated as a fatal error. Rollback can
> be lighter weight.
>
> For the hardware domain, we do things on a best effort basis. When rollback
> is not feasible (in early initialization phase or trade-off of complexity),
> at least, unmap upon maps error or then throw out error message.
>
> IOMMU unmapping should perhaps continue despite an error, in an attempt to
> do best effort cleanup.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
FWIW, looking through this I think Jan and Kevin covered everything I
would have brought up.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-04-27 15:48 ` George Dunlap
@ 2016-04-28 1:18 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-28 1:18 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: Tian, Kevin, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, dario.faggioli, Nakajima, Jun
On April 27, 2016 11:48 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> On 18/04/16 15:00, Quan Xu wrote:
> > If IOMMU mapping and unmapping failed, the domain (with the exception
> > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > can be lighter weight.
> >
> > For the hardware domain, we do things on a best effort basis. When
> > rollback is not feasible (in early initialization phase or trade-off
> > of complexity), at least, unmap upon maps error or then throw out error
> message.
> >
> > IOMMU unmapping should perhaps continue despite an error, in an
> > attempt to do best effort cleanup.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> FWIW, looking through this I think Jan and Kevin covered everything I would
> have brought up.
>
George, thanks for your attention. Your comments as always are greatly appreciated too. :)
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (2 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:46 ` Tian, Kevin
2016-04-25 9:55 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
` (6 subsequent siblings)
10 siblings, 2 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Quan Xu, Tim Deegan, dario.faggioli, Ian Jackson,
Jan Beulich
While grant table is unmapping, the domain (with the exception of the
hardware domain) may be crashed due to IOMMU mapping and unmapping
failures, and then it is unnecessary to flush specified CPUs' TLBs.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Keir Fraser <keir@xen.org>
CC: Tim Deegan <tim@xen.org>
---
xen/common/grant_table.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 8b22299..1801fe9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref(
return 0;
-fault:
- gnttab_flush_tlb(current->domain);
+ fault:
+ if ( current->domain->is_shut_down )
+ gnttab_flush_tlb(current->domain);
for ( i = 0; i < partial_done; i++ )
__gnttab_unmap_common_complete(&(common[i]));
@@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace(
return 0;
-fault:
- gnttab_flush_tlb(current->domain);
+ fault:
+ if ( current->domain->is_shut_down )
+ gnttab_flush_tlb(current->domain);
for ( i = 0; i < partial_done; i++ )
__gnttab_unmap_common_complete(&(common[i]));
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-18 14:00 ` [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping Quan Xu
@ 2016-04-19 6:46 ` Tian, Kevin
2016-04-19 13:27 ` Xu, Quan
2016-04-25 9:55 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Xu, Quan, Ian Jackson, dario.faggioli, Tim Deegan,
Jan Beulich
> From: Quan Xu
> Sent: Monday, April 18, 2016 10:00 PM
>
> While grant table is unmapping, the domain (with the exception of the
unmapping -> unmapped.
> hardware domain) may be crashed due to IOMMU mapping and unmapping
> failures, and then it is unnecessary to flush specified CPUs' TLBs.
Above description is not complete. You said "with the exception of
the hardware domain". Then people will ask whether 'unnecessary'
is also true for hardware domain (if not restriction for hardware
domain to invoke those interfaces).
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Tim Deegan <tim@xen.org>
> ---
> xen/common/grant_table.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 8b22299..1801fe9 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref(
>
> return 0;
>
> -fault:
> - gnttab_flush_tlb(current->domain);
> + fault:
> + if ( current->domain->is_shut_down )
> + gnttab_flush_tlb(current->domain);
>
> for ( i = 0; i < partial_done; i++ )
> __gnttab_unmap_common_complete(&(common[i]));
> @@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace(
>
> return 0;
>
> -fault:
> - gnttab_flush_tlb(current->domain);
> + fault:
> + if ( current->domain->is_shut_down )
> + gnttab_flush_tlb(current->domain);
>
> for ( i = 0; i < partial_done; i++ )
> __gnttab_unmap_common_complete(&(common[i]));
> --
> 1.9.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-19 6:46 ` Tian, Kevin
@ 2016-04-19 13:27 ` Xu, Quan
2016-04-20 5:35 ` Tian, Kevin
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-19 13:27 UTC (permalink / raw)
To: Tian, Kevin, xen-devel
Cc: Tim Deegan, dario.faggioli, Keir Fraser, Ian Jackson, Jan Beulich
On April 19, 2016 2:46pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Quan Xu
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > While grant table is unmapping, the domain (with the exception of the
>
> unmapping -> unmapped.
>
A slightly different take, IMO the hypercall is not returned, so it is DOING.
> > hardware domain) may be crashed due to IOMMU mapping and unmapping
> > failures, and then it is unnecessary to flush specified CPUs' TLBs.
>
> Above description is not complete. You said "with the exception of the
> hardware domain". Then people will ask whether 'unnecessary'
> is also true for hardware domain (if not restriction for hardware domain to
> invoke those interfaces).
>
Make sense.
Could I only drop '(with the exception of the hardware domain)'?
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Keir Fraser <keir@xen.org>
> > CC: Tim Deegan <tim@xen.org>
> > ---
> > xen/common/grant_table.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index
> > 8b22299..1801fe9 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref(
> >
> > return 0;
> >
> > -fault:
> > - gnttab_flush_tlb(current->domain);
> > + fault:
> > + if ( current->domain->is_shut_down )
> > + gnttab_flush_tlb(current->domain);
> >
> > for ( i = 0; i < partial_done; i++ )
> > __gnttab_unmap_common_complete(&(common[i]));
> > @@ -1429,8 +1430,9 @@ gnttab_unmap_and_replace(
> >
> > return 0;
> >
> > -fault:
> > - gnttab_flush_tlb(current->domain);
> > + fault:
> > + if ( current->domain->is_shut_down )
> > + gnttab_flush_tlb(current->domain);
> >
> > for ( i = 0; i < partial_done; i++ )
> > __gnttab_unmap_common_complete(&(common[i]));
> > --
> > 1.9.1
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-19 13:27 ` Xu, Quan
@ 2016-04-20 5:35 ` Tian, Kevin
0 siblings, 0 replies; 78+ messages in thread
From: Tian, Kevin @ 2016-04-20 5:35 UTC (permalink / raw)
To: Xu, Quan, xen-devel
Cc: Tim Deegan, dario.faggioli, Keir Fraser, Ian Jackson, Jan Beulich
> From: Xu, Quan
> Sent: Tuesday, April 19, 2016 9:28 PM
>
> On April 19, 2016 2:46pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > > From: Quan Xu
> > > Sent: Monday, April 18, 2016 10:00 PM
> > >
> > > While grant table is unmapping, the domain (with the exception of the
> >
> > unmapping -> unmapped.
> >
>
> A slightly different take, IMO the hypercall is not returned, so it is DOING.
More accurately you might want to say "When a granted page is being
unmapped".
>
>
> > > hardware domain) may be crashed due to IOMMU mapping and unmapping
> > > failures, and then it is unnecessary to flush specified CPUs' TLBs.
> >
> > Above description is not complete. You said "with the exception of the
> > hardware domain". Then people will ask whether 'unnecessary'
> > is also true for hardware domain (if not restriction for hardware domain to
> > invoke those interfaces).
> >
>
> Make sense.
> Could I only drop '(with the exception of the hardware domain)'?
>
Still didn't get the rationale here. After another look at the patch,
actually I'm even not sure how the change is related to the purpose
of this patch series. Also the change is opposite to your comment -
you limit flush to crashed domain instead:
> > -fault:
> > - gnttab_flush_tlb(current->domain);
> > + fault:
> > + if ( current->domain->is_shut_down )
> > + gnttab_flush_tlb(current->domain);
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-18 14:00 ` [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping Quan Xu
2016-04-19 6:46 ` Tian, Kevin
@ 2016-04-25 9:55 ` Jan Beulich
2016-04-26 6:48 ` Xu, Quan
1 sibling, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 9:55 UTC (permalink / raw)
To: Quan Xu; +Cc: Keir Fraser, dario.faggioli, Tim Deegan, Ian Jackson, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref(
>
> return 0;
>
> -fault:
> - gnttab_flush_tlb(current->domain);
> + fault:
> + if ( current->domain->is_shut_down )
> + gnttab_flush_tlb(current->domain);
First of all: Is avoiding this really all that useful? You don't avoid
further work in any of the earlier patches. I.e. not the least for
consistency reasons I'd suggest dropping this patch.
And then, besides the condition being inverted as Kevin has already
pointed out, I think you mean ->is_shutting_down, not
->is_shut_down.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping
2016-04-25 9:55 ` Jan Beulich
@ 2016-04-26 6:48 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 6:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, dario.faggioli, Tim Deegan, Ian Jackson, xen-devel
On April 25, 2016 5:56 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1366,8 +1366,9 @@ gnttab_unmap_grant_ref(
> >
> > return 0;
> >
> > -fault:
> > - gnttab_flush_tlb(current->domain);
> > + fault:
> > + if ( current->domain->is_shut_down )
> > + gnttab_flush_tlb(current->domain);
>
> First of all: Is avoiding this really all that useful? You don't avoid further work
> in any of the earlier patches. I.e. not the least for consistency reasons I'd
> suggest dropping this patch.
>
> And then, besides the condition being inverted as Kevin has already pointed
> out, I think you mean ->is_shutting_down, not
> ->is_shut_down.
>
I will drop this patch in next v3.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (3 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 04/11] grant_table: avoid unnecessary work during grant table unmapping Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:51 ` Tian, Kevin
2016-04-25 10:06 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 06/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
` (5 subsequent siblings)
10 siblings, 2 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Jan Beulich, Feng Wu, Kevin Tian, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/passthrough/vtd/iommu.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 50d98ac..580a9cf 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -617,11 +617,12 @@ static void intel_iommu_iotlb_flush_all(struct domain *d)
}
/* clear one page's page table */
-static void dma_pte_clear_one(struct domain *domain, u64 addr)
+static int dma_pte_clear_one(struct domain *domain, u64 addr)
{
struct hvm_iommu *hd = domain_hvm_iommu(domain);
struct dma_pte *page = NULL, *pte = NULL;
u64 pg_maddr;
+ int rc = 0;
spin_lock(&hd->arch.mapping_lock);
/* get last level pte */
@@ -629,7 +630,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
if ( pg_maddr == 0 )
{
spin_unlock(&hd->arch.mapping_lock);
- return;
+ return 0;
}
page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
@@ -639,7 +640,7 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
{
spin_unlock(&hd->arch.mapping_lock);
unmap_vtd_domain_page(page);
- return;
+ return 0;
}
dma_clear_pte(*pte);
@@ -647,9 +648,11 @@ static void dma_pte_clear_one(struct domain *domain, u64 addr)
iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
if ( !this_cpu(iommu_dont_flush_iotlb) )
- iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);
+ rc = iommu_flush_iotlb(domain, addr >> PAGE_SHIFT_4K, 1, 1);
unmap_vtd_domain_page(page);
+
+ return rc;
}
static void iommu_free_pagetable(u64 pt_maddr, int level)
@@ -1770,9 +1773,7 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
if ( iommu_passthrough && is_hardware_domain(d) )
return 0;
- dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
-
- return 0;
+ return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
}
int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-04-18 14:00 ` [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
@ 2016-04-19 6:51 ` Tian, Kevin
2016-04-19 7:01 ` Xu, Quan
2016-04-25 10:06 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:51 UTC (permalink / raw)
To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, Jan Beulich
> From: Xu, Quan
> Sent: Monday, April 18, 2016 10:00 PM
>
> While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> However the existing panic() is going to be eliminated, so we must
> propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
Thought you don't need replicate the same reason in every patch.
clear enough to just keep last line.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-04-19 6:51 ` Tian, Kevin
@ 2016-04-19 7:01 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-19 7:01 UTC (permalink / raw)
To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, Jan Beulich
On April 19, 2016 2:51pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> > However the existing panic() is going to be eliminated, so we must
> > propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
>
> Thought you don't need replicate the same reason in every patch.
> clear enough to just keep last line.
>
That's good. I also hesitated to add this same reason. I'll drop it in next v3.
Quan
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > CC: Jan Beulich <jbeulich@suse.com>
>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-04-18 14:00 ` [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
2016-04-19 6:51 ` Tian, Kevin
@ 2016-04-25 10:06 ` Jan Beulich
2016-04-26 6:49 ` Xu, Quan
1 sibling, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 10:06 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -617,11 +617,12 @@ static void intel_iommu_iotlb_flush_all(struct domain
> *d)
> }
>
> /* clear one page's page table */
> -static void dma_pte_clear_one(struct domain *domain, u64 addr)
> +static int dma_pte_clear_one(struct domain *domain, u64 addr)
This, at the latest in this series, is where __must_check should
come into play.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-04-25 10:06 ` Jan Beulich
@ 2016-04-26 6:49 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 6:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On April 25, 2016 6:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -617,11 +617,12 @@ static void intel_iommu_iotlb_flush_all(struct
> > domain
> > *d)
> > }
> >
> > /* clear one page's page table */
> > -static void dma_pte_clear_one(struct domain *domain, u64 addr)
> > +static int dma_pte_clear_one(struct domain *domain, u64 addr)
>
> This, at the latest in this series, is where __must_check should come into play.
>
Agreed.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 06/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping.
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (4 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 05/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:53 ` Tian, Kevin
2016-04-18 14:00 ` [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
` (4 subsequent siblings)
10 siblings, 1 reply; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Jan Beulich, Feng Wu, Kevin Tian, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to IOMMU mapping.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/drivers/passthrough/vtd/iommu.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 580a9cf..5cc70ca 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1720,6 +1720,7 @@ static int intel_iommu_map_page(
struct hvm_iommu *hd = domain_hvm_iommu(d);
struct dma_pte *page = NULL, *pte = NULL, old, new = { 0 };
u64 pg_maddr;
+ int rc = 0;
/* Do nothing if VT-d shares EPT page table */
if ( iommu_use_hap_pt(d) )
@@ -1762,9 +1763,9 @@ static int intel_iommu_map_page(
unmap_vtd_domain_page(page);
if ( !this_cpu(iommu_dont_flush_iotlb) )
- iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
+ rc = iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
- return 0;
+ return rc;
}
static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 06/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping.
2016-04-18 14:00 ` [PATCH v2 06/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
@ 2016-04-19 6:53 ` Tian, Kevin
0 siblings, 0 replies; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:53 UTC (permalink / raw)
To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, Jan Beulich
> From: Xu, Quan
> Sent: Monday, April 18, 2016 10:00 PM
>
> While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> However the existing panic() is going to be eliminated, so we must
> propagate the IOMMU Device-TLB flush error up to IOMMU mapping.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> CC: Jan Beulich <jbeulich@suse.com>
same comment on above message. for code itself:
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (5 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 06/11] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-25 10:19 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
` (3 subsequent siblings)
10 siblings, 1 reply; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel
Cc: dario.faggioli, Jan Beulich, Stefano Stabellini, Julien Grall, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}.
If IOMMU mapping and unmapping failed, the domain (with the exception of
the hardware domain) is crashed, treated as a fatal error. Rollback can
be lighter weight.
This patch fixes the top level ones (this patch doesn't fix the leaf
ones but the next patch does).
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
---
xen/arch/arm/p2m.c | 5 ++++-
xen/common/memory.c | 5 +++--
xen/drivers/passthrough/iommu.c | 12 ++++++++----
xen/drivers/passthrough/x86/iommu.c | 5 +++--
xen/include/xen/iommu.h | 4 ++--
5 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index a2a9c4b..11d2c02 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1177,7 +1177,10 @@ out:
if ( flush )
{
flush_tlb_domain(d);
- iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+ ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+
+ if ( ret )
+ rc = ret;
}
while ( (pg = page_list_remove_head(&free_pages)) )
diff --git a/xen/common/memory.c b/xen/common/memory.c
index c7fca96..965bd51 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain *d,
if ( need_iommu(d) )
{
this_cpu(iommu_dont_flush_iotlb) = 0;
- iommu_iotlb_flush(d, xatp->idx - done, done);
- iommu_iotlb_flush(d, xatp->gpfn - done, done);
+ rc = iommu_iotlb_flush(d, xatp->idx - done, done);
+ if ( !rc )
+ rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
}
#endif
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index c351209..9254760 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -301,24 +301,28 @@ static void iommu_free_pagetables(unsigned long unused)
cpumask_cycle(smp_processor_id(), &cpu_online_map));
}
-void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
+int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
- return;
+ return 0;
hd->platform_ops->iotlb_flush(d, gfn, page_count);
+
+ return 0;
}
-void iommu_iotlb_flush_all(struct domain *d)
+int iommu_iotlb_flush_all(struct domain *d)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
- return;
+ return 0;
hd->platform_ops->iotlb_flush_all(d);
+
+ return 0;
}
int __init iommu_setup(void)
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 8cbb655..c2c1ee3 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -104,8 +104,9 @@ int arch_iommu_populate_page_table(struct domain *d)
this_cpu(iommu_dont_flush_iotlb) = 0;
if ( !rc )
- iommu_iotlb_flush_all(d);
- else if ( rc != -ERESTART )
+ rc = iommu_iotlb_flush_all(d);
+
+ if ( rc && rc != -ERESTART )
iommu_teardown(d);
return rc;
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 8217cb7..ff4608d 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
int iommu_do_domctl(struct xen_domctl *, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
-void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count);
-void iommu_iotlb_flush_all(struct domain *d);
+int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count);
+int iommu_iotlb_flush_all(struct domain *d);
/*
* The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-18 14:00 ` [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
@ 2016-04-25 10:19 ` Jan Beulich
2016-04-26 12:23 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 10:19 UTC (permalink / raw)
To: Quan Xu; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain *d,
> if ( need_iommu(d) )
> {
> this_cpu(iommu_dont_flush_iotlb) = 0;
> - iommu_iotlb_flush(d, xatp->idx - done, done);
> - iommu_iotlb_flush(d, xatp->gpfn - done, done);
> + rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> + if ( !rc )
> + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
And again - best effort flushing in all cases. I.e. you shouldn't
bypass the second one if the first one failed, but instead
properly accumulate the return value, and also again not clobber
any negative value rc may already hold. What's worse (and
makes me wonder whether this got tested) - you also clobber the
positive return value this function may produce, even in the case
the flushes succeed (and hence the function as a whole was
successful).
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *, struct domain *d,
> int iommu_do_domctl(struct xen_domctl *, struct domain *d,
> XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>
> -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count);
> -void iommu_iotlb_flush_all(struct domain *d);
> +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count);
> +int iommu_iotlb_flush_all(struct domain *d);
__must_check
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-25 10:19 ` Jan Beulich
@ 2016-04-26 12:23 ` Xu, Quan
2016-04-26 12:48 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 12:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
On April 25, 2016 6:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain
> *d,
> > if ( need_iommu(d) )
> > {
> > this_cpu(iommu_dont_flush_iotlb) = 0;
> > - iommu_iotlb_flush(d, xatp->idx - done, done);
> > - iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > + rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> > + if ( !rc )
> > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
>
> And again - best effort flushing in all cases. I.e. you shouldn't bypass the
> second one if the first one failed, but instead properly accumulate the return
> value, and also again not clobber any negative value rc may already hold.
Thanks for correcting me. I did like accumulating the return value. :(
I will follow your suggestion in next v3.
> What's worse (and makes me wonder whether this got tested) - you also
> clobber the positive return value this function may produce, even in the case
> the flushes succeed (and hence the function as a whole was successful).
>
I have tested it with previous patches (i.e. 1st patch), launching a VM with PT ATS device to invoke this call tree.
btw, I fix the positive issue in 1st patch.
I know this is not strict, as I assume this patch set will be committed at the same time.
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -182,8 +182,8 @@ int iommu_do_pci_domctl(struct xen_domctl *,
> > struct domain *d, int iommu_do_domctl(struct xen_domctl *, struct domain
> *d,
> > XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
> >
> > -void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned
> > int page_count); -void iommu_iotlb_flush_all(struct domain *d);
> > +int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned
> > +int page_count); int iommu_iotlb_flush_all(struct domain *d);
>
> __must_check
>
Agreed.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-26 12:23 ` Xu, Quan
@ 2016-04-26 12:48 ` Jan Beulich
2016-04-27 6:21 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-26 12:48 UTC (permalink / raw)
To: Quan Xu; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
>>> On 26.04.16 at 14:23, <quan.xu@intel.com> wrote:
> On April 25, 2016 6:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > --- a/xen/common/memory.c
>> > +++ b/xen/common/memory.c
>> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct domain
>> *d,
>> > if ( need_iommu(d) )
>> > {
>> > this_cpu(iommu_dont_flush_iotlb) = 0;
>> > - iommu_iotlb_flush(d, xatp->idx - done, done);
>> > - iommu_iotlb_flush(d, xatp->gpfn - done, done);
>> > + rc = iommu_iotlb_flush(d, xatp->idx - done, done);
>> > + if ( !rc )
>> > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
>>
>> And again - best effort flushing in all cases. I.e. you shouldn't bypass the
>> second one if the first one failed, but instead properly accumulate the return
>> value, and also again not clobber any negative value rc may already hold.
>
>
> Thanks for correcting me. I did like accumulating the return value. :(
> I will follow your suggestion in next v3.
>
>> What's worse (and makes me wonder whether this got tested) - you also
>> clobber the positive return value this function may produce, even in the case
>> the flushes succeed (and hence the function as a whole was successful).
>>
> I have tested it with previous patches (i.e. 1st patch), launching a VM
> with PT ATS device to invoke this call tree.
> btw, I fix the positive issue in 1st patch.
> I know this is not strict, as I assume this patch set will be committed at
> the same time.
Hmm, the "positive" here has nothing to do with the "positive" in
patch 1. Please just have a look at xenmem_add_to_physmap() as
a whole.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-26 12:48 ` Jan Beulich
@ 2016-04-27 6:21 ` Xu, Quan
2016-04-27 6:31 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-27 6:21 UTC (permalink / raw)
To: Jan Beulich; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
On April 26, 2016 8:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 26.04.16 at 14:23, <quan.xu@intel.com> wrote:
> > On April 25, 2016 6:19 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> > --- a/xen/common/memory.c
> >> > +++ b/xen/common/memory.c
> >> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct
> domain
> >> *d,
> >> > if ( need_iommu(d) )
> >> > {
> >> > this_cpu(iommu_dont_flush_iotlb) = 0;
> >> > - iommu_iotlb_flush(d, xatp->idx - done, done);
> >> > - iommu_iotlb_flush(d, xatp->gpfn - done, done);
> >> > + rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> >> > + if ( !rc )
> >> > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> >>
> >> And again - best effort flushing in all cases. I.e. you shouldn't
> >> bypass the second one if the first one failed, but instead properly
> >> accumulate the return value, and also again not clobber any negative value
> rc may already hold.
> >
> >
> > Thanks for correcting me. I did like accumulating the return value.
> > :( I will follow your suggestion in next v3.
> >
> >> What's worse (and makes me wonder whether this got tested) - you also
> >> clobber the positive return value this function may produce, even in
> >> the case the flushes succeed (and hence the function as a whole was
> successful).
> >>
> > I have tested it with previous patches (i.e. 1st patch), launching a
> > VM with PT ATS device to invoke this call tree.
> > btw, I fix the positive issue in 1st patch.
> > I know this is not strict, as I assume this patch set will be
> > committed at the same time.
>
> Hmm, the "positive" here has nothing to do with the "positive" in patch 1.
> Please just have a look at xenmem_add_to_physmap() as a whole.
>
Thanks for reminding me. The 'positive' is ' rc = start + done'..
Think twice, I found I need two other new return values for this function (correct me if I am wrong!).
If the first iommu_iotlb_flush() is failed, I shouldn't accumulate the return value of
the second iommu_iotlb_flush() -- but instead properly accumulate the return value.
The following is my modification:
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -640,6 +640,10 @@ static int xenmem_add_to_physmap(struct domain *d,
unsigned int done = 0;
long rc = 0;
+#ifdef CONFIG_HAS_PASSTHROUGH
+ int ret, rv;
+#endif
+
if ( xatp->space != XENMAPSPACE_gmfn_range )
return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
xatp->idx, xatp->gpfn);
@@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct domain *d,
if ( need_iommu(d) )
{
this_cpu(iommu_dont_flush_iotlb) = 0;
- iommu_iotlb_flush(d, xatp->idx - done, done);
- iommu_iotlb_flush(d, xatp->gpfn - done, done);
+ ret = iommu_iotlb_flush(d, xatp->idx - done, done);
+
+ if ( rc >= 0 && ret < 0 )
+ rc = ret;
+
+ rv = iommu_iotlb_flush(d, xatp->gpfn - done, done);
+
+ if ( rc >=0 && ret == 0 && rv < 0 )
+ rc = rv;
}
#endif
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-27 6:21 ` Xu, Quan
@ 2016-04-27 6:31 ` Jan Beulich
2016-04-27 9:08 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-27 6:31 UTC (permalink / raw)
To: Quan Xu; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
>>> On 27.04.16 at 08:21, <quan.xu@intel.com> wrote:
> On April 26, 2016 8:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> Hmm, the "positive" here has nothing to do with the "positive" in patch 1.
>> Please just have a look at xenmem_add_to_physmap() as a whole.
>>
>
> Thanks for reminding me. The 'positive' is ' rc = start + done'..
>
> Think twice, I found I need two other new return values for this function
> (correct me if I am wrong!).
> If the first iommu_iotlb_flush() is failed, I shouldn't accumulate the
> return value of
> the second iommu_iotlb_flush() -- but instead properly accumulate the return
> value.
I think just one will do.
> The following is my modification:
>
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -640,6 +640,10 @@ static int xenmem_add_to_physmap(struct domain *d,
> unsigned int done = 0;
> long rc = 0;
>
> +#ifdef CONFIG_HAS_PASSTHROUGH
> + int ret, rv;
> +#endif
This should go into the scope below, allowing to avoid the extra
#ifdef.
> @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct domain *d,
> if ( need_iommu(d) )
> {
> this_cpu(iommu_dont_flush_iotlb) = 0;
> - iommu_iotlb_flush(d, xatp->idx - done, done);
> - iommu_iotlb_flush(d, xatp->gpfn - done, done);
> + ret = iommu_iotlb_flush(d, xatp->idx - done, done);
> +
> + if ( rc >= 0 && ret < 0 )
Why "ret < 0" instead of just "ret"?
> + rc = ret;
> +
> + rv = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> +
> + if ( rc >=0 && ret == 0 && rv < 0 )
> + rc = rv;
I don't see the middle part being needed here, eliminating the
need for "rv".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-27 6:31 ` Jan Beulich
@ 2016-04-27 9:08 ` Xu, Quan
2016-04-27 9:38 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-27 9:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
On April 27, 2016 2:32 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 27.04.16 at 08:21, <quan.xu@intel.com> wrote:
> > On April 26, 2016 8:49 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> Hmm, the "positive" here has nothing to do with the "positive" in patch 1.
> >> Please just have a look at xenmem_add_to_physmap() as a whole.
> >>
> >
> > Thanks for reminding me. The 'positive' is ' rc = start + done'..
> >
> > Think twice, I found I need two other new return values for this
> > function (correct me if I am wrong!).
> > If the first iommu_iotlb_flush() is failed, I shouldn't accumulate
> > the return value of the second iommu_iotlb_flush() -- but instead
> > properly accumulate the return value.
>
> I think just one will do.
>
Got it.
> > The following is my modification:
> >
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -640,6 +640,10 @@ static int xenmem_add_to_physmap(struct
> domain *d,
> > unsigned int done = 0;
> > long rc = 0;
> >
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > + int ret, rv;
> > +#endif
>
> This should go into the scope below, allowing to avoid the extra #ifdef.
>
Agreed .
> > @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct
> domain *d,
> > if ( need_iommu(d) )
> > {
Check it again. The 'int ret' declaration should be here?
> > this_cpu(iommu_dont_flush_iotlb) = 0;
> > - iommu_iotlb_flush(d, xatp->idx - done, done);
> > - iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > + ret = iommu_iotlb_flush(d, xatp->idx - done, done);
> > +
> > + if ( rc >= 0 && ret < 0 )
>
> Why "ret < 0" instead of just "ret"?
>
"ret" is enough.
> > + rc = ret;
> > +
> > + rv = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +
> > + if ( rc >=0 && ret == 0 && rv < 0 )
> > + rc = rv;
>
> I don't see the middle part being needed here, eliminating the need for "rv".
>
I will drop 'rv'.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-04-27 9:08 ` Xu, Quan
@ 2016-04-27 9:38 ` Jan Beulich
0 siblings, 0 replies; 78+ messages in thread
From: Jan Beulich @ 2016-04-27 9:38 UTC (permalink / raw)
To: Quan Xu; +Cc: Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
>>> On 27.04.16 at 11:08, <quan.xu@intel.com> wrote:
> On April 27, 2016 2:32 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 27.04.16 at 08:21, <quan.xu@intel.com> wrote:
>> > @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct
>> domain *d,
>> > if ( need_iommu(d) )
>> > {
>
> Check it again. The 'int ret' declaration should be here?
Yes, exactly.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (6 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-19 6:58 ` Tian, Kevin
2016-04-25 11:39 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
` (2 subsequent siblings)
10 siblings, 2 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, dario.faggioli, Feng Wu, Jan Beulich, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}.
If IOMMU mapping and unmapping failed, the domain (with the exception of
the hardware domain) is crashed, treated as a fatal error. Rollback can
be lighter weight.
This patch fixes the leaf ones.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/drivers/passthrough/iommu.c | 8 ++------
xen/drivers/passthrough/vtd/iommu.c | 8 ++++----
xen/include/xen/iommu.h | 4 ++--
3 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 9254760..d44fc39 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -308,9 +308,7 @@ int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_cou
if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
return 0;
- hd->platform_ops->iotlb_flush(d, gfn, page_count);
-
- return 0;
+ return hd->platform_ops->iotlb_flush(d, gfn, page_count);
}
int iommu_iotlb_flush_all(struct domain *d)
@@ -320,9 +318,7 @@ int iommu_iotlb_flush_all(struct domain *d)
if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
return 0;
- hd->platform_ops->iotlb_flush_all(d);
-
- return 0;
+ return hd->platform_ops->iotlb_flush_all(d);
}
int __init iommu_setup(void)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5cc70ca..930661a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -606,14 +606,14 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
return rc;
}
-static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
+static int intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
{
- iommu_flush_iotlb(d, gfn, 1, page_count);
+ return iommu_flush_iotlb(d, gfn, 1, page_count);
}
-static void intel_iommu_iotlb_flush_all(struct domain *d)
+static int intel_iommu_iotlb_flush_all(struct domain *d)
{
- iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
+ return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
}
/* clear one page's page table */
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index ff4608d..ca1c409 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -161,8 +161,8 @@ struct iommu_ops {
void (*resume)(void);
void (*share_p2m)(struct domain *d);
void (*crash_shutdown)(void);
- void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
- void (*iotlb_flush_all)(struct domain *d);
+ int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
+ int (*iotlb_flush_all)(struct domain *d);
int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
void (*dump_p2m_table)(struct domain *d);
};
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-18 14:00 ` [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
@ 2016-04-19 6:58 ` Tian, Kevin
2016-04-19 8:58 ` Xu, Quan
2016-04-25 11:39 ` Jan Beulich
1 sibling, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-19 6:58 UTC (permalink / raw)
To: Xu, Quan, xen-devel; +Cc: dario.faggioli, Wu, Feng, Jan Beulich
> From: Xu, Quan
> Sent: Monday, April 18, 2016 10:00 PM
>
> While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> However the existing panic() is going to be eliminated, so we must
> propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}.
>
> If IOMMU mapping and unmapping failed, the domain (with the exception of
> the hardware domain) is crashed, treated as a fatal error. Rollback can
> be lighter weight.
>
> This patch fixes the leaf ones.
what about other leaves like AMD/ARM callbacks?
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Feng Wu <feng.wu@intel.com>
> ---
> xen/drivers/passthrough/iommu.c | 8 ++------
> xen/drivers/passthrough/vtd/iommu.c | 8 ++++----
> xen/include/xen/iommu.h | 4 ++--
> 3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 9254760..d44fc39 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -308,9 +308,7 @@ int iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> unsigned int page_cou
> if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush )
> return 0;
>
> - hd->platform_ops->iotlb_flush(d, gfn, page_count);
> -
> - return 0;
> + return hd->platform_ops->iotlb_flush(d, gfn, page_count);
> }
>
> int iommu_iotlb_flush_all(struct domain *d)
> @@ -320,9 +318,7 @@ int iommu_iotlb_flush_all(struct domain *d)
> if ( !iommu_enabled || !hd->platform_ops || !hd->platform_ops->iotlb_flush_all )
> return 0;
>
> - hd->platform_ops->iotlb_flush_all(d);
> -
> - return 0;
> + return hd->platform_ops->iotlb_flush_all(d);
> }
>
> int __init iommu_setup(void)
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 5cc70ca..930661a 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -606,14 +606,14 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long
> gfn,
> return rc;
> }
>
> -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int
> page_count)
> +static int intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int
> page_count)
> {
> - iommu_flush_iotlb(d, gfn, 1, page_count);
> + return iommu_flush_iotlb(d, gfn, 1, page_count);
> }
>
> -static void intel_iommu_iotlb_flush_all(struct domain *d)
> +static int intel_iommu_iotlb_flush_all(struct domain *d)
> {
> - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> }
>
> /* clear one page's page table */
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index ff4608d..ca1c409 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -161,8 +161,8 @@ struct iommu_ops {
> void (*resume)(void);
> void (*share_p2m)(struct domain *d);
> void (*crash_shutdown)(void);
> - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
> - void (*iotlb_flush_all)(struct domain *d);
> + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int page_count);
> + int (*iotlb_flush_all)(struct domain *d);
> int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
> void (*dump_p2m_table)(struct domain *d);
> };
> --
> 1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-19 6:58 ` Tian, Kevin
@ 2016-04-19 8:58 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-19 8:58 UTC (permalink / raw)
To: Tian, Kevin, xen-devel; +Cc: dario.faggioli, Wu, Feng, Jan Beulich
On April 19, 2016 2:58pm, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> > However the existing panic() is going to be eliminated, so we must
> > propagate the IOMMU Device-TLB flush error up to the
> iommu_iotlb_flush{,_all}.
> >
> > If IOMMU mapping and unmapping failed, the domain (with the exception
> > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > can be lighter weight.
> >
> > This patch fixes the leaf ones.
>
> what about other leaves like AMD/ARM callbacks?
>
Thank you for reminding me of such cases. I really missed them in this v2.
Quan
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >
> > CC: Jan Beulich <jbeulich@suse.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Feng Wu <feng.wu@intel.com>
> > ---
> > xen/drivers/passthrough/iommu.c | 8 ++------
> > xen/drivers/passthrough/vtd/iommu.c | 8 ++++----
> > xen/include/xen/iommu.h | 4 ++--
> > 3 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/iommu.c
> > b/xen/drivers/passthrough/iommu.c index 9254760..d44fc39 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -308,9 +308,7 @@ int iommu_iotlb_flush(struct domain *d, unsigned
> > long gfn, unsigned int page_cou
> > if ( !iommu_enabled || !hd->platform_ops
> || !hd->platform_ops->iotlb_flush )
> > return 0;
> >
> > - hd->platform_ops->iotlb_flush(d, gfn, page_count);
> > -
> > - return 0;
> > + return hd->platform_ops->iotlb_flush(d, gfn, page_count);
> > }
> >
> > int iommu_iotlb_flush_all(struct domain *d) @@ -320,9 +318,7 @@ int
> > iommu_iotlb_flush_all(struct domain *d)
> > if ( !iommu_enabled || !hd->platform_ops
> || !hd->platform_ops->iotlb_flush_all )
> > return 0;
> >
> > - hd->platform_ops->iotlb_flush_all(d);
> > -
> > - return 0;
> > + return hd->platform_ops->iotlb_flush_all(d);
> > }
> >
> > int __init iommu_setup(void)
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index 5cc70ca..930661a 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -606,14 +606,14 @@ static int iommu_flush_iotlb(struct domain *d,
> > unsigned long gfn,
> > return rc;
> > }
> >
> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > gfn, unsigned int
> > page_count)
> > +static int intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > +gfn, unsigned int
> > page_count)
> > {
> > - iommu_flush_iotlb(d, gfn, 1, page_count);
> > + return iommu_flush_iotlb(d, gfn, 1, page_count);
> > }
> >
> > -static void intel_iommu_iotlb_flush_all(struct domain *d)
> > +static int intel_iommu_iotlb_flush_all(struct domain *d)
> > {
> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> > }
> >
> > /* clear one page's page table */
> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index
> > ff4608d..ca1c409 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -161,8 +161,8 @@ struct iommu_ops {
> > void (*resume)(void);
> > void (*share_p2m)(struct domain *d);
> > void (*crash_shutdown)(void);
> > - void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int
> page_count);
> > - void (*iotlb_flush_all)(struct domain *d);
> > + int (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int
> page_count);
> > + int (*iotlb_flush_all)(struct domain *d);
> > int (*get_reserved_device_memory)(iommu_grdm_t *, void *);
> > void (*dump_p2m_table)(struct domain *d); };
> > --
> > 1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-18 14:00 ` [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
2016-04-19 6:58 ` Tian, Kevin
@ 2016-04-25 11:39 ` Jan Beulich
2016-04-26 11:50 ` Xu, Quan
1 sibling, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 11:39 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> However the existing panic() is going to be eliminated, so we must
> propagate the IOMMU Device-TLB flush error up to the
> iommu_iotlb_flush{,_all}.
>
> If IOMMU mapping and unmapping failed, the domain (with the exception of
> the hardware domain) is crashed, treated as a fatal error. Rollback can
> be lighter weight.
>
> This patch fixes the leaf ones.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
Looks okay except for the again missing uses of __must_check on
function declarations or, where needed, definitions.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-25 11:39 ` Jan Beulich
@ 2016-04-26 11:50 ` Xu, Quan
2016-04-26 12:51 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 11:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On April 25, 2016 7:40 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> > However the existing panic() is going to be eliminated, so we must
> > propagate the IOMMU Device-TLB flush error up to the
> > iommu_iotlb_flush{,_all}.
> >
> > If IOMMU mapping and unmapping failed, the domain (with the exception
> > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > can be lighter weight.
> >
> > This patch fixes the leaf ones.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
>
> Looks okay except for the again missing uses of __must_check on function
> declarations or, where needed, definitions.
>
This patch modifies the common part 'iommu_ops', but not to
fix the ARM/AMD callbacks ( it seems the callbacks are not initialized for AMD code, but ARM does).
I need to fix ARM callbacks as well.
I wonder whether it is good to use __must_check on these callbacks or not.
IMO I'd better use __must_check on functions, i.e. iommu_iotlb_flush_all(), invoking these callbacks, instead of these callbacks.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-26 11:50 ` Xu, Quan
@ 2016-04-26 12:51 ` Jan Beulich
2016-04-26 13:20 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-26 12:51 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 26.04.16 at 13:50, <quan.xu@intel.com> wrote:
> On April 25, 2016 7:40 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > While IOMMU Device-TLB flush timed out, xen calls panic() at present.
>> > However the existing panic() is going to be eliminated, so we must
>> > propagate the IOMMU Device-TLB flush error up to the
>> > iommu_iotlb_flush{,_all}.
>> >
>> > If IOMMU mapping and unmapping failed, the domain (with the exception
>> > of the hardware domain) is crashed, treated as a fatal error. Rollback
>> > can be lighter weight.
>> >
>> > This patch fixes the leaf ones.
>> >
>> > Signed-off-by: Quan Xu <quan.xu@intel.com>
>>
>> Looks okay except for the again missing uses of __must_check on function
>> declarations or, where needed, definitions.
>
> This patch modifies the common part 'iommu_ops', but not to
> fix the ARM/AMD callbacks ( it seems the callbacks are not initialized for
> AMD code, but ARM does).
> I need to fix ARM callbacks as well.
>
>
> I wonder whether it is good to use __must_check on these callbacks or not.
For callbacks it's indeed questionable.
> IMO I'd better use __must_check on functions, i.e. iommu_iotlb_flush_all(),
> invoking these callbacks, instead of these callbacks.
At least the two functions in xen/drivers/passthrough/iommu.c that
this patch alters need to gain __must_check annotations (if they
don't already in an earlier patch - that's simply not possible to tell on
this version, which fails to add any such annotations).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-04-26 12:51 ` Jan Beulich
@ 2016-04-26 13:20 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 13:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On April 26, 2016 8:52 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 26.04.16 at 13:50, <quan.xu@intel.com> wrote:
> > On April 25, 2016 7:40 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> > While IOMMU Device-TLB flush timed out, xen calls panic() at present.
> >> > However the existing panic() is going to be eliminated, so we must
> >> > propagate the IOMMU Device-TLB flush error up to the
> >> > iommu_iotlb_flush{,_all}.
> >> >
> >> > If IOMMU mapping and unmapping failed, the domain (with the
> >> > exception of the hardware domain) is crashed, treated as a fatal
> >> > error. Rollback can be lighter weight.
> >> >
> >> > This patch fixes the leaf ones.
> >> >
> >> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> >>
> >> Looks okay except for the again missing uses of __must_check on
> >> function declarations or, where needed, definitions.
> >
> > This patch modifies the common part 'iommu_ops', but not to fix the
> > ARM/AMD callbacks ( it seems the callbacks are not initialized for AMD
> > code, but ARM does).
> > I need to fix ARM callbacks as well.
> >
> >
> > I wonder whether it is good to use __must_check on these callbacks or not.
>
> For callbacks it's indeed questionable.
>
I think so.
> > IMO I'd better use __must_check on functions, i.e.
> > iommu_iotlb_flush_all(), invoking these callbacks, instead of these
> callbacks.
>
> At least the two functions in xen/drivers/passthrough/iommu.c that this
> patch alters need to gain __must_check annotations (if they don't already in
> an earlier patch - that's simply not possible to tell on this version, which fails to
> add any such annotations).
>
I didn't add __must_check annotations in v2.
I will consider __must_check annotations carefully in next v3.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (7 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 08/11] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-25 11:52 ` Jan Beulich
2016-04-18 14:00 ` [PATCH v2 10/11] vt-d: propagate IOMMU Device-TLB flush error up to vt-d hardware initialization Quan Xu
2016-04-18 14:00 ` [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
10 siblings, 1 reply; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Quan Xu, Liu Jinsong, dario.faggioli,
Julien Grall, Stefano Stabellini, Jan Beulich, Andrew Cooper,
Feng Wu, Suravee Suthikulpanit
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to IOMMU suspending.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Liu Jinsong <jinsong.liu@alibaba-inc.com>
CC: Keir Fraser <keir@xen.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/acpi/power.c | 14 +++++++++++++-
xen/drivers/passthrough/amd/iommu_init.c | 9 ++++++++-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +-
xen/drivers/passthrough/arm/smmu.c | 10 ++++++----
xen/drivers/passthrough/iommu.c | 7 +++++--
xen/drivers/passthrough/vtd/iommu.c | 22 +++++++++++++++++-----
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 2 +-
xen/include/xen/iommu.h | 4 ++--
8 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 2885e31..9b87f00 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -45,19 +45,31 @@ void do_suspend_lowlevel(void);
static int device_power_down(void)
{
+ int err;
+
console_suspend();
time_suspend();
i8259A_suspend();
- ioapic_suspend();
+ err = iommu_suspend();
+ if ( err )
+ goto iommu_suspend_error;
iommu_suspend();
lapic_suspend();
return 0;
+
+ iommu_suspend_error:
+ ioapic_resume();
+ i8259A_resume();
+ time_resume();
+ console_resume();
+
+ return err;
}
static void device_power_up(void)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 4536106..02588aa 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1339,12 +1339,19 @@ static void invalidate_all_devices(void)
iterate_ivrs_mappings(_invalidate_all_devices);
}
-void amd_iommu_suspend(void)
+int amd_iommu_suspend(void)
{
struct amd_iommu *iommu;
for_each_amd_iommu ( iommu )
disable_iommu(iommu);
+
+ return 0;
+}
+
+void amd_iommu_crash_shutdown(void)
+{
+ amd_iommu_suspend();
}
void amd_iommu_resume(void)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index c8ee8dc..fb8e816 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -628,6 +628,6 @@ const struct iommu_ops amd_iommu_ops = {
.suspend = amd_iommu_suspend,
.resume = amd_iommu_resume,
.share_p2m = amd_iommu_share_p2m,
- .crash_shutdown = amd_iommu_suspend,
+ .crash_shutdown = amd_iommu_crash_shutdown,
.dump_p2m_table = amd_dump_p2m_table,
};
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 62da087..36c2c83 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2540,7 +2540,7 @@ static int force_stage = 2;
*/
static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
-static void arm_smmu_iotlb_flush_all(struct domain *d)
+static int arm_smmu_iotlb_flush_all(struct domain *d)
{
struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv;
struct iommu_domain *cfg;
@@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d)
arm_smmu_tlb_inv_context(cfg->priv);
}
spin_unlock(&smmu_domain->lock);
+
+ return 0;
}
-static void arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn,
- unsigned int page_count)
+static int arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn,
+ unsigned int page_count)
{
/* ARM SMMU v1 doesn't have flush by VMA and VMID */
- arm_smmu_iotlb_flush_all(d);
+ return arm_smmu_iotlb_flush_all(d);
}
static struct iommu_domain *arm_smmu_get_domain(struct domain *d,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index d44fc39..98b934b 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -392,11 +392,14 @@ int iommu_do_domctl(
return ret;
}
-void iommu_suspend()
+int iommu_suspend()
{
const struct iommu_ops *ops = iommu_get_ops();
+
if ( iommu_enabled )
- ops->suspend();
+ return ops->suspend();
+
+ return 0;
}
void iommu_share_p2m_table(struct domain* d)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 930661a..a821306 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -542,11 +542,12 @@ static int iommu_flush_iotlb_psi(
return status;
}
-static void iommu_flush_all(void)
+static int iommu_flush_all(void)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu;
int flush_dev_iotlb;
+ int rc = 0;
flush_all_cache();
for_each_drhd_unit ( drhd )
@@ -554,8 +555,13 @@ static void iommu_flush_all(void)
iommu = drhd->iommu;
iommu_flush_context_global(iommu, 0);
flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
- iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+ rc = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+
+ if ( rc )
+ break;
}
+
+ return rc;
}
static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
@@ -2421,16 +2427,20 @@ static int intel_iommu_group_id(u16 seg, u8 bus, u8 devfn)
}
static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
-static void vtd_suspend(void)
+static int vtd_suspend(void)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu;
u32 i;
+ int rc;
if ( !iommu_enabled )
- return;
+ return 0;
- iommu_flush_all();
+ rc = iommu_flush_all();
+
+ if ( rc )
+ return rc;
for_each_drhd_unit ( drhd )
{
@@ -2459,6 +2469,8 @@ static void vtd_suspend(void)
if ( !iommu_intremap && iommu_qinval )
disable_qinval(iommu);
}
+
+ return 0;
}
static void vtd_crash_shutdown(void)
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 9c51172..f540fc8 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -119,7 +119,7 @@ extern unsigned long *shared_intremap_inuse;
/* power management support */
void amd_iommu_resume(void);
-void amd_iommu_suspend(void);
+int amd_iommu_suspend(void);
void amd_iommu_crash_shutdown(void);
/* guest iommu support */
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index ca1c409..8710dfe 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -157,7 +157,7 @@ struct iommu_ops {
unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
int (*setup_hpet_msi)(struct msi_desc *);
#endif /* CONFIG_X86 */
- void (*suspend)(void);
+ int (*suspend)(void);
void (*resume)(void);
void (*share_p2m)(struct domain *d);
void (*crash_shutdown)(void);
@@ -167,7 +167,7 @@ struct iommu_ops {
void (*dump_p2m_table)(struct domain *d);
};
-void iommu_suspend(void);
+int iommu_suspend(void);
void iommu_resume(void);
void iommu_crash_shutdown(void);
int iommu_get_reserved_device_memory(iommu_grdm_t *, void *);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-18 14:00 ` [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
@ 2016-04-25 11:52 ` Jan Beulich
2016-04-25 13:58 ` Julien Grall
` (2 more replies)
0 siblings, 3 replies; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 11:52 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, Stefano Stabellini, Suravee Suthikulpanit,
Andrew Cooper, Keir Fraser
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -45,19 +45,31 @@ void do_suspend_lowlevel(void);
>
> static int device_power_down(void)
> {
> + int err;
> +
> console_suspend();
>
> time_suspend();
>
> i8259A_suspend();
>
> - ioapic_suspend();
> + err = iommu_suspend();
> + if ( err )
> + goto iommu_suspend_error;
>
> iommu_suspend();
A little more care please - this is definitely not what you meant.
> + iommu_suspend_error:
> + ioapic_resume();
> + i8259A_resume();
> + time_resume();
> + console_resume();
> +
> + return err;
Instead of mostly repeating what device_power_up() does I
wonder whether it wouldn't be better to re-use that function for
the error path here. Either by suitably making information
available to device_power_up() to skip the call to lapic_resume()
or by making lapic_resume() itself recognize it got called without
lapic_suspend() having got called first.
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2540,7 +2540,7 @@ static int force_stage = 2;
> */
> static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
>
> -static void arm_smmu_iotlb_flush_all(struct domain *d)
> +static int arm_smmu_iotlb_flush_all(struct domain *d)
> {
> struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv;
> struct iommu_domain *cfg;
> @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d)
> arm_smmu_tlb_inv_context(cfg->priv);
> }
> spin_unlock(&smmu_domain->lock);
> +
> + return 0;
> }
Even if indentation looks inconsistent in this file, please make your
addition match surrounding code.
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -542,11 +542,12 @@ static int iommu_flush_iotlb_psi(
> return status;
> }
>
> -static void iommu_flush_all(void)
> +static int iommu_flush_all(void)
__must_check
> @@ -554,8 +555,13 @@ static void iommu_flush_all(void)
> iommu = drhd->iommu;
> iommu_flush_context_global(iommu, 0);
> flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> - iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> + rc = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +
> + if ( rc )
> + break;
This again violates the best effort flushing principle.
> @@ -2421,16 +2427,20 @@ static int intel_iommu_group_id(u16 seg, u8 bus, u8 devfn)
> }
>
> static u32 iommu_state[MAX_IOMMUS][MAX_IOMMU_REGS];
> -static void vtd_suspend(void)
> +static int vtd_suspend(void)
Please take the opportunity to add the missing blank line between
variable and function definition.
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -157,7 +157,7 @@ struct iommu_ops {
> unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
> int (*setup_hpet_msi)(struct msi_desc *);
> #endif /* CONFIG_X86 */
> - void (*suspend)(void);
> + int (*suspend)(void);
> void (*resume)(void);
> void (*share_p2m)(struct domain *d);
> void (*crash_shutdown)(void);
> @@ -167,7 +167,7 @@ struct iommu_ops {
> void (*dump_p2m_table)(struct domain *d);
> };
>
> -void iommu_suspend(void);
> +int iommu_suspend(void);
At the very least this one should become __must_check.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-25 11:52 ` Jan Beulich
@ 2016-04-25 13:58 ` Julien Grall
2016-04-25 14:50 ` Xu, Quan
2016-04-26 11:28 ` Xu, Quan
2016-04-28 14:14 ` Xu, Quan
2 siblings, 1 reply; 78+ messages in thread
From: Julien Grall @ 2016-04-25 13:58 UTC (permalink / raw)
To: Jan Beulich, Quan Xu
Cc: Kevin Tian, Feng Wu, Liu Jinsong, dario.faggioli, xen-devel,
Stefano Stabellini, Suravee Suthikulpanit, Andrew Cooper,
Keir Fraser
Hi Jan,
On 25/04/16 12:52, Jan Beulich wrote:
>>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2540,7 +2540,7 @@ static int force_stage = 2;
>> */
>> static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
>>
>> -static void arm_smmu_iotlb_flush_all(struct domain *d)
>> +static int arm_smmu_iotlb_flush_all(struct domain *d)
>> {
>> struct arm_smmu_xen_domain *smmu_domain = domain_hvm_iommu(d)->arch.priv;
>> struct iommu_domain *cfg;
>> @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct domain *d)
>> arm_smmu_tlb_inv_context(cfg->priv);
>> }
>> spin_unlock(&smmu_domain->lock);
>> +
>> + return 0;
>> }
>
> Even if indentation looks inconsistent in this file, please make your
> addition match surrounding code.
The file smmu.c was imported from Linux and supposed to use only Linux
coding style. This is in order to help porting fixes.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-25 13:58 ` Julien Grall
@ 2016-04-25 14:50 ` Xu, Quan
2016-04-26 9:40 ` Julien Grall
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-25 14:50 UTC (permalink / raw)
To: Julien Grall, Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Stefano Stabellini, Suravee Suthikulpanit, Andrew Cooper,
Keir Fraser
On April 25, 2016 9:58pm, <julien.grall@arm.com> wrote:
> On 25/04/16 12:52, Jan Beulich wrote:
> >>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> --- a/xen/drivers/passthrough/arm/smmu.c
> >> +++ b/xen/drivers/passthrough/arm/smmu.c
> >> @@ -2540,7 +2540,7 @@ static int force_stage = 2;
> >> */
> >> static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
> >>
> >> -static void arm_smmu_iotlb_flush_all(struct domain *d)
> >> +static int arm_smmu_iotlb_flush_all(struct domain *d)
> >> {
> >> struct arm_smmu_xen_domain *smmu_domain =
> domain_hvm_iommu(d)->arch.priv;
> >> struct iommu_domain *cfg;
> >> @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct
> domain *d)
> >> arm_smmu_tlb_inv_context(cfg->priv);
> >> }
> >> spin_unlock(&smmu_domain->lock);
> >> +
> >> + return 0;
> >> }
> >
> > Even if indentation looks inconsistent in this file, please make your
> > addition match surrounding code.
>
> The file smmu.c was imported from Linux and supposed to use only Linux
> coding style. This is in order to help porting fixes.
>
Julien, A 'tab' ?
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-25 14:50 ` Xu, Quan
@ 2016-04-26 9:40 ` Julien Grall
0 siblings, 0 replies; 78+ messages in thread
From: Julien Grall @ 2016-04-26 9:40 UTC (permalink / raw)
To: Xu, Quan, Julien Grall, Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Stefano Stabellini, Suravee Suthikulpanit, Andrew Cooper,
Keir Fraser
Hi Quan,
On 25/04/2016 15:50, Xu, Quan wrote:
> On April 25, 2016 9:58pm, <julien.grall@arm.com> wrote:
>> On 25/04/16 12:52, Jan Beulich wrote:
>>>>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>>>> --- a/xen/drivers/passthrough/arm/smmu.c
>>>> +++ b/xen/drivers/passthrough/arm/smmu.c
>>>> @@ -2540,7 +2540,7 @@ static int force_stage = 2;
>>>> */
>>>> static u32 platform_features = ARM_SMMU_FEAT_COHERENT_WALK;
>>>>
>>>> -static void arm_smmu_iotlb_flush_all(struct domain *d)
>>>> +static int arm_smmu_iotlb_flush_all(struct domain *d)
>>>> {
>>>> struct arm_smmu_xen_domain *smmu_domain =
>> domain_hvm_iommu(d)->arch.priv;
>>>> struct iommu_domain *cfg;
>>>> @@ -2557,13 +2557,15 @@ static void arm_smmu_iotlb_flush_all(struct
>> domain *d)
>>>> arm_smmu_tlb_inv_context(cfg->priv);
>>>> }
>>>> spin_unlock(&smmu_domain->lock);
>>>> +
>>>> + return 0;
>>>> }
>>>
>>> Even if indentation looks inconsistent in this file, please make your
>>> addition match surrounding code.
>>
>> The file smmu.c was imported from Linux and supposed to use only Linux
>> coding style. This is in order to help porting fixes.
>>
>
> Julien, A 'tab' ?
Yes, Linux is using hard tabulation for the indentation.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-25 11:52 ` Jan Beulich
2016-04-25 13:58 ` Julien Grall
@ 2016-04-26 11:28 ` Xu, Quan
2016-04-28 14:14 ` Xu, Quan
2 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 11:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, Stefano Stabellini, Suravee Suthikulpanit,
Andrew Cooper, Keir Fraser
On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > @@ -45,19 +45,31 @@ void do_suspend_lowlevel(void);
> >
> > static int device_power_down(void)
> > {
> > + int err;
> > +
> > console_suspend();
> >
> > time_suspend();
> >
> > i8259A_suspend();
> >
> > - ioapic_suspend();
> > + err = iommu_suspend();
> > + if ( err )
> > + goto iommu_suspend_error;
> >
> > iommu_suspend();
>
> A little more care please - this is definitely not what you meant.
>
Very sorry for this..:(
> > + iommu_suspend_error:
> > + ioapic_resume();
> > + i8259A_resume();
> > + time_resume();
> > + console_resume();
> > +
> > + return err;
>
> Instead of mostly repeating what device_power_up() does I wonder whether
> it wouldn't be better to re-use that function for the error path here. Either by
> suitably making information available to device_power_up() to skip the call to
> lapic_resume() or by making lapic_resume() itself recognize it got called
> without
> lapic_suspend() having got called first.
>
To be honest, now this modification is much more readable to me, however I will reconsider your suggestion.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-25 11:52 ` Jan Beulich
2016-04-25 13:58 ` Julien Grall
2016-04-26 11:28 ` Xu, Quan
@ 2016-04-28 14:14 ` Xu, Quan
2016-04-28 14:36 ` Jan Beulich
2 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-28 14:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, Stefano Stabellini, Suravee Suthikulpanit,
Andrew Cooper, Keir Fraser
On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > -static void iommu_flush_all(void)
> > +static int iommu_flush_all(void)
>
> __must_check
>
The iommu_flush_all() is also called in intel_iommu_hwdom_init() and vtd_crash_shutdown().
As we were on the same page, we can ignore the error code propagation for these two call trees.
I wonder whether we really need this '__must_check' or not, furthermore, print-out message
Is pointless (at least, to me).
Jan, could I ignore this '__must_check '?
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-28 14:14 ` Xu, Quan
@ 2016-04-28 14:36 ` Jan Beulich
2016-04-28 15:03 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-28 14:36 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, Stefano Stabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
>>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
> On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>
>
>> > -static void iommu_flush_all(void)
>> > +static int iommu_flush_all(void)
>>
>> __must_check
>>
>
> The iommu_flush_all() is also called in intel_iommu_hwdom_init() and
> vtd_crash_shutdown().
> As we were on the same page, we can ignore the error code propagation for
> these two call trees.
I don't know what you're referring to here with "we were on the same
page". I don't think I've ever agreed (in the context of this series) to
ignore any error returns.
Jan
> I wonder whether we really need this '__must_check' or not, furthermore,
> print-out message
> Is pointless (at least, to me).
>
> Jan, could I ignore this '__must_check '?
>
> Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-28 14:36 ` Jan Beulich
@ 2016-04-28 15:03 ` Xu, Quan
2016-04-28 15:12 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-28 15:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, Stefano Stabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
On April 28, 2016 10:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
> > On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >
> >
> >> > -static void iommu_flush_all(void)
> >> > +static int iommu_flush_all(void)
> >>
> >> __must_check
> >>
> >
> > The iommu_flush_all() is also called in intel_iommu_hwdom_init() and
> > vtd_crash_shutdown().
> > As we were on the same page, we can ignore the error code propagation
> > for these two call trees.
>
> I don't know what you're referring to here with "we were on the same page". I
> don't think I've ever agreed (in the context of this series) to ignore any error
> returns.
>
Look at the below link.
http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.html
I hope I understand this correctly.
Quan
> Jan
>
> > I wonder whether we really need this '__must_check' or not,
> > furthermore, print-out message Is pointless (at least, to me).
> >
> > Jan, could I ignore this '__must_check '?
> >
> > Quan
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-28 15:03 ` Xu, Quan
@ 2016-04-28 15:12 ` Jan Beulich
2016-04-28 16:08 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-28 15:12 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, StefanoStabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
>>> On 28.04.16 at 17:03, <quan.xu@intel.com> wrote:
> On April 28, 2016 10:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
>> > On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >
>> >
>> >> > -static void iommu_flush_all(void)
>> >> > +static int iommu_flush_all(void)
>> >>
>> >> __must_check
>> >>
>> >
>> > The iommu_flush_all() is also called in intel_iommu_hwdom_init() and
>> > vtd_crash_shutdown().
>> > As we were on the same page, we can ignore the error code propagation
>> > for these two call trees.
>>
>> I don't know what you're referring to here with "we were on the same page".
> I
>> don't think I've ever agreed (in the context of this series) to ignore any
> error
>> returns.
>>
>
>
> Look at the below link.
> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.html
Which still talks about (conditionally) logging messages, not ignoring
of errors.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-28 15:12 ` Jan Beulich
@ 2016-04-28 16:08 ` Xu, Quan
2016-04-29 2:20 ` Tian, Kevin
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-28 16:08 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, StefanoStabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
On April 28, 2016 11:12 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 28.04.16 at 17:03, <quan.xu@intel.com> wrote:
> > On April 28, 2016 10:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
> >> > On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >
> >> >
> >> >> > -static void iommu_flush_all(void)
> >> >> > +static int iommu_flush_all(void)
> >> >>
> >> >> __must_check
> >> >>
> >> >
> >> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()
> >> > and vtd_crash_shutdown().
> >> > As we were on the same page, we can ignore the error code
> >> > propagation for these two call trees.
> >>
> >> I don't know what you're referring to here with "we were on the same
> page".
> > I
> >> don't think I've ever agreed (in the context of this series) to
> >> ignore any
> > error
> >> returns.
> >>
> >
> >
> > Look at the below link.
> > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.h
> > tml
>
> Which still talks about (conditionally) logging messages, not ignoring of errors.
>
Sorry, check it again.
in intel_iommu_hwdom_init() and vtd_crash_shutdown(),
I still need to log messages as:
if ( iommu_flush_all() )
printk()
?
Thanks for your patience.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-28 16:08 ` Xu, Quan
@ 2016-04-29 2:20 ` Tian, Kevin
2016-04-29 2:41 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Tian, Kevin @ 2016-04-29 2:20 UTC (permalink / raw)
To: Xu, Quan, Jan Beulich
Cc: Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel, Julien Grall,
StefanoStabellini, Suravee Suthikulpanit, AndrewCooper,
Keir Fraser
> From: Xu, Quan
> Sent: Friday, April 29, 2016 12:09 AM
>
> On April 28, 2016 11:12 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >>> On 28.04.16 at 17:03, <quan.xu@intel.com> wrote:
> > > On April 28, 2016 10:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >> >>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
> > >> > On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > >> >
> > >> >
> > >> >> > -static void iommu_flush_all(void)
> > >> >> > +static int iommu_flush_all(void)
> > >> >>
> > >> >> __must_check
> > >> >>
> > >> >
> > >> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()
> > >> > and vtd_crash_shutdown().
> > >> > As we were on the same page, we can ignore the error code
> > >> > propagation for these two call trees.
> > >>
> > >> I don't know what you're referring to here with "we were on the same
> > page".
> > > I
> > >> don't think I've ever agreed (in the context of this series) to
> > >> ignore any
> > > error
> > >> returns.
> > >>
> > >
> > >
> > > Look at the below link.
> > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.h
> > > tml
> >
> > Which still talks about (conditionally) logging messages, not ignoring of errors.
> >
>
> Sorry, check it again.
> in intel_iommu_hwdom_init() and vtd_crash_shutdown(),
> I still need to log messages as:
>
> if ( iommu_flush_all() )
> printk()
>
> ?
>
> Thanks for your patience.
> Quan
Yes, but please make sure you only print once to be useful.
Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-29 2:20 ` Tian, Kevin
@ 2016-04-29 2:41 ` Xu, Quan
2016-04-29 7:13 ` Jan Beulich
0 siblings, 1 reply; 78+ messages in thread
From: Xu, Quan @ 2016-04-29 2:41 UTC (permalink / raw)
To: Tian, Kevin, Jan Beulich
Cc: Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel, Julien Grall,
StefanoStabellini, Suravee Suthikulpanit, AndrewCooper,
Keir Fraser
On April 29, 2016 10:21 AM, Tian, Kevin <kevin.tian@intel.com> wrote:
> > From: Xu, Quan
> > Sent: Friday, April 29, 2016 12:09 AM
> > On April 28, 2016 11:12 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > > >>> On 28.04.16 at 17:03, <quan.xu@intel.com> wrote:
> > > > On April 28, 2016 10:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > > >> >>> On 28.04.16 at 16:14, <quan.xu@intel.com> wrote:
> > > >> > On April 25, 2016 7:53 PM, Jan Beulich <JBeulich@suse.com>
> wrote:
> > > >> >> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > >> >
> > > >> >
> > > >> >> > -static void iommu_flush_all(void)
> > > >> >> > +static int iommu_flush_all(void)
> > > >> >>
> > > >> >> __must_check
> > > >> >>
> > > >> >
> > > >> > The iommu_flush_all() is also called in
> > > >> > intel_iommu_hwdom_init() and vtd_crash_shutdown().
> > > >> > As we were on the same page, we can ignore the error code
> > > >> > propagation for these two call trees.
> > > >>
> > > >> I don't know what you're referring to here with "we were on the
> > > >> same
> > > page".
> > > > I
> > > >> don't think I've ever agreed (in the context of this series) to
> > > >> ignore any
> > > > error
> > > >> returns.
> > > >>
> > > >
> > > >
> > > > Look at the below link.
> > > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg032
> > > > 34.h
> > > > tml
> > >
> > > Which still talks about (conditionally) logging messages, not ignoring of
> errors.
> > >
> >
> > Sorry, check it again.
> > in intel_iommu_hwdom_init() and vtd_crash_shutdown(), I still need to
> > log messages as:
> >
> > if ( iommu_flush_all() )
> > printk()
> >
> > ?
> >
> > Thanks for your patience.
> > Quan
>
> Yes, but please make sure you only print once to be useful.
>
Thanks.
Now I check the status in caller to make the print include caller which is failed, instead print in iommu_flush_all().
i.e.,
vtd_crash_shutdown()
{
..
if ( iommu_flush_all() )
printk(XENLOG_WARNING VTDPREFIX
" vtd_crash_shutdown: IOMMU flush all failed.\n");
..
}
I am afraid I still don't get the point. To be honest, in such a fix,
The print is not so useful to me ( Correct me, I will continue to enhance it).
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-29 2:41 ` Xu, Quan
@ 2016-04-29 7:13 ` Jan Beulich
2016-04-29 8:23 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-29 7:13 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, StefanoStabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
>>> On 29.04.16 at 04:41, <quan.xu@intel.com> wrote:
> Now I check the status in caller to make the print include caller which is
> failed, instead print in iommu_flush_all().
> i.e.,
> vtd_crash_shutdown()
> {
> ..
> if ( iommu_flush_all() )
> printk(XENLOG_WARNING VTDPREFIX
> " vtd_crash_shutdown: IOMMU flush all failed.\n");
> ..
> }
>
> I am afraid I still don't get the point. To be honest, in such a fix,
> The print is not so useful to me ( Correct me, I will continue to enhance
> it).
So do you think it would be more useful to leave the admin with no
clue why a system misbehaves, instead of providing clear indication?
IOW - what usefulness concerns do you have?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread* Re: [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-04-29 7:13 ` Jan Beulich
@ 2016-04-29 8:23 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-29 8:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, Liu Jinsong, dario.faggioli, xen-devel,
Julien Grall, StefanoStabellini, Suravee Suthikulpanit,
AndrewCooper, Keir Fraser
On April 29, 2016 3:14 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 29.04.16 at 04:41, <quan.xu@intel.com> wrote:
> > Now I check the status in caller to make the print include caller
> > which is failed, instead print in iommu_flush_all().
> > i.e.,
> > vtd_crash_shutdown()
> > {
> > ..
> > if ( iommu_flush_all() )
> > printk(XENLOG_WARNING VTDPREFIX
> > " vtd_crash_shutdown: IOMMU flush all failed.\n"); ..
> > }
> >
> > I am afraid I still don't get the point. To be honest, in such a fix,
> > The print is not so useful to me ( Correct me, I will continue to
> > enhance it).
>
> So do you think it would be more useful to leave the admin with no clue why a
> system misbehaves, instead of providing clear indication?
> IOW - what usefulness concerns do you have?
>
Jan, I will follow your suggestion for v3. I look forward to hearing the other CCed maintainers' opinions on this.
...
i.e., In above case, in case I am an admin, I may be much more interested in what causes vt-d crash,
keeping quite during crash. It is just a matter of my personal preference. Ignore me.
...
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 10/11] vt-d: propagate IOMMU Device-TLB flush error up to vt-d hardware initialization
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (8 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-18 14:00 ` [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
10 siblings, 0 replies; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, dario.faggioli, Feng Wu, Jan Beulich, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to vt-d hardware
initialization.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/drivers/passthrough/vtd/iommu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a821306..223522f 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2137,8 +2137,8 @@ static int init_vtd_hw(void)
return -EIO;
}
}
- iommu_flush_all();
- return 0;
+
+ return iommu_flush_all();
}
static void __hwdom_init setup_hwdom_rmrr(struct domain *d)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-04-18 14:00 [PATCH v2 00/11] Check VT-d Device-TLB flush error Quan Xu
` (9 preceding siblings ...)
2016-04-18 14:00 ` [PATCH v2 10/11] vt-d: propagate IOMMU Device-TLB flush error up to vt-d hardware initialization Quan Xu
@ 2016-04-18 14:00 ` Quan Xu
2016-04-25 12:00 ` Jan Beulich
10 siblings, 1 reply; 78+ messages in thread
From: Quan Xu @ 2016-04-18 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, dario.faggioli, Feng Wu, Jan Beulich, Quan Xu
While IOMMU Device-TLB flush timed out, xen calls panic() at present.
However the existing panic() is going to be eliminated, so we must
propagate the IOMMU Device-TLB flush error up to ME phantom function
mapping and unmapping.
Signed-off-by: Quan Xu <quan.xu@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/drivers/passthrough/vtd/extern.h | 2 +-
xen/drivers/passthrough/vtd/quirks.c | 26 ++++++++++++++++----------
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index cbe0286..d4d37c3 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -91,7 +91,7 @@ int is_igd_vt_enabled_quirk(void);
void platform_quirks_init(void);
void vtd_ops_preamble_quirk(struct iommu* iommu);
void vtd_ops_postamble_quirk(struct iommu* iommu);
-void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
+int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
void pci_vtd_quirk(const struct pci_dev *);
bool_t platform_supports_intremap(void);
bool_t platform_supports_x2apic(void);
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 49df41d..c6d9a03 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -332,10 +332,11 @@ void __init platform_quirks_init(void)
* assigning Intel integrated wifi device to a guest.
*/
-static void map_me_phantom_function(struct domain *domain, u32 dev, int map)
+static int map_me_phantom_function(struct domain *domain, u32 dev, int map)
{
struct acpi_drhd_unit *drhd;
struct pci_dev *pdev;
+ int rc;
/* find ME VT-d engine base on a real ME device */
pdev = pci_get_pdev(0, 0, PCI_DEVFN(dev, 0));
@@ -343,23 +344,26 @@ static void map_me_phantom_function(struct domain *domain, u32 dev, int map)
/* map or unmap ME phantom function */
if ( map )
- domain_context_mapping_one(domain, drhd->iommu, 0,
- PCI_DEVFN(dev, 7), NULL);
+ rc = domain_context_mapping_one(domain, drhd->iommu, 0,
+ PCI_DEVFN(dev, 7), NULL);
else
- domain_context_unmap_one(domain, drhd->iommu, 0,
- PCI_DEVFN(dev, 7));
+ rc = domain_context_unmap_one(domain, drhd->iommu, 0,
+ PCI_DEVFN(dev, 7));
+
+ return rc;
}
-void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
+int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
{
u32 id;
+ int rc = 0;
id = pci_conf_read32(0, 0, 0, 0, 0);
if ( IS_CTG(id) )
{
/* quit if ME does not exist */
if ( pci_conf_read32(0, 0, 3, 0, 0) == 0xffffffff )
- return;
+ return 0;
/* if device is WLAN device, map ME phantom device 0:3.7 */
id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
@@ -373,7 +377,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
case 0x423b8086:
case 0x423c8086:
case 0x423d8086:
- map_me_phantom_function(domain, 3, map);
+ rc = map_me_phantom_function(domain, 3, map);
break;
default:
break;
@@ -383,7 +387,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
{
/* quit if ME does not exist */
if ( pci_conf_read32(0, 0, 22, 0, 0) == 0xffffffff )
- return;
+ return 0;
/* if device is WLAN device, map ME phantom device 0:22.7 */
id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
@@ -399,12 +403,14 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map)
case 0x42388086: /* Puma Peak */
case 0x422b8086:
case 0x422c8086:
- map_me_phantom_function(domain, 22, map);
+ rc = map_me_phantom_function(domain, 22, map);
break;
default:
break;
}
}
+
+ return rc;
}
void pci_vtd_quirk(const struct pci_dev *pdev)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 78+ messages in thread* Re: [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-04-18 14:00 ` [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
@ 2016-04-25 12:00 ` Jan Beulich
2016-04-26 10:48 ` Xu, Quan
0 siblings, 1 reply; 78+ messages in thread
From: Jan Beulich @ 2016-04-25 12:00 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -91,7 +91,7 @@ int is_igd_vt_enabled_quirk(void);
> void platform_quirks_init(void);
> void vtd_ops_preamble_quirk(struct iommu* iommu);
> void vtd_ops_postamble_quirk(struct iommu* iommu);
> -void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
> +int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
__must_check
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -332,10 +332,11 @@ void __init platform_quirks_init(void)
> * assigning Intel integrated wifi device to a guest.
> */
>
> -static void map_me_phantom_function(struct domain *domain, u32 dev, int map)
> +static int map_me_phantom_function(struct domain *domain, u32 dev, int map)
Again.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v2 11/11] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-04-25 12:00 ` Jan Beulich
@ 2016-04-26 10:48 ` Xu, Quan
0 siblings, 0 replies; 78+ messages in thread
From: Xu, Quan @ 2016-04-26 10:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On April 25, 2016 8:00 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -91,7 +91,7 @@ int is_igd_vt_enabled_quirk(void); void
> > platform_quirks_init(void); void vtd_ops_preamble_quirk(struct iommu*
> > iommu); void vtd_ops_postamble_quirk(struct iommu* iommu); -void
> > me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
> > +int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
>
> __must_check
>
Agreed, I will add it in next v3.
> > --- a/xen/drivers/passthrough/vtd/quirks.c
> > +++ b/xen/drivers/passthrough/vtd/quirks.c
> > @@ -332,10 +332,11 @@ void __init platform_quirks_init(void)
> > * assigning Intel integrated wifi device to a guest.
> > */
> >
> > -static void map_me_phantom_function(struct domain *domain, u32 dev,
> > int map)
> > +static int map_me_phantom_function(struct domain *domain, u32 dev,
> > +int map)
>
> Again.
>
Ditto.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 78+ messages in thread