Re: [PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init
On 09/20/16 at 02:53pm, Joerg Roedel wrote:
> Hmm, have you tried hooking this into the set_dma_mask call-back? Every
> driver should call it for its device, so that should be a better
> indicator to now map a new domain.
Hi Joerg,
I tried hooking this into set_dma_mask call-back, but on my local
machine with amd iommu v2, an ohci pci device doesn't call set_dma_mask.
Then IO_PAGE_FAULT printing flooded.
00:12.0 USB controller: Advanced Micro Devices, Inc. [AMD] FCH USB OHCI
Controller (rev 11)
Below is the patch I made using set_dma_mask call-back. So should we do
as the original method, or use set_dma_mask method and add set_dma_mask
calling in those driver which missed calling it?
>From 1ca66f886a46839cb937fd1e6a1d84b6719f23c4 Mon Sep 17 00:00:00 2001
From: Baoquan He
Date: Mon, 12 Sep 2016 08:05:15 +0800
Subject: [PATCH] iommu/amd: Update domain into to dte entry during device
driver init
All devices are supposed to reset themselves at device driver initialization
stage. At this time if in kdump kernel those on-flight DMA will be stopped
because of device reset. It's best time to update the protection domain info,
especially pte_root, to dte entry which the device relates to. Usually
device driver initialization will call set_dma_mask to set the
limitation of dma address. Do it in set_dma_mask call-back is a good
chance.
Signed-off-by: Baoquan He
---
drivers/iommu/amd_iommu.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6c37300..2a0b1ce 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2653,6 +2653,27 @@ static int amd_iommu_dma_supported(struct device *dev,
u64 mask)
return check_device(dev);
}
+static int set_dma_mask(struct device *dev, u64 mask)
+{
+ struct iommu_dev_data *dev_data = get_dev_data(dev);
+ struct protection_domain *domain = get_domain(dev);
+ u16 alias = amd_iommu_alias_table[dev_data->devid];
+ struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
+
+ if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
+ dev_data->domain_updated = true;
+ set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
+ if (alias != dev_data->devid)
+ set_dte_entry(alias, domain, dev_data->ats.enabled);
+ device_flush_dte(dev_data);
+ }
+
+ if (!dev->dma_mask || !amd_iommu_dma_supported(dev, mask))
+ return -EIO;
+ *dev->dma_mask = mask;
+ return 0;
+}
+
static struct dma_map_ops amd_iommu_dma_ops = {
.alloc = alloc_coherent,
.free = free_coherent,
@@ -2661,6 +2682,7 @@ static struct dma_map_ops amd_iommu_dma_ops = {
.map_sg = map_sg,
.unmap_sg = unmap_sg,
.dma_supported = amd_iommu_dma_supported,
+ .set_dma_mask = set_dma_mask,
};
static int init_reserved_iova_ranges(void)
--
2.5.5
Re: [PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init
On 09/20/16 at 02:53pm, Joerg Roedel wrote:
> On Thu, Sep 15, 2016 at 11:03:26PM +0800, Baoquan He wrote:
> > All devices are supposed to reset themselves at device driver initialization
> > stage. At this time if in kdump kernel those on-flight DMA will be stopped
> > because of device reset. It's best time to update the protection domain
> > info,
> > especially pte_root, to dte entry which the device relates to.
> >
> > Signed-off-by: Baoquan He
> > ---
> > drivers/iommu/amd_iommu.c | 21 +
> > 1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > index 6c37300..00b64ee 100644
> > --- a/drivers/iommu/amd_iommu.c
> > +++ b/drivers/iommu/amd_iommu.c
> > @@ -2310,6 +2310,10 @@ static dma_addr_t __map_single(struct device *dev,
> > unsigned int pages;
> > int prot = 0;
> > int i;
> > + struct iommu_dev_data *dev_data = get_dev_data(dev);
> > + struct protection_domain *domain = get_domain(dev);
> > + u16 alias = amd_iommu_alias_table[dev_data->devid];
> > + struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
> >
> > pages = iommu_num_pages(paddr, size, PAGE_SIZE);
> > paddr &= PAGE_MASK;
> > @@ -2319,6 +2323,13 @@ static dma_addr_t __map_single(struct device *dev,
> > goto out;
> >
> > prot = dir2prot(direction);
> > + if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
> > + dev_data->domain_updated = true;
> > + set_dte_entry(dev_data->devid, domain,
> > dev_data->ats.enabled);
> > + if (alias != dev_data->devid)
> > + set_dte_entry(alias, domain, dev_data->ats.enabled);
> > + device_flush_dte(dev_data);
> > + }
>
> Hmm, have you tried hooking this into the set_dma_mask call-back? Every
> driver should call it for its device, so that should be a better
> indicator to now map a new domain.
Very earlier you mentioned this, and I tried, but failed. I guess that
is because of the bnx2 NIC resetting problem. Let me try it again. In
theory it should be better. Just sometime people probably don't call
set_dma_mask explicitly, then default dma mask is used. This could be
seen in those simple device. For those complicated device like sata disk
and ethernet NIC, set_dma_mask should be called. So I would like to try
and use set_dma_mask. Thanks for your great suggestion.
Thanks
Baoquan
Re: [PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init
On Thu, Sep 15, 2016 at 11:03:26PM +0800, Baoquan He wrote:
> All devices are supposed to reset themselves at device driver initialization
> stage. At this time if in kdump kernel those on-flight DMA will be stopped
> because of device reset. It's best time to update the protection domain info,
> especially pte_root, to dte entry which the device relates to.
>
> Signed-off-by: Baoquan He
> ---
> drivers/iommu/amd_iommu.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 6c37300..00b64ee 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2310,6 +2310,10 @@ static dma_addr_t __map_single(struct device *dev,
> unsigned int pages;
> int prot = 0;
> int i;
> + struct iommu_dev_data *dev_data = get_dev_data(dev);
> + struct protection_domain *domain = get_domain(dev);
> + u16 alias = amd_iommu_alias_table[dev_data->devid];
> + struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
>
> pages = iommu_num_pages(paddr, size, PAGE_SIZE);
> paddr &= PAGE_MASK;
> @@ -2319,6 +2323,13 @@ static dma_addr_t __map_single(struct device *dev,
> goto out;
>
> prot = dir2prot(direction);
> + if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
> + dev_data->domain_updated = true;
> + set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
> + if (alias != dev_data->devid)
> + set_dte_entry(alias, domain, dev_data->ats.enabled);
> + device_flush_dte(dev_data);
> + }
Hmm, have you tried hooking this into the set_dma_mask call-back? Every
driver should call it for its device, so that should be a better
indicator to now map a new domain.
Joerg
[PATCH v5 8/8] iommu/amd: Update domain into to dte entry during device driver init
All devices are supposed to reset themselves at device driver initialization
stage. At this time if in kdump kernel those on-flight DMA will be stopped
because of device reset. It's best time to update the protection domain info,
especially pte_root, to dte entry which the device relates to.
Signed-off-by: Baoquan He
---
drivers/iommu/amd_iommu.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 6c37300..00b64ee 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2310,6 +2310,10 @@ static dma_addr_t __map_single(struct device *dev,
unsigned int pages;
int prot = 0;
int i;
+ struct iommu_dev_data *dev_data = get_dev_data(dev);
+ struct protection_domain *domain = get_domain(dev);
+ u16 alias = amd_iommu_alias_table[dev_data->devid];
+ struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
pages = iommu_num_pages(paddr, size, PAGE_SIZE);
paddr &= PAGE_MASK;
@@ -2319,6 +2323,13 @@ static dma_addr_t __map_single(struct device *dev,
goto out;
prot = dir2prot(direction);
+ if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
+ dev_data->domain_updated = true;
+ set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
+ if (alias != dev_data->devid)
+ set_dte_entry(alias, domain, dev_data->ats.enabled);
+ device_flush_dte(dev_data);
+ }
start = address;
for (i = 0; i < pages; ++i) {
@@ -2470,6 +2481,9 @@ static int map_sg(struct device *dev, struct scatterlist
*sglist,
struct scatterlist *s;
unsigned long address;
u64 dma_mask;
+ struct iommu_dev_data *dev_data = get_dev_data(dev);
+ u16 alias = amd_iommu_alias_table[dev_data->devid];
+ struct amd_iommu *iommu = amd_iommu_rlookup_table[dev_data->devid];
domain = get_domain(dev);
if (IS_ERR(domain))
@@ -2485,6 +2499,13 @@ static int map_sg(struct device *dev, struct scatterlist
*sglist,
goto out_err;
prot = dir2prot(direction);
+ if (translation_pre_enabled(iommu) && !dev_data->domain_updated) {
+ dev_data->domain_updated = true;
+ set_dte_entry(dev_data->devid, domain, dev_data->ats.enabled);
+ if (alias != dev_data->devid)
+ set_dte_entry(alias, domain, dev_data->ats.enabled);
+ device_flush_dte(dev_data);
+ }
/* Map all sg entries */
for_each_sg(sglist, s, nelems, i) {
--
2.5.5
