* [PATCH v4 00/10] Check VT-d Device-TLB flush error
@ 2016-05-06 8:54 Quan Xu
2016-05-06 8:54 ` [PATCH v4 01/10] vt-d: fix the IOMMU flush issue Quan Xu
` (9 more replies)
0 siblings, 10 replies; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Quan Xu
This patch set is a prereq patch set for Patch:'VT-d Device-TLB flush issue'.
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 call trees.
This patch set is also based on the discussion of 'abstract model of IOMMU unmaping/mapping failures'
Quan Xu (10):
vt-d: fix the IOMMU flush issue
IOMMU: handle IOMMU mapping and unmapping failures
IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping.
IOMMU/MMU: propagate IOMMU Device-TLB flush error up to
iommu_iotlb_flush{,_all} (top level ones).
IOMMU: propagate IOMMU Device-TLB flush error up to
iommu_iotlb_flush{,_all} (leaf ones).
vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
vt-d: propagate error up to ME phantom function mapping and unmapping
xen/arch/arm/p2m.c | 5 +-
xen/arch/x86/acpi/power.c | 72 ++++++++---
xen/arch/x86/mm.c | 13 +-
xen/arch/x86/mm/p2m-ept.c | 43 +++++--
xen/arch/x86/mm/p2m-pt.c | 26 +++-
xen/arch/x86/mm/p2m.c | 11 +-
xen/common/memory.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 | 12 +-
xen/drivers/passthrough/iommu.c | 64 ++++++++--
xen/drivers/passthrough/vtd/extern.h | 3 +-
xen/drivers/passthrough/vtd/iommu.c | 168 ++++++++++++++++++--------
xen/drivers/passthrough/vtd/quirks.c | 28 +++--
xen/drivers/passthrough/x86/iommu.c | 5 +-
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 2 +-
xen/include/asm-x86/iommu.h | 3 +-
xen/include/xen/iommu.h | 13 +-
18 files changed, 360 insertions(+), 133 deletions(-)
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-09 16:09 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
` (8 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 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 | 99 ++++++++++++++++++++++++-------------
xen/include/asm-x86/iommu.h | 2 +-
2 files changed, 65 insertions(+), 36 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index db83949..64093a9 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -557,14 +557,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,
+ bool_t dma_old_pte_present,
+ unsigned int page_count)
{
struct domain_iommu *hd = dom_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
@@ -583,29 +585,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;
}
}
+
+ return rc;
}
-static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
+static void iommu_flush_iotlb_page(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)
+static void iommu_flush_iotlb_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 */
@@ -639,7 +646,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);
}
@@ -1278,6 +1285,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);
@@ -1391,13 +1399,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);
@@ -1407,7 +1421,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(
@@ -1502,6 +1516,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);
@@ -1529,14 +1544,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);
@@ -1545,7 +1566,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(
@@ -1734,7 +1755,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;
}
@@ -1750,14 +1771,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, bool_t present)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu = NULL;
struct domain_iommu *hd = dom_iommu(d);
int flush_dev_iotlb;
int iommu_domid;
+ int rc = 0;
iommu_flush_cache_entry(pte, sizeof(struct dma_pte));
@@ -1771,11 +1793,18 @@ 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;
+ }
}
+
+ return rc;
}
static int __init vtd_ept_page_compatible(struct iommu *iommu)
@@ -2553,8 +2582,8 @@ const struct iommu_ops intel_iommu_ops = {
.resume = vtd_resume,
.share_p2m = iommu_set_pgd,
.crash_shutdown = vtd_crash_shutdown,
- .iotlb_flush = intel_iommu_iotlb_flush,
- .iotlb_flush_all = intel_iommu_iotlb_flush_all,
+ .iotlb_flush = iommu_flush_iotlb_page,
+ .iotlb_flush_all = iommu_flush_iotlb_all,
.get_reserved_device_memory = intel_iommu_get_reserved_device_memory,
.dump_p2m_table = vtd_dump_p2m_table,
};
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index e82a2f0..43f1620 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -27,7 +27,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, bool_t 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] 64+ messages in thread
* [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
2016-05-06 8:54 ` [PATCH v4 01/10] vt-d: fix the IOMMU flush issue Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-09 16:13 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
` (7 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, dario.faggioli, Jan Beulich, Quan Xu
Treat IOMMU mapping and unmapping failures 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
xen/drivers/passthrough/iommu.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index c59b2ab..09560c0 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
const struct domain_iommu *hd = dom_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 ( unlikely(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)
{
const struct domain_iommu *hd = dom_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 ( unlikely(rc) )
+ {
+ printk(XENLOG_ERR
+ "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
+ gfn, d->domain_id);
+
+ if ( !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] 64+ messages in thread
* [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
2016-05-06 8:54 ` [PATCH v4 01/10] vt-d: fix the IOMMU flush issue Quan Xu
2016-05-06 8:54 ` [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 8:44 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
` (6 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Keir Fraser, Jan Beulich, George Dunlap,
Andrew Cooper, dario.faggioli, Jun Nakajima, Quan Xu
When IOMMU mapping is failed, we issue a best effort rollback, stopping
IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
error up to 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 an 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 | 40 ++++++++++++++++++++++++++++++++--------
xen/arch/x86/mm/p2m-pt.c | 26 ++++++++++++++++++++++----
xen/arch/x86/mm/p2m.c | 11 +++++++++--
xen/drivers/passthrough/iommu.c | 14 +++++++++++++-
5 files changed, 84 insertions(+), 20 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 2bb920b..14b54a9 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 ( !rc )
+ rc = ret;
+
return rc;
}
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 1ed5b47..814cb72 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -667,6 +667,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
unsigned long gfn_remainder = gfn;
unsigned int i, target = order / EPT_TABLE_ORDER;
int ret, rc = 0;
+ bool_t entry_written = 0;
bool_t direct_mmio = (p2mt == p2m_mmio_direct);
uint8_t ipat = 0;
bool_t need_modify_vtd_table = 1;
@@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
rc = atomic_write_ept_entry(ept_entry, new_entry, target);
if ( unlikely(rc) )
old_entry.epte = 0;
- else if ( p2mt != p2m_invalid &&
- (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
- /* Track the highest gfn for which we have ever had a valid mapping */
- p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
+ else
+ {
+ entry_written = 1;
+
+ if ( p2mt != p2m_invalid &&
+ (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
+ /* Track the highest gfn for which we have ever had a valid mapping */
+ p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
+ }
out:
if ( needs_sync )
ept_sync_domain(p2m);
/* For host p2m, may need to change VT-d page table.*/
- if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
+ if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
need_modify_vtd_table )
{
if ( iommu_hap_pt_share )
@@ -831,10 +837,28 @@ 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(p2m->domain, gfn + i);
+
+ if ( !rc )
+ rc = ret;
+
+ break;
+ }
+ }
else
for ( i = 0; i < (1 << order); i++ )
- iommu_unmap_page(d, gfn + i);
+ {
+ ret = iommu_unmap_page(d, gfn + i);
+
+ if ( !rc )
+ rc = ret;
+ }
}
}
@@ -847,7 +871,7 @@ out:
if ( is_epte_present(&old_entry) )
ept_free_entry(p2m, &old_entry, target);
- if ( rc == 0 && p2m_is_hostp2m(p2m) )
+ if ( entry_written && p2m_is_hostp2m(p2m) )
p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
return rc;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 3d80612..5426f92 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,29 @@ 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);
+
+ if ( !rc )
+ rc = ret;
+
+ break;
+ }
+ }
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 ( !rc )
+ rc = ret;
+ }
}
/*
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 94eabf6..cb77ef2 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -638,13 +638,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 rc = 0, ret;
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;
+ {
+ ret = iommu_unmap_page(p2m->domain, mfn + i);
+
+ if ( !rc )
+ rc = ret;
+ }
+
+ return rc;
}
ASSERT(gfn_locked_by_me(p2m, gfn));
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 09560c0..cca4cf3 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -171,6 +171,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);
@@ -181,10 +183,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_WARNING
+ "iommu_hwdom_init: IOMMU mapping failed for dom%d.",
+ 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] 64+ messages in thread
* [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (2 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 8:50 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 05/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
` (5 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Jan Beulich, Feng Wu, Kevin Tian, Quan Xu
Propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@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 64093a9..e0e1126 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -616,11 +616,12 @@ static void iommu_flush_iotlb_all(struct domain *d)
}
/* clear one page's page table */
-static void dma_pte_clear_one(struct domain *domain, u64 addr)
+static int __must_check dma_pte_clear_one(struct domain *domain, u64 addr)
{
struct domain_iommu *hd = dom_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 */
@@ -628,7 +629,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);
@@ -638,7 +639,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);
@@ -646,9 +647,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)
@@ -1766,9 +1769,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] 64+ messages in thread
* [PATCH v4 05/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping.
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (3 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-06 8:54 ` [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
` (4 subsequent siblings)
9 siblings, 0 replies; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Jan Beulich, Feng Wu, Kevin Tian, Quan Xu
Propagate the IOMMU Device-TLB flush error up to IOMMU mapping.
Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@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 e0e1126..584982a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1716,6 +1716,7 @@ static int intel_iommu_map_page(
struct domain_iommu *hd = dom_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) )
@@ -1758,9 +1759,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] 64+ messages in thread
* [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (4 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 05/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 9:04 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
` (3 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Quan Xu, dario.faggioli,
Julien Grall, Jan Beulich
Propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}.
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 <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
xen/arch/arm/p2m.c | 5 ++++-
xen/common/memory.c | 14 ++++++++++++--
xen/drivers/passthrough/iommu.c | 13 +++++++++----
xen/drivers/passthrough/x86/iommu.c | 5 +++--
xen/include/xen/iommu.h | 5 +++--
5 files changed, 31 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index db21433..fe201d0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1178,7 +1178,10 @@ out:
if ( flush )
{
flush_tlb_domain(d);
- iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+ ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+
+ if ( !rc )
+ rc = ret;
}
while ( (pg = page_list_remove_head(&free_pages)) )
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 644f81a..21b8098 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -677,9 +677,19 @@ static int xenmem_add_to_physmap(struct domain *d,
#ifdef CONFIG_HAS_PASSTHROUGH
if ( need_iommu(d) )
{
+ int ret;
+
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 && unlikely(ret) )
+ rc = ret;
+
+ ret = iommu_iotlb_flush(d, xatp->gpfn - done, done);
+
+ if ( rc >= 0 && unlikely(ret) )
+ rc = ret;
}
#endif
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index cca4cf3..7b5fc59 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -312,24 +312,29 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
+ unsigned int page_count)
{
const struct domain_iommu *hd = dom_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 __must_check iommu_iotlb_flush_all(struct domain *d)
{
const struct domain_iommu *hd = dom_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 b64b98f..a18a608 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 19ba976..75c17a7 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -200,8 +200,9 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
+ unsigned int page_count);
+int __must_check 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] 64+ messages in thread
* [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (5 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 9:06 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update Quan Xu
` (2 subsequent siblings)
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, Quan Xu, dario.faggioli,
Julien Grall, Jan Beulich
Propagate the IOMMU Device-TLB flush error up to the iommu_iotlb_flush{,_all}.
This patch fixes the leaf ones.
Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/drivers/passthrough/arm/smmu.c | 12 +++++++-----
xen/drivers/passthrough/iommu.c | 8 ++------
xen/drivers/passthrough/vtd/iommu.c | 10 +++++-----
xen/include/xen/iommu.h | 4 ++--
4 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 54a03a6..1864b6d 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 = dom_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);
+ /* ARM SMMU v1 doesn't have flush by VMA and VMID */
+ 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 7b5fc59..c013d91 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -320,9 +320,7 @@ int __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
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 __must_check iommu_iotlb_flush_all(struct domain *d)
@@ -332,9 +330,7 @@ int __must_check 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 584982a..a749c67 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
return rc;
}
-static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
- unsigned int page_count)
+static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
+static int iommu_flush_iotlb_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 75c17a7..6a64014 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -179,8 +179,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] 64+ messages in thread
* [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (6 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 9:09 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
2016-05-06 8:54 ` [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Feng Wu, Jun Nakajima, George Dunlap, Andrew Cooper,
dario.faggioli, Jan Beulich, Quan Xu
Propagate the IOMMU Device-TLB flush error up to the ept_set_entry(),
when VT-d shares EPT page table.
Signed-off-by: Quan Xu <quan.xu@intel.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Feng Wu <feng.wu@intel.com>
---
xen/arch/x86/mm/p2m-ept.c | 3 ++-
xen/drivers/passthrough/vtd/iommu.c | 4 ++--
xen/include/asm-x86/iommu.h | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 814cb72..34b96f8 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -832,7 +832,8 @@ out:
need_modify_vtd_table )
{
if ( iommu_hap_pt_share )
- iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
+ ret = iommu_pte_flush(d, gfn, &ept_entry->epte,
+ order, vtd_pte_present);
else
{
if ( iommu_flags )
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index a749c67..bf77417 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1773,8 +1773,8 @@ static int intel_iommu_unmap_page(struct domain *d, unsigned long gfn)
return dma_pte_clear_one(d, (paddr_t)gfn << PAGE_SHIFT_4K);
}
-int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
- int order, bool_t present)
+int __must_check iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
+ int order, bool_t present)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu = NULL;
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 43f1620..3d2c354 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -27,7 +27,8 @@ 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);
-int iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte, int order, bool_t present);
+int __must_check iommu_pte_flush(struct domain *d, u64 gfn, u64 *pte,
+ int order, bool_t 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] 64+ messages in thread
* [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (7 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 9:24 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Keir Fraser, Quan Xu,
Liu Jinsong, dario.faggioli, Julien Grall, Jan Beulich,
Andrew Cooper, Feng Wu, Suravee Suthikulpanit
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 <sstabellini@kernel.org>
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 | 72 ++++++++++++++++++++-------
xen/drivers/passthrough/amd/iommu_init.c | 9 +++-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +-
xen/drivers/passthrough/iommu.c | 7 ++-
xen/drivers/passthrough/vtd/iommu.c | 39 +++++++++++----
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 2 +-
xen/include/xen/iommu.h | 4 +-
7 files changed, 101 insertions(+), 34 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 2885e31..1f7b8bf 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -43,36 +43,71 @@ struct acpi_sleep_info acpi_sinfo;
void do_suspend_lowlevel(void);
+enum dev_power_type
+{
+ /* Use for all of device power type */
+ TYPE_ALL,
+ TYPE_CONSOLE,
+ TYPE_TIME,
+ TYPE_I8259A,
+ TYPE_IOAPIC,
+ TYPE_IOMMU,
+ TYPE_LAPIC,
+ /* Use for error */
+ TYPE_UNKNOWN
+};
+
static int device_power_down(void)
{
- console_suspend();
+ if ( console_suspend() )
+ return TYPE_CONSOLE;
- time_suspend();
+ if ( time_suspend() )
+ return TYPE_TIME;
- i8259A_suspend();
+ if ( i8259A_suspend() )
+ return TYPE_I8259A;
+ /* ioapic_suspend should never fail */
ioapic_suspend();
- iommu_suspend();
+ if ( iommu_suspend() )
+ return TYPE_IOMMU;
- lapic_suspend();
+ if ( lapic_suspend() )
+ return TYPE_LAPIC;
return 0;
}
-static void device_power_up(void)
+static void device_power_up(enum dev_power_type type)
{
- lapic_resume();
-
- iommu_resume();
-
- ioapic_resume();
-
- i8259A_resume();
-
- time_resume();
-
- console_resume();
+ switch ( type )
+ {
+ case TYPE_ALL:
+ /* fall through */
+ case TYPE_LAPIC:
+ lapic_resume();
+ /* fall through */
+ case TYPE_IOMMU:
+ iommu_resume();
+ /* fall through */
+ case TYPE_IOAPIC:
+ ioapic_resume();
+ /* fall through */
+ case TYPE_I8259A:
+ i8259A_resume();
+ /* fall through */
+ case TYPE_TIME:
+ time_resume();
+ /* fall through */
+ case TYPE_CONSOLE:
+ console_resume();
+ break;
+ default:
+ BUG();
+ break;
+ }
}
static void freeze_domains(void)
@@ -169,6 +204,7 @@ static int enter_state(u32 state)
{
printk(XENLOG_ERR "Some devices failed to power down.");
system_state = SYS_STATE_resume;
+ device_power_up(error);
goto done;
}
@@ -196,7 +232,7 @@ static int enter_state(u32 state)
write_cr4(cr4 & ~X86_CR4_MCE);
write_efer(read_efer());
- device_power_up();
+ device_power_up(TYPE_ALL);
mcheck_init(&boot_cpu_data, 0);
write_cr4(cr4);
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 70b7475..ac40f63 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -626,6 +626,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/iommu.c b/xen/drivers/passthrough/iommu.c
index c013d91..8b26b1c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -404,11 +404,14 @@ int iommu_do_domctl(
return ret;
}
-void iommu_suspend()
+int __must_check 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 bf77417..29cf870 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -541,11 +541,12 @@ static int iommu_flush_iotlb_psi(
return status;
}
-static void iommu_flush_all(void)
+static int __must_check iommu_flush_all(void)
{
struct acpi_drhd_unit *drhd;
struct iommu *iommu;
int flush_dev_iotlb;
+ int rc = 0, ret;
flush_all_cache();
for_each_drhd_unit ( drhd )
@@ -553,8 +554,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);
+ ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+
+ if ( ret )
+ rc = ret;
}
+
+ return rc;
}
static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
@@ -1267,7 +1273,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
setup_hwdom_pci_devices(d, setup_hwdom_device);
setup_hwdom_rmrr(d);
- iommu_flush_all();
+ if ( iommu_flush_all() )
+ printk(XENLOG_WARNING VTDPREFIX
+ " intel_iommu_hwdom_init: IOMMU flush all failed.\n");
for_each_drhd_unit ( drhd )
{
@@ -2126,8 +2134,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)
@@ -2416,16 +2424,25 @@ 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 )
+ {
+ printk(XENLOG_WARNING VTDPREFIX
+ " vtd_suspend: IOMMU flush all failed.\n");
+ return rc;
+ }
for_each_drhd_unit ( drhd )
{
@@ -2454,6 +2471,8 @@ static void vtd_suspend(void)
if ( !iommu_intremap && iommu_qinval )
disable_qinval(iommu);
}
+
+ return 0;
}
static void vtd_crash_shutdown(void)
@@ -2464,7 +2483,9 @@ static void vtd_crash_shutdown(void)
if ( !iommu_enabled )
return;
- iommu_flush_all();
+ if ( iommu_flush_all() )
+ printk(XENLOG_WARNING VTDPREFIX
+ " vtd_crash_shutdown: IOMMU flush all failed.\n");
for_each_drhd_unit ( drhd )
{
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 6a64014..fee5a8f 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -175,7 +175,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);
@@ -185,7 +185,7 @@ struct iommu_ops {
void (*dump_p2m_table)(struct domain *d);
};
-void iommu_suspend(void);
+int __must_check 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] 64+ messages in thread
* [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
` (8 preceding siblings ...)
2016-05-06 8:54 ` [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
@ 2016-05-06 8:54 ` Quan Xu
2016-05-10 9:29 ` Jan Beulich
9 siblings, 1 reply; 64+ messages in thread
From: Quan Xu @ 2016-05-06 8:54 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, dario.faggioli, Feng Wu, Jan Beulich, Quan Xu
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 | 3 ++-
xen/drivers/passthrough/vtd/iommu.c | 18 ++++++++++++++----
xen/drivers/passthrough/vtd/quirks.c | 28 ++++++++++++++++++----------
3 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h
index cbe0286..6772839 100644
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -91,7 +91,8 @@ 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 __must_check 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/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 29cf870..ac81c8a 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1296,7 +1296,7 @@ int domain_context_mapping_one(
u64 maddr, pgd_maddr;
u16 seg = iommu->intel->drhd->segment;
int agaw;
- int rc;
+ int rc, ret;
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
@@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
unmap_vtd_domain_page(context_entries);
if ( !seg )
- me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+ {
+ ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+
+ if ( !rc )
+ rc = ret;
+ }
return rc;
}
@@ -1527,7 +1532,7 @@ int domain_context_unmap_one(
struct context_entry *context, *context_entries;
u64 maddr;
int iommu_domid;
- int rc;
+ int rc, ret;
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
@@ -1575,7 +1580,12 @@ int domain_context_unmap_one(
unmap_vtd_domain_page(context_entries);
if ( !iommu->intel->drhd->segment )
- me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
+ {
+ ret = me_wifi_quirk(domain, bus, devfn, UNMAP_ME_PHANTOM_FUNC);
+
+ if ( !rc )
+ rc = ret;
+ }
return rc;
}
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 473d1fc..e37060c 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -331,10 +331,12 @@ 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 __must_check 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));
@@ -342,23 +344,27 @@ 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 __must_check 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);
@@ -372,7 +378,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;
@@ -382,7 +388,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);
@@ -398,12 +404,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] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-06 8:54 ` [PATCH v4 01/10] vt-d: fix the IOMMU flush issue Quan Xu
@ 2016-05-09 16:09 ` Jan Beulich
2016-05-12 7:50 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-09 16:09 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Feng Wu
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count)
> +static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> + unsigned int page_count)
The new name suggests just one page. Please use e.g.
iommu_flush_iotlb_pages() instead.
> {
> - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> + iommu_flush_iotlb(d, gfn, 1, page_count);
> }
But of course the question is whether having this wrapper is useful in
the first place, the more that ...
> @@ -639,7 +646,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);
... it's being open coded here. IOW if you want to retain the
wrapper, please use it here.
> @@ -1391,13 +1399,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);
Please take the opportunity and add the missing blank line (between
declaration(s) and statement(s) in cases like this.
> + }
> +
> + if ( rc > 0 )
Can iommu_flush_context_device() return a positive value? If so,
the logic is now likely wrong. If not (which is what I assume) I'd
like to suggest adding a respective ASSERT() (even if only to
document the fact). Or alternatively this if() could move into the
immediately preceding one.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-06 8:54 ` [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
@ 2016-05-09 16:13 ` Jan Beulich
2016-05-10 3:41 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-09 16:13 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
> unsigned int flags)
> {
> const struct domain_iommu *hd = dom_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 ( unlikely(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);
> + }
This still may spam the console in at least the case of Dom0. For
DomU I'd really expect you to state in the commit message why no
spamming can occur (of course assuming it really can't, which I'm
not convinced of).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-09 16:13 ` Jan Beulich
@ 2016-05-10 3:41 ` Xu, Quan
2016-05-10 6:53 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-10 3:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Tian, Kevin, xen-devel
On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d,
> unsigned long gfn, unsigned long mfn,
> > unsigned int flags) {
> > const struct domain_iommu *hd = dom_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 ( unlikely(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);
> > + }
>
> This still may spam the console in at least the case of Dom0.
I am afraid we may need a minor trade-off. What about:
dprintk(XENLOG_ERR, "...");
to print out in debug mode.
> For DomU I'd
> really expect you to state in the commit message why no spamming can occur
> (of course assuming it really can't, which I'm not convinced of).
>
In this v4, I think we will still spam the console in extreme cases :(:(..
For mapping:
+ ret = iommu_map_page();
+ if ( unlikely(ret) )
+ {
+ while ( i-- )
+ iommu_unmap_page();
+ }
We'll stop map against any error and unmapping the previous mappings. The extreme case is error for unmapping the previous mappings.
Again -- I think dprintk is a better solution. Any suggestion?
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 3:41 ` Xu, Quan
@ 2016-05-10 6:53 ` Jan Beulich
2016-05-10 7:53 ` Xu, Quan
2016-05-12 14:28 ` Xu, Quan
0 siblings, 2 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 6:53 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d,
>> unsigned long gfn, unsigned long mfn,
>> > unsigned int flags) {
>> > const struct domain_iommu *hd = dom_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 ( unlikely(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);
>> > + }
>>
>> This still may spam the console in at least the case of Dom0.
>
> I am afraid we may need a minor trade-off. What about:
>
> dprintk(XENLOG_ERR, "...");
>
> to print out in debug mode.
And be silent in non-debug mode? That's not what we want.
>> For DomU I'd
>> really expect you to state in the commit message why no spamming can occur
>> (of course assuming it really can't, which I'm not convinced of).
>>
>
> In this v4, I think we will still spam the console in extreme cases :(:(..
>
> For mapping:
> + ret = iommu_map_page();
> + if ( unlikely(ret) )
> + {
> + while ( i-- )
> + iommu_unmap_page();
> + }
>
> We'll stop map against any error and unmapping the previous mappings. The
> extreme case is error for unmapping the previous mappings.
>
> Again -- I think dprintk is a better solution. Any suggestion?
For DomU the solution seems quite obvious: Only log a message if
the domain is not already marked crashed. For Dom0 you'll need to
get a little more creative (but by leveraging the fact that there's
only one in the system, this can't be too difficult a problem to solve:
e.g. "manually" rate limit these messages - see printk_ratelimit() et
al).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 6:53 ` Jan Beulich
@ 2016-05-10 7:53 ` Xu, Quan
2016-05-10 8:02 ` Jan Beulich
2016-05-12 14:28 ` Xu, Quan
1 sibling, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-10 7:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Tian, Kevin, xen-devel
On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/iommu.c
> >> > +++ b/xen/drivers/passthrough/iommu.c
> >> > @@ -240,21 +240,47 @@ int iommu_map_page(struct domain *d,
> >> unsigned long gfn, unsigned long mfn,
> >> > unsigned int flags) {
> >> > const struct domain_iommu *hd = dom_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 ( unlikely(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);
> >> > + }
> >>
> >> This still may spam the console in at least the case of Dom0.
> >
> > I am afraid we may need a minor trade-off. What about:
> >
> > dprintk(XENLOG_ERR, "...");
> >
> > to print out in debug mode.
>
> And be silent in non-debug mode? That's not what we want.
>
Without your below suggestion, this is really my best solution.
> >> For DomU I'd
> >> really expect you to state in the commit message why no spamming can
> >> occur (of course assuming it really can't, which I'm not convinced of).
> >>
> >
> > In this v4, I think we will still spam the console in extreme cases :(:(..
> >
> > For mapping:
> > + ret = iommu_map_page();
> > + if ( unlikely(ret) )
> > + {
> > + while ( i-- )
> > + iommu_unmap_page();
> > + }
> >
> > We'll stop map against any error and unmapping the previous mappings.
> > The extreme case is error for unmapping the previous mappings.
> >
> > Again -- I think dprintk is a better solution. Any suggestion?
>
> For DomU the solution seems quite obvious: Only log a message if the domain
> is not already marked crashed. For Dom0 you'll need to get a little more
> creative (but by leveraging the fact that there's only one in the system, this
> can't be too difficult a problem to solve:
> e.g. "manually" rate limit these messages - see printk_ratelimit() et al).
Amazing!!
As the comment said, printk_ratelimit() is lifted from Linux. referred to the Linux, __iiuc__ , I will fix this issue as below (a variant):
...
+ rc = hd->platform_ops->unmap_page(d, gfn);
+
+ if ( unlikely(rc) )
+ {
+ if ( printk_ratelimit() )
+ printk(XENLOG_ERR
+ "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
+ gfn, d->domain_id);
+
+ if ( !is_hardware_domain(d) )
+ domain_crash(d);
+ }
+
+ return rc;
...
Thanks!!
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 7:53 ` Xu, Quan
@ 2016-05-10 8:02 ` Jan Beulich
2016-05-10 8:20 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 8:02 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 10.05.16 at 09:53, <quan.xu@intel.com> wrote:
> + rc = hd->platform_ops->unmap_page(d, gfn);
> +
> + if ( unlikely(rc) )
> + {
> + if ( printk_ratelimit() )
> + printk(XENLOG_ERR
> + "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
> + gfn, d->domain_id);
> +
> + if ( !is_hardware_domain(d) )
> + domain_crash(d);
> + }
> +
> + return rc;
But please - as said - also avoid logging any message for already
dying domains.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 8:02 ` Jan Beulich
@ 2016-05-10 8:20 ` Xu, Quan
2016-05-10 8:26 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-10 8:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Tian, Kevin, xen-devel
On May 10, 2016 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 10.05.16 at 09:53, <quan.xu@intel.com> wrote:
> > + rc = hd->platform_ops->unmap_page(d, gfn);
> > +
> > + if ( unlikely(rc) )
> > + {
> > + if ( printk_ratelimit() )
> > + printk(XENLOG_ERR
> > + "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for
> dom%d.",
> > + gfn, d->domain_id);
> > +
> > + if ( !is_hardware_domain(d) )
> > + domain_crash(d);
> > + }
> > +
> > + return rc;
>
> But please - as said - also avoid logging any message for already dying
> domains.
>
Kept Kevin's opinion for later, I hope I have got your point as below:
...
+ rc = hd->platform_ops->unmap_page(d, gfn);
+
+ if ( unlikely(rc) )
+ {
+ if ( is_hardware_domain(d) )
+ if ( printk_ratelimit() )
+ printk(XENLOG_ERR
+ "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
+ gfn, d->domain_id);
+ else
+ domain_crash(d);
+ }
+
+ return rc;
...
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] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 8:20 ` Xu, Quan
@ 2016-05-10 8:26 ` Jan Beulich
0 siblings, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 8:26 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 10.05.16 at 10:20, <quan.xu@intel.com> wrote:
> On May 10, 2016 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> But please - as said - also avoid logging any message for already dying
>> domains.
>>
>
>
> Kept Kevin's opinion for later, I hope I have got your point as below:
> ...
> + rc = hd->platform_ops->unmap_page(d, gfn);
> +
> + if ( unlikely(rc) )
> + {
> + if ( is_hardware_domain(d) )
> + if ( printk_ratelimit() )
> + printk(XENLOG_ERR
> + "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.",
> + gfn, d->domain_id);
> + else
> + domain_crash(d);
> + }
> +
> + return rc;
> ...
I don't see how this would address my previous comment (not to
speak of the "else" now being associated with the wrong "if").
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-06 8:54 ` [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
@ 2016-05-10 8:44 ` Jan Beulich
2016-05-10 14:45 ` George Dunlap
2016-05-11 3:39 ` Xu, Quan
0 siblings, 2 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 8:44 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> When IOMMU mapping is failed, we issue a best effort rollback, stopping
> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
> error up to 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 an 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>
> ---
Somewhere here I continue to miss a summary on what has changed
compared to the previous version. For review especially of larger
patches (where preferably one wouldn't want to re-review the entire
thing) this is more than just a nice-to-have.
> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> rc = atomic_write_ept_entry(ept_entry, new_entry, target);
> if ( unlikely(rc) )
> old_entry.epte = 0;
> - else if ( p2mt != p2m_invalid &&
> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> - /* Track the highest gfn for which we have ever had a valid mapping */
> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> + else
> + {
> + entry_written = 1;
> +
> + if ( p2mt != p2m_invalid &&
> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> + /* Track the highest gfn for which we have ever had a valid mapping */
> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> + }
>
> out:
> if ( needs_sync )
> ept_sync_domain(p2m);
>
> /* For host p2m, may need to change VT-d page table.*/
> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> need_modify_vtd_table )
> {
I'd prefer this conditional to remain untouched, but I'll leave the
decision to the maintainers of the file.
> @@ -831,10 +837,28 @@ 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(p2m->domain, gfn + i);
Loops like this are, btw., good examples of the log spam issue I've
been pointing out on an earlier patch of this series.
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -638,13 +638,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 rc = 0, ret;
>
> 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;
> + {
> + ret = iommu_unmap_page(p2m->domain, mfn + i);
> +
> + if ( !rc )
> + rc = ret;
> + }
> +
> + return rc;
> }
In code like this, btw., restricting the scope of "ret" to the innermost
block would help future readers see immediately that the value of
"ret" is of no further interest outside of that block.
Having reached the end of the patch, I'm missing the __must_check
additions that you said you would do in this new iteration. Is there
any reason for their absence? Did I overlook something?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-05-06 8:54 ` [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
@ 2016-05-10 8:50 ` Jan Beulich
2016-05-11 3:49 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 8:50 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> Propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
>
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but note ...
> @@ -1766,9 +1769,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);
> }
... how you lose the __must_check here, since
intel_iommu_unmap_page() isn't __must_check (which we said you
may skip as long as the common code wrapper has it, but in the
context here I'm no longer convinced skipping this at any layer is a
good idea, as that makes validation of the call trees more difficult).
(This is just a remark regarding the comment on the earlier patch,
i.e. not something needing any further change here.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-05-06 8:54 ` [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
@ 2016-05-10 9:04 ` Jan Beulich
2016-05-11 5:52 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 9:04 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -677,9 +677,19 @@ static int xenmem_add_to_physmap(struct domain *d,
> #ifdef CONFIG_HAS_PASSTHROUGH
> if ( need_iommu(d) )
> {
> + int ret;
> +
> 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 && unlikely(ret) )
> + rc = ret;
> +
> + ret = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> +
> + if ( rc >= 0 && unlikely(ret) )
> + rc = ret;
In both cases I think it would be preferable to switch the two sides
of the &&.
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -312,24 +312,29 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> + unsigned int page_count)
The annotation generally belongs on declarations; the only exception
are static functions which don't have any (i.e. they get defined before
first getting used).
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -200,8 +200,9 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> + unsigned int page_count);
> +int __must_check iommu_iotlb_flush_all(struct domain *d);
I.e. these changes suffice, repeating the annotations on the
function definitions is redundant with the ones put here.
With respective adjustments done
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-06 8:54 ` [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
@ 2016-05-10 9:06 ` Jan Beulich
2016-05-11 6:47 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 9:06 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, dario.faggioli,
xen-devel, Julien Grall
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d, unsigned long gfn,
> return rc;
> }
>
> -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> - unsigned int page_count)
> +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
> +static int iommu_flush_iotlb_all(struct domain *d)
> {
> - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> }
As already indicated in a reply to an earlier patch, despite what was
said on the earlier version I think we should have __must_check here
and ...
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -179,8 +179,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);
... here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
2016-05-06 8:54 ` [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update Quan Xu
@ 2016-05-10 9:09 ` Jan Beulich
2016-05-10 14:58 ` George Dunlap
2016-05-11 7:25 ` Xu, Quan
0 siblings, 2 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 9:09 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Feng Wu, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -832,7 +832,8 @@ out:
> need_modify_vtd_table )
> {
> if ( iommu_hap_pt_share )
> - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
> + ret = iommu_pte_flush(d, gfn, &ept_entry->epte,
> + order, vtd_pte_present);
> else
> {
> if ( iommu_flags )
Looking at this in conjunction with patch 3, I can't see where "ret"
would get consumed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-05-06 8:54 ` [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
@ 2016-05-10 9:24 ` Jan Beulich
2016-05-13 3:39 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 9:24 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, Liu Jinsong,
dario.faggioli, xen-devel, Julien Grall, Suravee Suthikulpanit,
Andrew Cooper, Keir Fraser
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -43,36 +43,71 @@ struct acpi_sleep_info acpi_sinfo;
>
> void do_suspend_lowlevel(void);
>
> +enum dev_power_type
> +{
> + /* Use for all of device power type */
No sure what this comment is meant to say. Also if you mean it to
apply to just TYPE_ALL, it would probably better be placed on that
same line. In any event it is missing a full stop.
> + TYPE_ALL,
> + TYPE_CONSOLE,
> + TYPE_TIME,
> + TYPE_I8259A,
> + TYPE_IOAPIC,
> + TYPE_IOMMU,
> + TYPE_LAPIC,
> + /* Use for error */
Same here, but ...
> + TYPE_UNKNOWN
... this isn't used anywhere anyway, and hence should be left out.
Errors would better be reported via -E... values anyway.
> static int device_power_down(void)
> {
> - console_suspend();
> + if ( console_suspend() )
> + return TYPE_CONSOLE;
This (together with the resume side) makes me guess that the use
of TYPE_ as a prefix confused not just me, but also you: Here you
want to tell the caller that everything _prior_ to console suspend
(which happens to be nothing in this specific case) needs to be
undone. Yet below you use TYPE_CONSOLE as an indication that
console_resume() needs to be called.
> - time_suspend();
> + if ( time_suspend() )
> + return TYPE_TIME;
>
> - i8259A_suspend();
> + if ( i8259A_suspend() )
> + return TYPE_I8259A;
>
> + /* ioapic_suspend should never fail */
> ioapic_suspend();
The comment is bogus: "should" means it can in theory. Yet the
function having void return type means it just cannot fail.
> -static void device_power_up(void)
> +static void device_power_up(enum dev_power_type type)
> {
> - lapic_resume();
> -
> - iommu_resume();
> -
> - ioapic_resume();
> -
> - i8259A_resume();
> -
> - time_resume();
> -
> - console_resume();
> + switch ( type )
> + {
> + case TYPE_ALL:
> + /* fall through */
> + case TYPE_LAPIC:
No need for a fall-through comment between immediately adjacent
case labels.
> @@ -169,6 +204,7 @@ static int enter_state(u32 state)
> {
> printk(XENLOG_ERR "Some devices failed to power down.");
> system_state = SYS_STATE_resume;
> + device_power_up(error);
Either error's and device_power_down()'s return type get changed
to enum dev_power_type, or this needs a "error > 0" conditional.
> @@ -1267,7 +1273,9 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
> setup_hwdom_pci_devices(d, setup_hwdom_device);
> setup_hwdom_rmrr(d);
>
> - iommu_flush_all();
> + if ( iommu_flush_all() )
> + printk(XENLOG_WARNING VTDPREFIX
> + " intel_iommu_hwdom_init: IOMMU flush all failed.\n");
As said (and I think a number of times) before: At least when you
already hold the error value in your hands, please also log it. Also
personally I'm opposed to including function names in non-debug
log messages, but I'll leave that decision to the VT-d maintainers
here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-06 8:54 ` [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
@ 2016-05-10 9:29 ` Jan Beulich
2016-05-11 8:35 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 9:29 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
> unmap_vtd_domain_page(context_entries);
>
> if ( !seg )
> - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> + {
> + ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> +
> + if ( !rc )
> + rc = ret;
> + }
Is there any use in calling this function if an earlier error occurred?
If not, the change can be more lightweight (while in the unmap case
it should probably stay as is, to fit the "best effort" theme).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 8:44 ` Jan Beulich
@ 2016-05-10 14:45 ` George Dunlap
2016-05-10 14:59 ` George Dunlap
` (2 more replies)
2016-05-11 3:39 ` Xu, Quan
1 sibling, 3 replies; 64+ messages in thread
From: George Dunlap @ 2016-05-10 14:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Keir Fraser, Jun Nakajima, Andrew Cooper,
Dario Faggioli, xen-devel, Quan Xu
On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> When IOMMU mapping is failed, we issue a best effort rollback, stopping
>> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
>> error up to 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 an 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>
>> ---
>
> Somewhere here I continue to miss a summary on what has changed
> compared to the previous version. For review especially of larger
> patches (where preferably one wouldn't want to re-review the entire
> thing) this is more than just a nice-to-have.
>
>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>> rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>> if ( unlikely(rc) )
>> old_entry.epte = 0;
>> - else if ( p2mt != p2m_invalid &&
>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>> - /* Track the highest gfn for which we have ever had a valid mapping */
>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>> + else
>> + {
>> + entry_written = 1;
>> +
>> + if ( p2mt != p2m_invalid &&
>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>> + /* Track the highest gfn for which we have ever had a valid mapping */
>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>> + }
>>
>> out:
>> if ( needs_sync )
>> ept_sync_domain(p2m);
>>
>> /* For host p2m, may need to change VT-d page table.*/
>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>> need_modify_vtd_table )
>> {
>
> I'd prefer this conditional to remain untouched, but I'll leave the
> decision to the maintainers of the file.
Any particular reason you think it would be better untouched?
I asked for it to be changed to "entry_written", because it seemed to
me that's what was actually wanted (i.e., you're checking whether rc
== 0 to determine whether the entry was written or not). At the
moment the checks will be identical, but if someone changed something
later, rc might be non-zero even though the entry had been written, in
which case (I think) you'd want the iommu update to happen.
It's not that big a deal to me, but I do prefer it this way (unless
I've misunderstood something).
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -638,13 +638,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 rc = 0, ret;
>>
>> 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;
>> + {
>> + ret = iommu_unmap_page(p2m->domain, mfn + i);
>> +
>> + if ( !rc )
>> + rc = ret;
>> + }
>> +
>> + return rc;
>> }
>
> In code like this, btw., restricting the scope of "ret" to the innermost
> block would help future readers see immediately that the value of
> "ret" is of no further interest outside of that block.
I wouldn't ask for re-send just for this, but...
> Having reached the end of the patch, I'm missing the __must_check
> additions that you said you would do in this new iteration. Is there
> any reason for their absence? Did I overlook something?
If it's going to be re-sent anyway, moving the ret declaration inside
the loop might as well be done.
Other than that, it looks good to me, thanks.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
2016-05-10 9:09 ` Jan Beulich
@ 2016-05-10 14:58 ` George Dunlap
2016-05-10 15:04 ` Jan Beulich
2016-05-11 7:25 ` Xu, Quan
1 sibling, 1 reply; 64+ messages in thread
From: George Dunlap @ 2016-05-10 14:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Feng Wu, Jun Nakajima, Andrew Cooper, Dario Faggioli,
xen-devel, Quan Xu
On Tue, May 10, 2016 at 10:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -832,7 +832,8 @@ out:
>> need_modify_vtd_table )
>> {
>> if ( iommu_hap_pt_share )
>> - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
>> + ret = iommu_pte_flush(d, gfn, &ept_entry->epte,
>> + order, vtd_pte_present);
>> else
>> {
>> if ( iommu_flags )
>
> Looking at this in conjunction with patch 3, I can't see where "ret"
> would get consumed.
Hmm, and here I see where "rc == 0" might be a better option than
"entry_written".
If we know rc is zero, we can just use rc here instead of 'ret', and I
think everything falls out.
If rc is not zero, then we have to do this "if ( !rc ) rc = ret;"
business, which seems a bit silly to do when we know it's zero and
don't expect that to change.
On the other hand, using rc *without* actually checking that it's zero
seems like asking for trouble.
So perhaps it would be better if we take your advice for patch 3, and
then use 'rc' here?
Everything else looks good.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 14:45 ` George Dunlap
@ 2016-05-10 14:59 ` George Dunlap
2016-05-11 2:26 ` Xu, Quan
2016-05-10 15:02 ` Jan Beulich
2016-05-11 2:29 ` Xu, Quan
2 siblings, 1 reply; 64+ messages in thread
From: George Dunlap @ 2016-05-10 14:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Keir Fraser, Jun Nakajima, Andrew Cooper,
Dario Faggioli, xen-devel, Quan Xu
On Tue, May 10, 2016 at 3:45 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>>> When IOMMU mapping is failed, we issue a best effort rollback, stopping
>>> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
>>> error up to 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 an 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>
>>> ---
>>
>> Somewhere here I continue to miss a summary on what has changed
>> compared to the previous version. For review especially of larger
>> patches (where preferably one wouldn't want to re-review the entire
>> thing) this is more than just a nice-to-have.
>>
>>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>>> rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>>> if ( unlikely(rc) )
>>> old_entry.epte = 0;
>>> - else if ( p2mt != p2m_invalid &&
>>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>>> - /* Track the highest gfn for which we have ever had a valid mapping */
>>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>>> + else
>>> + {
>>> + entry_written = 1;
>>> +
>>> + if ( p2mt != p2m_invalid &&
>>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>>> + /* Track the highest gfn for which we have ever had a valid mapping */
>>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>>> + }
>>>
>>> out:
>>> if ( needs_sync )
>>> ept_sync_domain(p2m);
>>>
>>> /* For host p2m, may need to change VT-d page table.*/
>>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>>> need_modify_vtd_table )
>>> {
>>
>> I'd prefer this conditional to remain untouched, but I'll leave the
>> decision to the maintainers of the file.
>
> Any particular reason you think it would be better untouched?
>
> I asked for it to be changed to "entry_written", because it seemed to
> me that's what was actually wanted (i.e., you're checking whether rc
> == 0 to determine whether the entry was written or not). At the
> moment the checks will be identical, but if someone changed something
> later, rc might be non-zero even though the entry had been written, in
> which case (I think) you'd want the iommu update to happen.
>
> It's not that big a deal to me, but I do prefer it this way (unless
> I've misunderstood something).
See the discussion on patch 8 regarding why I now think Jan is probably right.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 14:45 ` George Dunlap
2016-05-10 14:59 ` George Dunlap
@ 2016-05-10 15:02 ` Jan Beulich
2016-05-11 2:29 ` Xu, Quan
2 siblings, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 15:02 UTC (permalink / raw)
To: George Dunlap
Cc: Kevin Tian, Keir Fraser, Jun Nakajima, Andrew Cooper,
Dario Faggioli, xen-devel, Quan Xu
>>> On 10.05.16 at 16:45, <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>>> rc = atomic_write_ept_entry(ept_entry, new_entry, target);
>>> if ( unlikely(rc) )
>>> old_entry.epte = 0;
>>> - else if ( p2mt != p2m_invalid &&
>>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>>> - /* Track the highest gfn for which we have ever had a valid mapping */
>>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>>> + else
>>> + {
>>> + entry_written = 1;
>>> +
>>> + if ( p2mt != p2m_invalid &&
>>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>>> + /* Track the highest gfn for which we have ever had a valid mapping */
>>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
>>> + }
>>>
>>> out:
>>> if ( needs_sync )
>>> ept_sync_domain(p2m);
>>>
>>> /* For host p2m, may need to change VT-d page table.*/
>>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
>>> need_modify_vtd_table )
>>> {
>>
>> I'd prefer this conditional to remain untouched, but I'll leave the
>> decision to the maintainers of the file.
>
> Any particular reason you think it would be better untouched?
IMO it's misleading here, and appropriate only in the other place
(where the altp2m-s get updated).
> I asked for it to be changed to "entry_written", because it seemed to
> me that's what was actually wanted (i.e., you're checking whether rc
> == 0 to determine whether the entry was written or not). At the
> moment the checks will be identical, but if someone changed something
> later, rc might be non-zero even though the entry had been written, in
> which case (I think) you'd want the iommu update to happen.
>
> It's not that big a deal to me, but I do prefer it this way (unless
> I've misunderstood something).
>
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -638,13 +638,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 rc = 0, ret;
>>>
>>> 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;
>>> + {
>>> + ret = iommu_unmap_page(p2m->domain, mfn + i);
>>> +
>>> + if ( !rc )
>>> + rc = ret;
>>> + }
>>> +
>>> + return rc;
>>> }
>>
>> In code like this, btw., restricting the scope of "ret" to the innermost
>> block would help future readers see immediately that the value of
>> "ret" is of no further interest outside of that block.
>
> I wouldn't ask for re-send just for this, but...
I agree, and I meant to express this with the "btw".
Jan
>> Having reached the end of the patch, I'm missing the __must_check
>> additions that you said you would do in this new iteration. Is there
>> any reason for their absence? Did I overlook something?
>
> If it's going to be re-sent anyway, moving the ret declaration inside
> the loop might as well be done.
>
> Other than that, it looks good to me, thanks.
>
> -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
2016-05-10 14:58 ` George Dunlap
@ 2016-05-10 15:04 ` Jan Beulich
0 siblings, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-10 15:04 UTC (permalink / raw)
To: George Dunlap
Cc: Kevin Tian, Feng Wu, Jun Nakajima, Andrew Cooper, Dario Faggioli,
xen-devel, Quan Xu
>>> On 10.05.16 at 16:58, <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 10:09 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -832,7 +832,8 @@ out:
>>> need_modify_vtd_table )
>>> {
>>> if ( iommu_hap_pt_share )
>>> - iommu_pte_flush(d, gfn, &ept_entry->epte, order,
> vtd_pte_present);
>>> + ret = iommu_pte_flush(d, gfn, &ept_entry->epte,
>>> + order, vtd_pte_present);
>>> else
>>> {
>>> if ( iommu_flags )
>>
>> Looking at this in conjunction with patch 3, I can't see where "ret"
>> would get consumed.
>
> Hmm, and here I see where "rc == 0" might be a better option than
> "entry_written".
>
> If we know rc is zero, we can just use rc here instead of 'ret', and I
> think everything falls out.
>
> If rc is not zero, then we have to do this "if ( !rc ) rc = ret;"
> business, which seems a bit silly to do when we know it's zero and
> don't expect that to change.
>
> On the other hand, using rc *without* actually checking that it's zero
> seems like asking for trouble.
>
> So perhaps it would be better if we take your advice for patch 3, and
> then use 'rc' here?
Ah, yes, that's a good 2nd reason.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 14:59 ` George Dunlap
@ 2016-05-11 2:26 ` Xu, Quan
2016-05-11 8:45 ` George Dunlap
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 2:26 UTC (permalink / raw)
To: George Dunlap, Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, Dario Faggioli,
xen-devel, Nakajima, Jun
On May 10, 2016 11:00 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 3:45 PM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
> > On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >>> When IOMMU mapping is failed, we issue a best effort rollback,
> >>> stopping IOMMU mapping, unmapping the previous IOMMU maps and
> then
> >>> reporting the error up to 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 an 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>
> >>> ---
> >>
> >> Somewhere here I continue to miss a summary on what has changed
> >> compared to the previous version. For review especially of larger
> >> patches (where preferably one wouldn't want to re-review the entire
> >> thing) this is more than just a nice-to-have.
> >>
> >>> @@ -812,17 +813,22 @@ ept_set_entry(struct p2m_domain *p2m,
> unsigned long gfn, mfn_t mfn,
> >>> rc = atomic_write_ept_entry(ept_entry, new_entry, target);
> >>> if ( unlikely(rc) )
> >>> old_entry.epte = 0;
> >>> - else if ( p2mt != p2m_invalid &&
> >>> - (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> >>> - /* Track the highest gfn for which we have ever had a valid mapping
> */
> >>> - p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> >>> + else
> >>> + {
> >>> + entry_written = 1;
> >>> +
> >>> + if ( p2mt != p2m_invalid &&
> >>> + (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
> >>> + /* Track the highest gfn for which we have ever had a valid
> mapping */
> >>> + p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> >>> + }
> >>>
> >>> out:
> >>> if ( needs_sync )
> >>> ept_sync_domain(p2m);
> >>>
> >>> /* For host p2m, may need to change VT-d page table.*/
> >>> - if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> >>> + if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> >>> need_modify_vtd_table )
> >>> {
> >>
> >> I'd prefer this conditional to remain untouched, but I'll leave the
> >> decision to the maintainers of the file.
> >
> > Any particular reason you think it would be better untouched?
> >
> > I asked for it to be changed to "entry_written", because it seemed to
> > me that's what was actually wanted (i.e., you're checking whether rc
> > == 0 to determine whether the entry was written or not). At the
> > moment the checks will be identical, but if someone changed something
> > later, rc might be non-zero even though the entry had been written, in
> > which case (I think) you'd want the iommu update to happen.
> >
> > It's not that big a deal to me, but I do prefer it this way (unless
> > I've misunderstood something).
>
> See the discussion on patch 8 regarding why I now think Jan is probably right.
>
Agreed. Thanks for your careful checking.
Check it again --
1. then I am no need to check 'rc' as below:
if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
need_modify_vtd_table )
{
+ if ( !rc )
+ rc = ret;
...
+ if ( !rc && unlikely(ret) )
+ rc = ret;
}
2. leave the below as is:
- if ( rc == 0 && p2m_is_hostp2m(p2m) )
+ if ( entry_written && p2m_is_hostp2m(p2m) )
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 14:45 ` George Dunlap
2016-05-10 14:59 ` George Dunlap
2016-05-10 15:02 ` Jan Beulich
@ 2016-05-11 2:29 ` Xu, Quan
2 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 2:29 UTC (permalink / raw)
To: George Dunlap, Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, Dario Faggioli,
xen-devel, Nakajima, Jun
On May 10, 2016 10:45 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Tue, May 10, 2016 at 9:44 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> When IOMMU mapping is failed, we issue a best effort rollback,
> >> stopping IOMMU mapping, unmapping the previous IOMMU maps and
> then
> >> reporting the error up to 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 an error message.
> >>
> >> IOMMU unmapping should perhaps continue despite an error, in an
> >> attempt to do best effort cleanup.
> >>
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -638,13 +638,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 rc = 0, ret;
> >>
> >> 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;
> >> + {
> >> + ret = iommu_unmap_page(p2m->domain, mfn + i);
> >> +
> >> + if ( !rc )
> >> + rc = ret;
> >> + }
> >> +
> >> + return rc;
> >> }
> >
> > In code like this, btw., restricting the scope of "ret" to the
> > innermost block would help future readers see immediately that the
> > value of "ret" is of no further interest outside of that block.
>
> I wouldn't ask for re-send just for this, but...
>
> > Having reached the end of the patch, I'm missing the __must_check
> > additions that you said you would do in this new iteration. Is there
> > any reason for their absence? Did I overlook something?
>
> If it's going to be re-sent anyway, moving the ret declaration inside the loop
> might as well be done.
>
> Other than that, it looks good to me, thanks.
>
I'll fix and re-send it in next v5.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-10 8:44 ` Jan Beulich
2016-05-10 14:45 ` George Dunlap
@ 2016-05-11 3:39 ` Xu, Quan
2016-05-11 7:02 ` Jan Beulich
1 sibling, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 3:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Nakajima, Jun
On May 10, 2016 4:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -638,13 +638,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 rc = 0, ret;
> >
> > 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;
> > + {
> > + ret = iommu_unmap_page(p2m->domain, mfn + i);
> > +
> > + if ( !rc )
> > + rc = ret;
> > + }
> > +
> > + return rc;
> > }
>
> In code like this, btw., restricting the scope of "ret" to the innermost block
> would help future readers see immediately that the value of "ret" is of no
> further interest outside of that block.
>
> Having reached the end of the patch, I'm missing the __must_check additions
> that you said you would do in this new iteration. Is there any reason for their
> absence? Did I overlook something?
>
Sorry, I did overlook something.
Checked the v2/v3 replies again, I still can't find it.
I only add the __must_check annotation for these functions you point out.
Do I need to add the __must_check annotation for these functions (but not void function) in this patch?
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping.
2016-05-10 8:50 ` Jan Beulich
@ 2016-05-11 3:49 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 3:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On May 10, 2016 4:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > Propagate the IOMMU Device-TLB flush error up to IOMMU unmapping.
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > Acked-by: Kevin Tian <kevin.tian@intel.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> but note ...
>
> > @@ -1766,9 +1769,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);
> > }
>
> ... how you lose the __must_check here, since
> intel_iommu_unmap_page() isn't __must_check (which we said you may skip
> as long as the common code wrapper has it, but in the context here I'm no
> longer convinced skipping this at any layer is a good idea, as that makes
> validation of the call trees more difficult).
> (This is just a remark regarding the comment on the earlier patch, i.e. not
> something needing any further change here.)
>
I'll be bold to add __must_check..
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).
2016-05-10 9:04 ` Jan Beulich
@ 2016-05-11 5:52 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 5:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Julien Grall, Stefano Stabellini, dario.faggioli, xen-devel
On May 10, 2016 5:04 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -677,9 +677,19 @@ static int xenmem_add_to_physmap(struct
> domain
> > *d, #ifdef CONFIG_HAS_PASSTHROUGH
> > if ( need_iommu(d) )
> > {
> > + int ret;
> > +
> > 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 && unlikely(ret) )
> > + rc = ret;
> > +
> > + ret = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> > +
> > + if ( rc >= 0 && unlikely(ret) )
> > + rc = ret;
>
> In both cases I think it would be preferable to switch the two sides of the &&.
>
Make sense. Thanks for your careful check.
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -312,24 +312,29 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> > + unsigned int page_count)
>
> The annotation generally belongs on declarations; the only exception are static
> functions which don't have any (i.e. they get defined before first getting
> used).
>
You are right. I will drop these __must_check annotation here.
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -200,8 +200,9 @@ 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 __must_check iommu_iotlb_flush(struct domain *d, unsigned long gfn,
> > + unsigned int page_count); int
> > +__must_check iommu_iotlb_flush_all(struct domain *d);
>
> I.e. these changes suffice, repeating the annotations on the function
> definitions is redundant with the ones put here.
>
Agreed.
> With respective adjustments done
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-10 9:06 ` Jan Beulich
@ 2016-05-11 6:47 ` Xu, Quan
2016-05-11 7:06 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 6:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Stefano Stabellini, Wu, Feng, dario.faggioli,
xen-devel, Julien Grall
On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d,
> unsigned long gfn,
> > return rc;
> > }
> >
> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> > - unsigned int page_count)
> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
> > +static int iommu_flush_iotlb_all(struct domain *d)
> > {
> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> > }
>
> As already indicated in a reply to an earlier patch, despite what was said on the
> earlier version I think we should have __must_check here
If the static one is initialized for .callback, is it really necessary to add __must_check here?
I check it with compiler, and it is ok when I didn't add __must_check here.
If yes, I'll add __must_check for ARM one as well.
> and ...
>
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -179,8 +179,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);
>
> ... here.
Yes, it is necessary here.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-11 3:39 ` Xu, Quan
@ 2016-05-11 7:02 ` Jan Beulich
0 siblings, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-11 7:02 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Jun Nakajima
>>> On 11.05.16 at 05:39, <quan.xu@intel.com> wrote:
> On May 10, 2016 4:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > --- a/xen/arch/x86/mm/p2m.c
>> > +++ b/xen/arch/x86/mm/p2m.c
>> > @@ -638,13 +638,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 rc = 0, ret;
>> >
>> > 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;
>> > + {
>> > + ret = iommu_unmap_page(p2m->domain, mfn + i);
>> > +
>> > + if ( !rc )
>> > + rc = ret;
>> > + }
>> > +
>> > + return rc;
>> > }
>>
>> In code like this, btw., restricting the scope of "ret" to the innermost
> block
>> would help future readers see immediately that the value of "ret" is of no
>> further interest outside of that block.
>>
>> Having reached the end of the patch, I'm missing the __must_check additions
>> that you said you would do in this new iteration. Is there any reason for
> their
>> absence? Did I overlook something?
>>
>
> Sorry, I did overlook something.
> Checked the v2/v3 replies again, I still can't find it.
> I only add the __must_check annotation for these functions you point out.
Okay, that's the problem then: When we discussed this originally
(in abstract terms) I had clearly said that all involved functions
should become __must_check ones to make sure all cases get
caught where so far error returns got ignored. And on that basis
(as well as on the common grounds that I try to avoid repeating
the same comment over and over when reviewing a patch or a
series of patches) you should have determined yourself the full
set of functions needing the annotation. The rule of thumb is: If
a function calls a __must_check one and doesn't itself consume
the return value, it should also obtain __must_check.
> Do I need to add the __must_check annotation for these functions (but not
> void function) in this patch?
IOW - yes. And you'll need to apply the same consideration to
most (all?) other patches in this series.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-11 6:47 ` Xu, Quan
@ 2016-05-11 7:06 ` Jan Beulich
2016-05-11 7:12 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-11 7:06 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, dario.faggioli,
xen-devel, Julien Grall
>>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote:
> On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain *d,
>> unsigned long gfn,
>> > return rc;
>> > }
>> >
>> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
>> > - unsigned int page_count)
>> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
>> > +static int iommu_flush_iotlb_all(struct domain *d)
>> > {
>> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> > }
>>
>> As already indicated in a reply to an earlier patch, despite what was said on the
>> earlier version I think we should have __must_check here
>
> If the static one is initialized for .callback, is it really necessary to
> add __must_check here?
> I check it with compiler, and it is ok when I didn't add __must_check here.
Without you telling us what exactly you checked, I can't respond
to this. Extending from the reply just sent to patch 3(?) and for
the avoidance of doubt, you now obviously also need to
__must_check-annotate the function pointer (to match the desire
of wanting to never lose such an annotation on the way back up
the call tree).
> If yes, I'll add __must_check for ARM one as well.
Of course.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-11 7:06 ` Jan Beulich
@ 2016-05-11 7:12 ` Xu, Quan
2016-05-11 7:16 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 7:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Stefano Stabellini, Wu, Feng, dario.faggioli,
xen-devel, Julien Grall
On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote:
> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain
> >> > *d,
> >> unsigned long gfn,
> >> > return rc;
> >> > }
> >> >
> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long
> gfn,
> >> > - unsigned int page_count)
> >> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
> >> > +static int iommu_flush_iotlb_all(struct domain *d)
> >> > {
> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> >> > }
> >>
> >> As already indicated in a reply to an earlier patch, despite what was
> >> said on the earlier version I think we should have __must_check here
> >
> > If the static one is initialized for .callback, is it really necessary
> > to add __must_check here?
> > I check it with compiler, and it is ok when I didn't add __must_check here.
>
> Without you telling us what exactly you checked, I can't respond to this.
> Extending from the reply just sent to patch 3(?) and for the avoidance of
> doubt, you now obviously also need to __must_check-annotate the function
> pointer (to match the desire of wanting to never lose such an annotation on
> the way back up the call tree).
>
I checked -- without __must_check for iommu_flush_iotlb_page() / iommu_flush_iotlb_all().
> > If yes, I'll add __must_check for ARM one as well.
>
> Of course.
>
Got it.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-11 7:12 ` Xu, Quan
@ 2016-05-11 7:16 ` Jan Beulich
2016-05-11 7:20 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-11 7:16 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, dario.faggioli,
xen-devel, Julien Grall
>>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote:
> On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote:
>> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain
>> >> > *d,
>> >> unsigned long gfn,
>> >> > return rc;
>> >> > }
>> >> >
>> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned long
>> gfn,
>> >> > - unsigned int page_count)
>> >> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
>> >> > +static int iommu_flush_iotlb_all(struct domain *d)
>> >> > {
>> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> >> > }
>> >>
>> >> As already indicated in a reply to an earlier patch, despite what was
>> >> said on the earlier version I think we should have __must_check here
>> >
>> > If the static one is initialized for .callback, is it really necessary
>> > to add __must_check here?
>> > I check it with compiler, and it is ok when I didn't add __must_check here.
>>
>> Without you telling us what exactly you checked, I can't respond to this.
>> Extending from the reply just sent to patch 3(?) and for the avoidance of
>> doubt, you now obviously also need to __must_check-annotate the function
>> pointer (to match the desire of wanting to never lose such an annotation on
>> the way back up the call tree).
>>
>
> I checked -- without __must_check for iommu_flush_iotlb_page() /
> iommu_flush_iotlb_all().
But _what_ did you check? I.e. the question isn't which functions
you did your check with, but what behavioral checking you did.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-11 7:16 ` Jan Beulich
@ 2016-05-11 7:20 ` Xu, Quan
2016-05-11 7:37 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 7:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Stefano Stabellini, Wu, Feng, dario.faggioli,
xen-devel, Julien Grall
On May 11, 2016 3:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote:
> > On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote:
> >> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain
> >> >> > *d,
> >> >> unsigned long gfn,
> >> >> > return rc;
> >> >> > }
> >> >> >
> >> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned
> >> >> > long
> >> gfn,
> >> >> > - unsigned int page_count)
> >> >> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
> >> >> > +static int iommu_flush_iotlb_all(struct domain *d)
> >> >> > {
> >> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> >> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
> >> >> > }
> >> >>
> >> >> As already indicated in a reply to an earlier patch, despite what
> >> >> was said on the earlier version I think we should have
> >> >> __must_check here
> >> >
> >> > If the static one is initialized for .callback, is it really
> >> > necessary to add __must_check here?
> >> > I check it with compiler, and it is ok when I didn't add __must_check here.
> >>
> >> Without you telling us what exactly you checked, I can't respond to this.
> >> Extending from the reply just sent to patch 3(?) and for the
> >> avoidance of doubt, you now obviously also need to
> >> __must_check-annotate the function pointer (to match the desire of
> >> wanting to never lose such an annotation on the way back up the call tree).
> >>
> >
> > I checked -- without __must_check for iommu_flush_iotlb_page() /
> > iommu_flush_iotlb_all().
>
> But _what_ did you check? I.e. the question isn't which functions you did your
> check with, but what behavioral checking you did.
>
without __must_check for iommu_flush_iotlb_page() /iommu_flush_iotlb_all(), I can run 'make xen' successfully. Sorry.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update.
2016-05-10 9:09 ` Jan Beulich
2016-05-10 14:58 ` George Dunlap
@ 2016-05-11 7:25 ` Xu, Quan
1 sibling, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 7:25 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Wu, Feng, George Dunlap, Andrew Cooper,
dario.faggioli, xen-devel, Nakajima, Jun
On Tuesday, May 10, 2016 5:10 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -832,7 +832,8 @@ out:
> > need_modify_vtd_table )
> > {
> > if ( iommu_hap_pt_share )
> > - iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
> > + ret = iommu_pte_flush(d, gfn, &ept_entry->epte,
> > + order, vtd_pte_present);
> > else
> > {
> > if ( iommu_flags )
>
> Looking at this in conjunction with patch 3, I can't see where "ret"
> would get consumed.
>
Sorry, this 'ret' should be 'rc' here, as I removed the pending
"
+ if ( !rc )
+ rc = ret;
+
"
from v4.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones).
2016-05-11 7:20 ` Xu, Quan
@ 2016-05-11 7:37 ` Jan Beulich
0 siblings, 0 replies; 64+ messages in thread
From: Jan Beulich @ 2016-05-11 7:37 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, dario.faggioli,
xen-devel, Julien Grall
>>> On 11.05.16 at 09:20, <quan.xu@intel.com> wrote:
> On May 11, 2016 3:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 11.05.16 at 09:12, <quan.xu@intel.com> wrote:
>> > On May 11, 2016 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 11.05.16 at 08:47, <quan.xu@intel.com> wrote:
>> >> > On May 10, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> >> > @@ -604,15 +604,15 @@ static int iommu_flush_iotlb(struct domain
>> >> >> > *d,
>> >> >> unsigned long gfn,
>> >> >> > return rc;
>> >> >> > }
>> >> >> >
>> >> >> > -static void iommu_flush_iotlb_page(struct domain *d, unsigned
>> >> >> > long
>> >> gfn,
>> >> >> > - unsigned int page_count)
>> >> >> > +static int iommu_flush_iotlb_page(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 iommu_flush_iotlb_all(struct domain *d)
>> >> >> > +static int iommu_flush_iotlb_all(struct domain *d)
>> >> >> > {
>> >> >> > - iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> >> >> > + return iommu_flush_iotlb(d, INVALID_GFN, 0, 0);
>> >> >> > }
>> >> >>
>> >> >> As already indicated in a reply to an earlier patch, despite what
>> >> >> was said on the earlier version I think we should have
>> >> >> __must_check here
>> >> >
>> >> > If the static one is initialized for .callback, is it really
>> >> > necessary to add __must_check here?
>> >> > I check it with compiler, and it is ok when I didn't add __must_check
> here.
>> >>
>> >> Without you telling us what exactly you checked, I can't respond to this.
>> >> Extending from the reply just sent to patch 3(?) and for the
>> >> avoidance of doubt, you now obviously also need to
>> >> __must_check-annotate the function pointer (to match the desire of
>> >> wanting to never lose such an annotation on the way back up the call tree).
>> >>
>> >
>> > I checked -- without __must_check for iommu_flush_iotlb_page() /
>> > iommu_flush_iotlb_all().
>>
>> But _what_ did you check? I.e. the question isn't which functions you did
> your
>> check with, but what behavioral checking you did.
>>
> without __must_check for iommu_flush_iotlb_page() /iommu_flush_iotlb_all(),
> I can run 'make xen' successfully. Sorry.
Oh, sure you can. Leaving the annotation out will always result in
no more (and likely less) diagnostics. I.e. that's not a valid criteria.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-10 9:29 ` Jan Beulich
@ 2016-05-11 8:35 ` Xu, Quan
2016-05-11 9:07 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 8:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
> > unmap_vtd_domain_page(context_entries);
> >
> > if ( !seg )
> > - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> > + {
> > + ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> > +
> > + if ( !rc )
> > + rc = ret;
> > + }
>
> Is there any use in calling this function if an earlier error occurred?
> If not,
It is no use.
We may need to consider this call tree:
$ me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reassign_device_ownership()--...
Then, what about dropping this patch? Leave it as is,
or remove ' __must_check' annotation and propagate error up to the above call tree only?
> the change can be more lightweight (while in the unmap case it should
> probably stay as is, to fit the "best effort" theme).
>
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-11 2:26 ` Xu, Quan
@ 2016-05-11 8:45 ` George Dunlap
2016-05-11 8:58 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: George Dunlap @ 2016-05-11 8:45 UTC (permalink / raw)
To: Xu, Quan
Cc: Tian, Kevin, Keir Fraser, Nakajima, Jun, Andrew Cooper,
Dario Faggioli, xen-devel, Jan Beulich
[-- Attachment #1: Type: text/plain, Size: 817 bytes --]
On Wed, May 11, 2016 at 3:26 AM, Xu, Quan <quan.xu@intel.com> wrote:
> Agreed. Thanks for your careful checking.
>
> Check it again --
> 1. then I am no need to check 'rc' as below:
>
> if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> need_modify_vtd_table )
> {
> + if ( !rc )
> + rc = ret;
> ...
>
> + if ( !rc && unlikely(ret) )
> + rc = ret;
> }
Actually, in the case of iommu_map_page(), you can just use rc
directly and not bother using ret at all. (In the case of
iommu_unmap_page(), you still need to use ret and check that rc != 0
to make sure you get the first error.)
Something like applying the attached patch (not built or nitpicked for
style, just for clarity).
-George
[-- Attachment #2: patch3-mod.diff --]
[-- Type: text/plain, Size: 1152 bytes --]
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 814cb72..f8360c6 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -828,7 +828,7 @@ out:
ept_sync_domain(p2m);
/* For host p2m, may need to change VT-d page table.*/
- if ( entry_written && p2m_is_hostp2m(p2m) && need_iommu(d) &&
+ if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
need_modify_vtd_table )
{
if ( iommu_hap_pt_share )
@@ -838,16 +838,13 @@ out:
if ( iommu_flags )
for ( i = 0; i < (1 << order); i++ )
{
- ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
+ rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
- if ( unlikely(ret) )
+ if ( unlikely(rc) )
{
while ( i-- )
iommu_unmap_page(p2m->domain, gfn + i);
- if ( !rc )
- rc = ret;
-
break;
}
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread
* Re: [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
2016-05-11 8:45 ` George Dunlap
@ 2016-05-11 8:58 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-11 8:58 UTC (permalink / raw)
To: George Dunlap
Cc: Tian, Kevin, Keir Fraser, Nakajima, Jun, Andrew Cooper,
Dario Faggioli, xen-devel, Jan Beulich
On May 11, 2016 4:46 PM, George Dunlap <George.Dunlap@eu.citrix.com> wrote:
> On Wed, May 11, 2016 at 3:26 AM, Xu, Quan <quan.xu@intel.com> wrote:
> > Agreed. Thanks for your careful checking.
> >
> > Check it again --
> > 1. then I am no need to check 'rc' as below:
> >
> > if ( rc == 0 && p2m_is_hostp2m(p2m) && need_iommu(d) &&
> > need_modify_vtd_table )
> > {
> > + if ( !rc )
> > + rc = ret;
> > ...
> >
> > + if ( !rc && unlikely(ret) )
> > + rc = ret;
> > }
>
> Actually, in the case of iommu_map_page(), you can just use rc directly and
> not bother using ret at all. (In the case of iommu_unmap_page(), you still
> need to use ret and check that rc != 0 to make sure you get the first error.)
>
> Something like applying the attached patch (not built or nitpicked for style,
> just for clarity).
>
Got it, thanks.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-11 8:35 ` Xu, Quan
@ 2016-05-11 9:07 ` Jan Beulich
2016-05-12 5:16 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-11 9:07 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 11.05.16 at 10:35, <quan.xu@intel.com> wrote:
> On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
>> > unmap_vtd_domain_page(context_entries);
>> >
>> > if ( !seg )
>> > - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
>> > + {
>> > + ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
>> > +
>> > + if ( !rc )
>> > + rc = ret;
>> > + }
>>
>> Is there any use in calling this function if an earlier error occurred?
>> If not,
>
> It is no use.
With this I don't understand ...
> We may need to consider this call tree:
> $
> me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reass
> ign_device_ownership()--...
>
> Then, what about dropping this patch? Leave it as is,
> or remove ' __must_check' annotation and propagate error up to the above
> call tree only?
... this. If calling the function is pointless if an earlier error occurred,
why don't you just check rc to be zero alongside the !seg check?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-11 9:07 ` Jan Beulich
@ 2016-05-12 5:16 ` Xu, Quan
2016-05-12 8:44 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 5:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On May 11, 2016 5:07 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 11.05.16 at 10:35, <quan.xu@intel.com> wrote:
> > On May 10, 2016 5:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one(
> >> > unmap_vtd_domain_page(context_entries);
> >> >
> >> > if ( !seg )
> >> > - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
> >> > + {
> >> > + ret = me_wifi_quirk(domain, bus, devfn,
> >> > + MAP_ME_PHANTOM_FUNC);
> >> > +
> >> > + if ( !rc )
> >> > + rc = ret;
> >> > + }
> >>
> >> Is there any use in calling this function if an earlier error occurred?
> >> If not,
> >
> > It is no use.
>
> With this I don't understand ...
>
> > We may need to consider this call tree:
> > $
> > me_wifi_quirk()--domain_context_mapping_one()--
> domain_context_mapping(
> > )--reass
> > ign_device_ownership()--...
> >
> > Then, what about dropping this patch? Leave it as is, or remove '
> > __must_check' annotation and propagate error up to the above call tree
> > only?
>
> ... this. If calling the function is pointless if an earlier error occurred, why don't
> you just check rc to be zero alongside the !seg check?
>
---
Good idea.
---
Taken together, there are 3 call trees to me_wifi_quirk():
1). ...--me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--setup_hwdom_device()
There is no use in calling this function if an earlier error occurred. The change can be more lightweight (the detailed change is pending).
2). me_wifi_quirk()--domain_context_unmap_one()--...
As you mentioned, while in the unmap case it should probably stay as is, to fit the "best effort" theme.
Then I need to remove the __must_check annotation of me_wifi_quirk().
3). me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reassign_device_ownership()
This is not an earlier error, so we need propagate the error up to the call tree (the detailed change is pending).
The below is based on previous v4 p1...p9:
---
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/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 29cf870..0ac7894 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1429,8 +1429,8 @@ int domain_context_mapping_one(
unmap_vtd_domain_page(context_entries);
- if ( !seg )
- me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+ if ( !seg && !rc )
+ rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
return rc;
}
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index 473d1fc..3606b52 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -331,10 +331,12 @@ 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 __must_check 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));
@@ -342,23 +344,27 @@ 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);
@@ -372,7 +378,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;
@@ -382,7 +388,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);
@@ -398,12 +404,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
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-09 16:09 ` Jan Beulich
@ 2016-05-12 7:50 ` Xu, Quan
2016-05-12 8:53 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 7:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, Andrew Cooper, dario.faggioli,
xen-devel, Wu, Feng
On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
> > gfn, unsigned int page_count)
> > +static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
> > + unsigned int page_count)
>
> The new name suggests just one page. Please use e.g.
> iommu_flush_iotlb_pages() instead.
>
Make sense.
> > {
> > - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> > + iommu_flush_iotlb(d, gfn, 1, page_count);
> > }
>
> But of course the question is whether having this wrapper is useful in the first
> place,
This wrapper assumes the 'dma_old_pte_present' is '1', but in another caller intel_iommu_map_page(), i.e.
intel_iommu_map_page()
{
...
if ( !this_cpu(iommu_dont_flush_iotlb) )
iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
...
}
the 'dma_old_pte_present' is not sure.
in intel_iommu_map_page(), if we can check the 'dma_pte_present(old)':
-- 1, flush the pages.
-- 0, don't flush the pages.
Then we can remove this wrapper.
If my description is not clear, I can send out the related change.
> the more that ...
>
> > @@ -639,7 +646,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);
>
> ... it's being open coded here. IOW if you want to retain the wrapper, please
> use it here.
>
Waiting for the above discussion, if we still need the wrapper, I will use it here.
> > @@ -1391,13 +1399,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);
>
> Please take the opportunity and add the missing blank line (between
> declaration(s) and statement(s) in cases like this.
>
> > + }
> > +
> > + if ( rc > 0 )
>
> Can iommu_flush_context_device() return a positive value? If so, the logic is
> now likely wrong. If not (which is what I assume) I'd like to suggest adding a
> respective ASSERT() (even if only to document the fact). Or alternatively this
> if() could move into the immediately preceding one.
>
Check it again. iommu_flush_context_device() can return a positive value.
If VT-d QI is enabled, the call tree up to iommu_flush_context_device():
-- flush_context_qi() -- iommu_flush_context_device()
i.e.
In flush_context_qi()
{
...
if ( flush_non_present_entry )
{
if ( !cap_caching_mode(iommu->cap) )
return 1;
else
did = 0;
}
...
}
and the ' flush_non_present_entry ' is really '1' for above code.
Could you tell me why the logic is now likely wrong? I will fix it first.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-12 5:16 ` Xu, Quan
@ 2016-05-12 8:44 ` Jan Beulich
2016-05-12 9:02 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-12 8:44 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Feng Wu, Kevin Tian, xen-devel
>>> On 12.05.16 at 07:16, <quan.xu@intel.com> wrote:
> Taken together, there are 3 call trees to me_wifi_quirk():
>
> 1).
> ...--me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--se
> tup_hwdom_device()
>
> There is no use in calling this function if an earlier
> error occurred. The change can be more lightweight (the detailed change is
> pending).
>
> 2). me_wifi_quirk()--domain_context_unmap_one()--...
>
> As you mentioned, while in the unmap case it should
> probably stay as is, to fit the "best effort" theme.
>
> Then I need to remove the __must_check annotation of
> me_wifi_quirk().
This does not follow from the above. You again should propagate
the error in all cases (unless it would overwrite an earlier error - as
you're doing in various other places).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-12 7:50 ` Xu, Quan
@ 2016-05-12 8:53 ` Jan Beulich
2016-05-12 13:29 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-12 8:53 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, AndrewCooper, dario.faggioli, xen-devel,
Feng Wu
>>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote:
> On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> > gfn, unsigned int page_count)
>> > +static void iommu_flush_iotlb_page(struct domain *d, unsigned long gfn,
>> > + unsigned int page_count)
>>
>> The new name suggests just one page. Please use e.g.
>> iommu_flush_iotlb_pages() instead.
>>
>
> Make sense.
>
>> > {
>> > - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
>> > + iommu_flush_iotlb(d, gfn, 1, page_count);
>> > }
>>
>> But of course the question is whether having this wrapper is useful in the first
>> place,
>
>
> This wrapper assumes the 'dma_old_pte_present' is '1', but in another caller
> intel_iommu_map_page(), i.e.
>
>
> intel_iommu_map_page()
> {
> ...
> if ( !this_cpu(iommu_dont_flush_iotlb) )
> iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
> ...
> }
>
>
> the 'dma_old_pte_present' is not sure.
I'm sorry, but you're looking at this backwards: I suggested to
remove the wrapper, not to move any check into iommu_flush_iotlb().
Removing the wrapper simply means to move the passing of the
hard coded 1 into the current callers of that wrapper.
>> > @@ -1391,13 +1399,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);
>>
>> Please take the opportunity and add the missing blank line (between
>> declaration(s) and statement(s) in cases like this.
>>
>> > + }
>> > +
>> > + if ( rc > 0 )
>>
>> Can iommu_flush_context_device() return a positive value? If so, the logic is
>> now likely wrong. If not (which is what I assume) I'd like to suggest adding a
>> respective ASSERT() (even if only to document the fact). Or alternatively this
>> if() could move into the immediately preceding one.
>
> Check it again. iommu_flush_context_device() can return a positive value.
> [...]
> Could you tell me why the logic is now likely wrong? I will fix it first.
With
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;
rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
}
if ( rc > 0 )
{
iommu_flush_write_buffer(iommu);
rc = 0;
}
it seems pretty clear that you won't call iommu_flush_iotlb_dsi() if
iommu_flush_context_device() returned 1, which doesn't look like
what is wanted at the first glance. But I may be wrong, hence the
"likely" in my earlier reply.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping
2016-05-12 8:44 ` Jan Beulich
@ 2016-05-12 9:02 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 9:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Wu, Feng, Tian, Kevin, xen-devel
On May 12, 2016 4:45 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 12.05.16 at 07:16, <quan.xu@intel.com> wrote:
> > Taken together, there are 3 call trees to me_wifi_quirk():
> >
> > 1).
> > ...--me_wifi_quirk()--domain_context_mapping_one()--
> domain_context_map
> > ping()--se
> > tup_hwdom_device()
> >
> > There is no use in calling this function if an
> > earlier error occurred. The change can be more lightweight (the
> > detailed change is pending).
> >
> > 2). me_wifi_quirk()--domain_context_unmap_one()--...
> >
> > As you mentioned, while in the unmap case it
> > should probably stay as is, to fit the "best effort" theme.
> >
> > Then I need to remove the __must_check annotation
> > of me_wifi_quirk().
>
> This does not follow from the above. You again should propagate the error in
> all cases (unless it would overwrite an earlier error - as you're doing in various
> other places).
>
Sorry, I know the item 2). is tricky, as I am confused about ' while in the unmap case it should probably stay as is, to fit the "best effort" theme '.
Actually, what I need to enhance the p10 are:
- drop ret
- replace the
if ( !seg )
- me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+ {
+ ret = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+
+ if ( !rc )
+ rc = ret;
+ }
____TO_____
- if ( !seg )
- me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
+ if ( !seg && !rc )
+ rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC);
btw, I found I am struggling in this v4 and I will spend more time fixing. 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] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-12 8:53 ` Jan Beulich
@ 2016-05-12 13:29 ` Xu, Quan
2016-05-12 13:37 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 13:29 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, AndrewCooper, dario.faggioli,
xen-devel, Wu, Feng
On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote:
> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned
> >> > long gfn, unsigned int page_count)
> >> > +static void iommu_flush_iotlb_page(struct domain *d, unsigned long
> gfn,
> >> > + unsigned int page_count)
> >>
> >> The new name suggests just one page. Please use e.g.
> >> iommu_flush_iotlb_pages() instead.
> >>
> >
> > Make sense.
> >
> >> > {
> >> > - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
> >> > + iommu_flush_iotlb(d, gfn, 1, page_count);
> >> > }
> >>
> >> But of course the question is whether having this wrapper is useful
> >> in the first place,
> >
> >
> > This wrapper assumes the 'dma_old_pte_present' is '1', but in another
> > caller intel_iommu_map_page(), i.e.
> >
> >
> > intel_iommu_map_page()
> > {
> > ...
> > if ( !this_cpu(iommu_dont_flush_iotlb) )
> > iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
> > ...
> > }
> >
> >
> > the 'dma_old_pte_present' is not sure.
>
> I'm sorry, but you're looking at this backwards: I suggested to remove the
> wrapper, not to move any check into iommu_flush_iotlb().
> Removing the wrapper simply means to move the passing of the hard coded 1
> into the current callers of that wrapper.
>
A little bit confused.
Check one thing, do the wrappers refer to iommu_flush_iotlb_page() and iommu_flush_iotlb_all() ?
If yes, we can't ignore another thing:
These two wrappers are also initialized for 2 .callbacks at the bottom of this file:
....
.iotlb_flush = iommu_flush_iotlb_pages,
.iotlb_flush_all = iommu_flush_iotlb_all,
....
> >> > @@ -1391,13 +1399,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);
> >>
> >> Please take the opportunity and add the missing blank line (between
> >> declaration(s) and statement(s) in cases like this.
> >>
> >> > + }
> >> > +
> >> > + if ( rc > 0 )
> >>
> >> Can iommu_flush_context_device() return a positive value? If so, the
> >> logic is now likely wrong. If not (which is what I assume) I'd like
> >> to suggest adding a respective ASSERT() (even if only to document the
> >> fact). Or alternatively this
> >> if() could move into the immediately preceding one.
> >
> > Check it again. iommu_flush_context_device() can return a positive value.
> > [...]
> > Could you tell me why the logic is now likely wrong? I will fix it first.
>
> With
>
> 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;
> rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> }
>
> if ( rc > 0 )
> {
> iommu_flush_write_buffer(iommu);
> rc = 0;
> }
>
> it seems pretty clear that you won't call iommu_flush_iotlb_dsi() if
> iommu_flush_context_device() returned 1, which doesn't look like what is
> wanted at the first glance. But I may be wrong, hence the "likely" in my earlier
> reply.
>
Oh, this was on purpose.
If iommu_flush_context_device() returned 1, the iommu_flush_iotlb_dsi() returned 1 too.
As both flush_context_qi() and flush_iotlb_qi () are the same at the beginning of the functions.
One concern is if iommu_flush_context_device() is failed, then we won't call iommu_flush_iotlb_dsi(), which is not best effort to flush.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-12 13:29 ` Xu, Quan
@ 2016-05-12 13:37 ` Jan Beulich
2016-05-12 13:43 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-12 13:37 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Keir Fraser, AndrewCooper, dario.faggioli, xen-devel,
Feng Wu
>>> On 12.05.16 at 15:29, <quan.xu@intel.com> wrote:
> On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote:
>> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> >> > -static void intel_iommu_iotlb_flush(struct domain *d, unsigned
>> >> > long gfn, unsigned int page_count)
>> >> > +static void iommu_flush_iotlb_page(struct domain *d, unsigned long
>> gfn,
>> >> > + unsigned int page_count)
>> >>
>> >> The new name suggests just one page. Please use e.g.
>> >> iommu_flush_iotlb_pages() instead.
>> >>
>> >
>> > Make sense.
>> >
>> >> > {
>> >> > - __intel_iommu_iotlb_flush(d, gfn, 1, page_count);
>> >> > + iommu_flush_iotlb(d, gfn, 1, page_count);
>> >> > }
>> >>
>> >> But of course the question is whether having this wrapper is useful
>> >> in the first place,
>> >
>> >
>> > This wrapper assumes the 'dma_old_pte_present' is '1', but in another
>> > caller intel_iommu_map_page(), i.e.
>> >
>> >
>> > intel_iommu_map_page()
>> > {
>> > ...
>> > if ( !this_cpu(iommu_dont_flush_iotlb) )
>> > iommu_flush_iotlb(d, gfn, dma_pte_present(old), 1);
>> > ...
>> > }
>> >
>> >
>> > the 'dma_old_pte_present' is not sure.
>>
>> I'm sorry, but you're looking at this backwards: I suggested to remove the
>> wrapper, not to move any check into iommu_flush_iotlb().
>> Removing the wrapper simply means to move the passing of the hard coded 1
>> into the current callers of that wrapper.
>>
>
> A little bit confused.
> Check one thing, do the wrappers refer to iommu_flush_iotlb_page() and
> iommu_flush_iotlb_all() ?
>
> If yes, we can't ignore another thing:
>
> These two wrappers are also initialized for 2 .callbacks at the bottom of
> this file:
>
> ....
> .iotlb_flush = iommu_flush_iotlb_pages,
> .iotlb_flush_all = iommu_flush_iotlb_all,
> ....
Ah, good point. With the renaming going on I didn't realize these
are used here. So in fact they're not just wrappers. Please disregard
my respective comments then.
>> >> > @@ -1391,13 +1399,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);
>> >>
>> >> Please take the opportunity and add the missing blank line (between
>> >> declaration(s) and statement(s) in cases like this.
>> >>
>> >> > + }
>> >> > +
>> >> > + if ( rc > 0 )
>> >>
>> >> Can iommu_flush_context_device() return a positive value? If so, the
>> >> logic is now likely wrong. If not (which is what I assume) I'd like
>> >> to suggest adding a respective ASSERT() (even if only to document the
>> >> fact). Or alternatively this
>> >> if() could move into the immediately preceding one.
>> >
>> > Check it again. iommu_flush_context_device() can return a positive value.
>> > [...]
>> > Could you tell me why the logic is now likely wrong? I will fix it first.
>>
>> With
>>
>> 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;
>> rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
>> }
>>
>> if ( rc > 0 )
>> {
>> iommu_flush_write_buffer(iommu);
>> rc = 0;
>> }
>>
>> it seems pretty clear that you won't call iommu_flush_iotlb_dsi() if
>> iommu_flush_context_device() returned 1, which doesn't look like what is
>> wanted at the first glance. But I may be wrong, hence the "likely" in my
> earlier
>> reply.
>>
>
> Oh, this was on purpose.
>
> If iommu_flush_context_device() returned 1, the iommu_flush_iotlb_dsi()
> returned 1 too.
> As both flush_context_qi() and flush_iotlb_qi () are the same at the
> beginning of the functions.
Such implications need to be commented on, so readers (like me)
don't assume brokenness.
> One concern is if iommu_flush_context_device() is failed, then we won't call
> iommu_flush_iotlb_dsi(), which is not best effort to flush.
Indeed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 01/10] vt-d: fix the IOMMU flush issue
2016-05-12 13:37 ` Jan Beulich
@ 2016-05-12 13:43 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 13:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Keir Fraser, AndrewCooper, dario.faggioli,
xen-devel, Wu, Feng
On May 12, 2016 9:38 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 12.05.16 at 15:29, <quan.xu@intel.com> wrote:
> > On May 12, 2016 4:53 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 12.05.16 at 09:50, <quan.xu@intel.com> wrote:
> >> > On May 10, 2016 12:10 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> >> > @@ -1391,13 +1399,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);
> >> >>
> >> >> Please take the opportunity and add the missing blank line
> >> >> (between
> >> >> declaration(s) and statement(s) in cases like this.
> >> >>
> >> >> > + }
> >> >> > +
> >> >> > + if ( rc > 0 )
> >> >>
> >> >> Can iommu_flush_context_device() return a positive value? If so,
> >> >> the logic is now likely wrong. If not (which is what I assume) I'd
> >> >> like to suggest adding a respective ASSERT() (even if only to
> >> >> document the fact). Or alternatively this
> >> >> if() could move into the immediately preceding one.
> >> >
> >> > Check it again. iommu_flush_context_device() can return a positive value.
> >> > [...]
> >> > Could you tell me why the logic is now likely wrong? I will fix it first.
> >>
> >> With
> >>
> >> 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;
> >> rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> >> }
> >>
> >> if ( rc > 0 )
> >> {
> >> iommu_flush_write_buffer(iommu);
> >> rc = 0;
> >> }
> >>
> >> it seems pretty clear that you won't call iommu_flush_iotlb_dsi() if
> >> iommu_flush_context_device() returned 1, which doesn't look like what
> >> is wanted at the first glance. But I may be wrong, hence the "likely"
> >> in my
> > earlier
> >> reply.
> >>
> >
> > Oh, this was on purpose.
> >
> > If iommu_flush_context_device() returned 1, the
> > iommu_flush_iotlb_dsi() returned 1 too.
> > As both flush_context_qi() and flush_iotlb_qi () are the same at the
> > beginning of the functions.
>
> Such implications need to be commented on, so readers (like me) don't
> assume brokenness.
>
ok, I will add a comment.
> > One concern is if iommu_flush_context_device() is failed, then we
> > won't call iommu_flush_iotlb_dsi(), which is not best effort to flush.
>
> Indeed.
>
I'll fix it as well.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-10 6:53 ` Jan Beulich
2016-05-10 7:53 ` Xu, Quan
@ 2016-05-12 14:28 ` Xu, Quan
2016-05-12 15:06 ` Jan Beulich
1 sibling, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-12 14:28 UTC (permalink / raw)
To: Jan Beulich, Tian, Kevin; +Cc: dario.faggioli, xen-devel
On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> For DomU the solution seems quite obvious: Only log a message if the domain
> is not already marked crashed.
Jan, I am still confused about this sentence and your another sentence ( _as said_ also avoid logging any message for already dying domains).
> For Dom0 you'll need to get a little more
> creative (but by leveraging the fact that there's only one in the system, this
> can't be too difficult a problem to solve:
> e.g. "manually" rate limit these messages - see printk_ratelimit() et al).
>
Reading this thread again and again, sorry, I am still inclined to:
+ rc = hd->platform_ops->unmap_page(d, gfn);
+
+ if ( unlikely(rc) )
+ {
+ if ( printk_ratelimit() )
+ printk(XENLOG_ERR
+ "dom%d: IOMMU unmapping gfn %#lx failed %d.",
+ d->domain_id, gfn, rc);
+
+ if ( !is_hardware_domain(d) )
+ domain_crash(d);
+ }
+
+ return rc;
Waiting for Kevin's opinion..
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-12 14:28 ` Xu, Quan
@ 2016-05-12 15:06 ` Jan Beulich
2016-05-13 8:04 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-12 15:06 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
> On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
>> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> For DomU the solution seems quite obvious: Only log a message if the domain
>> is not already marked crashed.
>
> Jan, I am still confused about this sentence and your another sentence (
> _as said_ also avoid logging any message for already dying domains).
The two say the same, so I don't see what you're confused about.
Please be more precise.
>> For Dom0 you'll need to get a little more
>> creative (but by leveraging the fact that there's only one in the system,
> this
>> can't be too difficult a problem to solve:
>> e.g. "manually" rate limit these messages - see printk_ratelimit() et al).
>>
>
> Reading this thread again and again, sorry, I am still inclined to:
>
> + rc = hd->platform_ops->unmap_page(d, gfn);
> +
> + if ( unlikely(rc) )
> + {
> + if ( printk_ratelimit() )
> + printk(XENLOG_ERR
> + "dom%d: IOMMU unmapping gfn %#lx failed %d.",
> + d->domain_id, gfn, rc);
> +
> + if ( !is_hardware_domain(d) )
> + domain_crash(d);
> + }
> +
> + return rc;
This is certainly better than unconditional logging, but will still
produce more than one message per crashed guest (or for
Dom0) on a batch of unmaps.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-05-10 9:24 ` Jan Beulich
@ 2016-05-13 3:39 ` Xu, Quan
2016-05-13 6:16 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-13 3:39 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Stefano Stabellini, Wu, Feng, Liu Jinsong,
dario.faggioli, xen-devel, Julien Grall, Suravee Suthikulpanit,
Andrew Cooper, Keir Fraser
On May 10, 2016 5:25 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> > --- a/xen/arch/x86/acpi/power.c
> > +++ b/xen/arch/x86/acpi/power.c
> > static int device_power_down(void)
> > {
> > - console_suspend();
> > + if ( console_suspend() )
> > + return TYPE_CONSOLE;
>
> This (together with the resume side) makes me guess that the use of TYPE_ as
> a prefix confused not just me, but also you:
Yes, this is really not a good prefix, and probably pretty bad to use 'ERROR_'.
What about 'PRIOR_'? then I also need to adjust device_power_up() as ...
> Here you want to tell the caller
> that everything _prior_ to console suspend (which happens to be nothing in
> this specific case) needs to be undone. Yet below you use TYPE_CONSOLE as
> an indication that
> console_resume() needs to be called.
... this,
+ switch ( prior )
+ {
...
+ time_resume();
+ case PRIOR_TIME:
+ console_resume();
+ case PRIOR_CONSOLE:
...
+ }
>
> > - time_suspend();
> > + if ( time_suspend() )
> > + return TYPE_TIME;
> >
> > - i8259A_suspend();
> > + if ( i8259A_suspend() )
> > + return TYPE_I8259A;
> >
> > + /* ioapic_suspend should never fail */
> > ioapic_suspend();
>
> The comment is bogus: "should" means it can in theory. Yet the function
> having void return type means it just cannot fail.
>
I'll use 'cannot', instead of 'should'.
Another question, I check the code again, and the rest of the functions (console_suspend/ time_suspend/ i8259A_suspend / ioapic_suspend / lapic_suspend ), in device_power_down(), always returned '0'.
Maybe I need to fix these functions annotation from 'int' to 'void', and then I can add a comment on the device_power_down().
More that, if the ' iommu_suspend()' is the only fail, could we re-consider the previous v2/v3 solution with 'goto'?
> > @@ -169,6 +204,7 @@ static int enter_state(u32 state)
> > {
> > printk(XENLOG_ERR "Some devices failed to power down.");
> > system_state = SYS_STATE_resume;
> > + device_power_up(error);
>
> Either error's and device_power_down()'s return type get changed to enum
> dev_power_type, or this needs a "error > 0" conditional.
>
> > @@ -1267,7 +1273,9 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
> > setup_hwdom_pci_devices(d, setup_hwdom_device);
> > setup_hwdom_rmrr(d);
> >
> > - iommu_flush_all();
> > + if ( iommu_flush_all() )
> > + printk(XENLOG_WARNING VTDPREFIX
> > + " intel_iommu_hwdom_init: IOMMU flush all failed.\n");
>
> As said (and I think a number of times) before: At least when you already hold
> the error value in your hands, please also log it.
Agreed, and I will apply for other patches.
> Also personally I'm opposed to
> including function names in non-debug log messages, but I'll leave that
> decision to the VT-d maintainers here.
>
Albeit Reluctantly, I will fix it.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-05-13 3:39 ` Xu, Quan
@ 2016-05-13 6:16 ` Jan Beulich
2016-05-13 6:27 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-13 6:16 UTC (permalink / raw)
To: Quan Xu
Cc: Kevin Tian, Stefano Stabellini, Feng Wu, Liu Jinsong,
dario.faggioli, xen-devel, Julien Grall, SuraveeSuthikulpanit,
Andrew Cooper, Keir Fraser
>>> On 13.05.16 at 05:39, <quan.xu@intel.com> wrote:
> On May 10, 2016 5:25 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
>> > --- a/xen/arch/x86/acpi/power.c
>> > +++ b/xen/arch/x86/acpi/power.c
>> > static int device_power_down(void)
>> > {
>> > - console_suspend();
>> > + if ( console_suspend() )
>> > + return TYPE_CONSOLE;
>>
>> This (together with the resume side) makes me guess that the use of TYPE_ as
>> a prefix confused not just me, but also you:
>
> Yes, this is really not a good prefix, and probably pretty bad to use
> 'ERROR_'.
> What about 'PRIOR_'? then I also need to adjust device_power_up() as ...
What about SAVED_?
>> > - time_suspend();
>> > + if ( time_suspend() )
>> > + return TYPE_TIME;
>> >
>> > - i8259A_suspend();
>> > + if ( i8259A_suspend() )
>> > + return TYPE_I8259A;
>> >
>> > + /* ioapic_suspend should never fail */
>> > ioapic_suspend();
>>
>> The comment is bogus: "should" means it can in theory. Yet the function
>> having void return type means it just cannot fail.
>>
>
> I'll use 'cannot', instead of 'should'.
> Another question, I check the code again, and the rest of the functions
> (console_suspend/ time_suspend/ i8259A_suspend / ioapic_suspend /
> lapic_suspend ), in device_power_down(), always returned '0'.
> Maybe I need to fix these functions annotation from 'int' to 'void', and
> then I can add a comment on the device_power_down().
Please don't. Generally the possibility of failure exists, and hence if
functions have already been written to account for that, we shouldn't
strip that capability out.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending
2016-05-13 6:16 ` Jan Beulich
@ 2016-05-13 6:27 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-13 6:27 UTC (permalink / raw)
To: Jan Beulich
Cc: Tian, Kevin, Stefano Stabellini, Wu, Feng, Liu Jinsong,
dario.faggioli, xen-devel, Julien Grall, SuraveeSuthikulpanit,
Andrew Cooper, Keir Fraser
On May 13, 2016 2:17 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 13.05.16 at 05:39, <quan.xu@intel.com> wrote:
> > On May 10, 2016 5:25 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> >> > --- a/xen/arch/x86/acpi/power.c
> >> > +++ b/xen/arch/x86/acpi/power.c
> >> > static int device_power_down(void) {
> >> > - console_suspend();
> >> > + if ( console_suspend() )
> >> > + return TYPE_CONSOLE;
> >>
> >> This (together with the resume side) makes me guess that the use of
> >> TYPE_ as a prefix confused not just me, but also you:
> >
> > Yes, this is really not a good prefix, and probably pretty bad to use
> > 'ERROR_'.
> > What about 'PRIOR_'? then I also need to adjust device_power_up() as ...
>
> What about SAVED_?
>
much better, I'll take it in v5. Thanks.
> >> > - time_suspend();
> >> > + if ( time_suspend() )
> >> > + return TYPE_TIME;
> >> >
> >> > - i8259A_suspend();
> >> > + if ( i8259A_suspend() )
> >> > + return TYPE_I8259A;
> >> >
> >> > + /* ioapic_suspend should never fail */
> >> > ioapic_suspend();
> >>
> >> The comment is bogus: "should" means it can in theory. Yet the
> >> function having void return type means it just cannot fail.
> >>
> >
> > I'll use 'cannot', instead of 'should'.
> > Another question, I check the code again, and the rest of the
> > functions (console_suspend/ time_suspend/ i8259A_suspend /
> > ioapic_suspend / lapic_suspend ), in device_power_down(), always returned
> '0'.
> > Maybe I need to fix these functions annotation from 'int' to 'void',
> > and then I can add a comment on the device_power_down().
>
> Please don't. Generally the possibility of failure exists, and hence if functions
> have already been written to account for that, we shouldn't strip that
> capability out.
>
Got it.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-12 15:06 ` Jan Beulich
@ 2016-05-13 8:04 ` Xu, Quan
2016-05-13 9:08 ` Jan Beulich
0 siblings, 1 reply; 64+ messages in thread
From: Xu, Quan @ 2016-05-13 8:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Tian, Kevin, xen-devel
On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
> > On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> >> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
Jan,
Try it again, I hope I have got it. If not, could you write some sample code for me as an exception? :)
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -240,21 +240,63 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
unsigned int flags)
{
const struct domain_iommu *hd = dom_iommu(d);
+ static int printed = 0;
+ 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 ( unlikely(rc) )
+ {
+ if ( is_hardware_domain(d) )
+ {
+ if ( !printed )
+ {
+ printk(XENLOG_ERR
+ "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
+ d->domain_id, gfn, mfn, rc);
+
+ printed = 1;
+ }
+ }
+ else
+ domain_crash(d);
+ }
+
+ return rc;
}
int iommu_unmap_page(struct domain *d, unsigned long gfn)
{
const struct domain_iommu *hd = dom_iommu(d);
+ static int printed = 0;
+ 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 ( unlikely(rc) )
+ {
+ if ( is_hardware_domain(d) )
+ {
+ if ( !printed )
+ {
+ printk(XENLOG_ERR
+ "d%d: IOMMU unmapping gfn %#lx failed %d.",
+ d->domain_id, gfn, rc);
+
+ printed = 1;
+ }
+ }
+ else
+ domain_crash(d);
+ }
+
+ return rc;
}
Thanks again!!
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-13 8:04 ` Xu, Quan
@ 2016-05-13 9:08 ` Jan Beulich
2016-05-13 9:20 ` Xu, Quan
0 siblings, 1 reply; 64+ messages in thread
From: Jan Beulich @ 2016-05-13 9:08 UTC (permalink / raw)
To: Quan Xu; +Cc: dario.faggioli, Kevin Tian, xen-devel
>>> On 13.05.16 at 10:04, <quan.xu@intel.com> wrote:
> On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
>> > On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
>> >> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> Try it again, I hope I have got it. If not, could you write some sample code
> for me as an exception? :)
Well, this would be acceptable (albeit not ideal) to me as a first
cut (and instead of continuing this apparently fruitless discussion
I'd then see to provide a follow-up patch improving it), but won't
(I'm afraid) please Kevin: You now again don't log anything for
DomU-s. That's one half of the "not ideal" part; the other is that
of two far apart events for Dom0, only the first one would get
logged.
In any event there's stylistic cleanup necessary:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -240,21 +240,63 @@ int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
> unsigned int flags)
> {
> const struct domain_iommu *hd = dom_iommu(d);
> + static int printed = 0;
This doesn't need an initializer, should be bool_t, and should get
moved ...
> + 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 ( unlikely(rc) )
> + {
> + if ( is_hardware_domain(d) )
> + {
... here.
> + if ( !printed )
> + {
> + printk(XENLOG_ERR
> + "d%d: IOMMU mapping gfn %#lx mfn %#lx failed %d.",
> + d->domain_id, gfn, mfn, rc);
> +
> + printed = 1;
> + }
> + }
> + else
> + domain_crash(d);
> + }
What I think might at least come close to what Kevin and I would
like to see is something like
if ( !d->is_shutting_down && printk_ratelimit() )
printk(...);
if ( !is_hardware_domain(d) )
domain_crash(d);
For Dom0 that'll still be more verbose than we'd really like, but it
at least wouldn't outright flood the console.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures
2016-05-13 9:08 ` Jan Beulich
@ 2016-05-13 9:20 ` Xu, Quan
0 siblings, 0 replies; 64+ messages in thread
From: Xu, Quan @ 2016-05-13 9:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: dario.faggioli, Tian, Kevin, xen-devel
On May 13, 2016 5:09 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 13.05.16 at 10:04, <quan.xu@intel.com> wrote:
> > On May 12, 2016 11:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 12.05.16 at 16:28, <quan.xu@intel.com> wrote:
> >> > On May 10, 2016 2:54 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 10.05.16 at 05:41, <quan.xu@intel.com> wrote:
> >> >> > On May 10, 2016 12:14 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >> >>> On 06.05.16 at 10:54, <quan.xu@intel.com> wrote:
> What I think might at least come close to what Kevin and I would like to see is
> something like
>
> if ( !d->is_shutting_down && printk_ratelimit() )
> printk(...);
> if ( !is_hardware_domain(d) )
> domain_crash(d);
>
> For Dom0 that'll still be more verbose than we'd really like, but it at least
> wouldn't outright flood the console.
>
Thanks!!
It is really better to come close to Kevin's previous suggestion. I'll follow it for v5.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 64+ messages in thread
end of thread, other threads:[~2016-05-13 9:20 UTC | newest]
Thread overview: 64+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-06 8:54 [PATCH v4 00/10] Check VT-d Device-TLB flush error Quan Xu
2016-05-06 8:54 ` [PATCH v4 01/10] vt-d: fix the IOMMU flush issue Quan Xu
2016-05-09 16:09 ` Jan Beulich
2016-05-12 7:50 ` Xu, Quan
2016-05-12 8:53 ` Jan Beulich
2016-05-12 13:29 ` Xu, Quan
2016-05-12 13:37 ` Jan Beulich
2016-05-12 13:43 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 02/10] IOMMU: handle IOMMU mapping and unmapping failures Quan Xu
2016-05-09 16:13 ` Jan Beulich
2016-05-10 3:41 ` Xu, Quan
2016-05-10 6:53 ` Jan Beulich
2016-05-10 7:53 ` Xu, Quan
2016-05-10 8:02 ` Jan Beulich
2016-05-10 8:20 ` Xu, Quan
2016-05-10 8:26 ` Jan Beulich
2016-05-12 14:28 ` Xu, Quan
2016-05-12 15:06 ` Jan Beulich
2016-05-13 8:04 ` Xu, Quan
2016-05-13 9:08 ` Jan Beulich
2016-05-13 9:20 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Quan Xu
2016-05-10 8:44 ` Jan Beulich
2016-05-10 14:45 ` George Dunlap
2016-05-10 14:59 ` George Dunlap
2016-05-11 2:26 ` Xu, Quan
2016-05-11 8:45 ` George Dunlap
2016-05-11 8:58 ` Xu, Quan
2016-05-10 15:02 ` Jan Beulich
2016-05-11 2:29 ` Xu, Quan
2016-05-11 3:39 ` Xu, Quan
2016-05-11 7:02 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 04/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU unmapping Quan Xu
2016-05-10 8:50 ` Jan Beulich
2016-05-11 3:49 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 05/10] vt-d: propagate IOMMU Device-TLB flush error up to IOMMU mapping Quan Xu
2016-05-06 8:54 ` [PATCH v4 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones) Quan Xu
2016-05-10 9:04 ` Jan Beulich
2016-05-11 5:52 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 07/10] IOMMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (leaf ones) Quan Xu
2016-05-10 9:06 ` Jan Beulich
2016-05-11 6:47 ` Xu, Quan
2016-05-11 7:06 ` Jan Beulich
2016-05-11 7:12 ` Xu, Quan
2016-05-11 7:16 ` Jan Beulich
2016-05-11 7:20 ` Xu, Quan
2016-05-11 7:37 ` Jan Beulich
2016-05-06 8:54 ` [PATCH v4 08/10] vt-d/ept: propagate IOMMU Device-TLB flush error up to EPT update Quan Xu
2016-05-10 9:09 ` Jan Beulich
2016-05-10 14:58 ` George Dunlap
2016-05-10 15:04 ` Jan Beulich
2016-05-11 7:25 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 09/10] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending Quan Xu
2016-05-10 9:24 ` Jan Beulich
2016-05-13 3:39 ` Xu, Quan
2016-05-13 6:16 ` Jan Beulich
2016-05-13 6:27 ` Xu, Quan
2016-05-06 8:54 ` [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping Quan Xu
2016-05-10 9:29 ` Jan Beulich
2016-05-11 8:35 ` Xu, Quan
2016-05-11 9:07 ` Jan Beulich
2016-05-12 5:16 ` Xu, Quan
2016-05-12 8:44 ` Jan Beulich
2016-05-12 9:02 ` Xu, Quan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).