Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
Hi Alex,
On 2020/7/30 4:25, Alex Williamson wrote:
On Tue, 14 Jul 2020 13:57:02 +0800
Lu Baolu wrote:
The device driver needs an API to get its aux-domain. A typical usage
scenario is:
unsigned long pasid;
struct iommu_domain *domain;
struct device *dev = mdev_dev(mdev);
struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
domain = iommu_aux_get_domain_for_dev(dev);
if (!domain)
return -ENODEV;
pasid = iommu_aux_get_pasid(domain, iommu_device);
if (pasid <= 0)
return -EINVAL;
/* Program the device context */
This adds an API for such use case.
Suggested-by: Alex Williamson
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 18 ++
include/linux/iommu.h | 7 +++
2 files changed, 25 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cad5a19ebf22..434bf42b6b9b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
}
EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
+{
+ struct iommu_domain *domain = NULL;
+ struct iommu_group *group;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return NULL;
+
+ if (group->aux_domain_attached)
+ domain = group->domain;
Why wouldn't the aux domain flag be on the domain itself rather than
the group? Then if we wanted sanity checking in patch 1/ we'd only
need to test the flag on the object we're provided.
Agreed. Given that a group may contain both non-aux and aux devices,
adding such flag in iommu_group doesn't make sense.
If we had such a flag, we could create an iommu_domain_is_aux()
function and then simply use iommu_get_domain_for_dev() and test that
it's an aux domain in the example use case. It seems like that would
resolve the jump from a domain to an aux-domain just as well as adding
this separate iommu_aux_get_domain_for_dev() interface. The is_aux
test might also be useful in other cases too.
Let's rehearsal our use case.
unsigned long pasid;
struct iommu_domain *domain;
struct device *dev = mdev_dev(mdev);
struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
[1] domain = iommu_get_domain_for_dev(dev);
if (!domain)
return -ENODEV;
[2] pasid = iommu_aux_get_pasid(domain, iommu_device);
if (pasid <= 0)
return -EINVAL;
/* Program the device context */
The reason why I add this iommu_aux_get_domain_for_dev() is that we need
to make sure the domain got at [1] is valid to be used at [2].
https://lore.kernel.org/linux-iommu/20200707150408.474d8...@x1.home/
When calling into iommu_aux_get_pasid(), the iommu driver should make
sure that @domain is a valid aux-domain for @iommu_device. Hence, for
our use case, it seems that there's no need for a is_aux_domain() api.
Anyway, I'm not against adding a new is_aux_domain() api if there's a
need elsewhere.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/arm-smmu: Implement qcom,skip-init
On Wed 22 Jul 13:11 PDT 2020, Konrad Dybcio wrote: > >Is the problem on SDM630 that when you write to SMR/S2CR the device > >reboots? Or that when you start writing out the context bank > >configuration that trips the display and the device reboots? > > I added some debug prints and the phone hangs after reaching the > seventh CB (with i=6) at > > arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_FSR_FAULT); > > line in arm_smmu_device_reset. > Sounds like things are progressing nicely for a while there, presumably until the next time the display is being refreshed. Would you be willing to try out the following work in progress: https://lore.kernel.org/linux-arm-msm/20200717001619.325317-1-bjorn.anders...@linaro.org/ You need to adjust drivers/iommu/arm-smmu-impl.c so that arm_smmu_impl_init() will invoke qcom_smmu_impl_init() as it spots your apps smmu. Regards, Bjorn ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
Hi Alex,
On 2020/7/31 3:46, Alex Williamson wrote:
On Wed, 29 Jul 2020 23:34:40 +
"Tian, Kevin" wrote:
From: Alex Williamson
Sent: Thursday, July 30, 2020 4:04 AM
On Thu, 16 Jul 2020 09:07:46 +0800
Lu Baolu wrote:
Hi Jacob,
On 7/16/20 12:01 AM, Jacob Pan wrote:
On Wed, 15 Jul 2020 08:47:36 +0800
Lu Baolu wrote:
Hi Jacob,
On 7/15/20 12:39 AM, Jacob Pan wrote:
On Tue, 14 Jul 2020 13:57:01 +0800
Lu Baolu wrote:
This adds two new aux-domain APIs for a use case like vfio/mdev
where sub-devices derived from an aux-domain capable device are
created and put in an iommu_group.
/**
* iommu_aux_attach_group - attach an aux-domain to an
iommu_group
which
* contains sub-devices (for example
mdevs) derived
* from @dev.
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
@dev;
* @dev:the physical device which supports
IOMMU_DEV_FEAT_AUX.
*
* Returns 0 on success, or an error value.
*/
int iommu_aux_attach_group(struct iommu_domain *domain,
struct iommu_group *group,
struct device *dev)
/**
* iommu_aux_detach_group - detach an aux-domain from an
iommu_group *
* @domain: an aux-domain;
* @group: an iommu_group which contains sub-devices derived
from
@dev;
* @dev:the physical device which supports
IOMMU_DEV_FEAT_AUX.
*
* @domain must have been attached to @group via
iommu_aux_attach_group(). */
void iommu_aux_detach_group(struct iommu_domain *domain,
struct iommu_group *group,
struct device *dev)
It also adds a flag in the iommu_group data structure to identify
an iommu_group with aux-domain attached from those normal ones.
Signed-off-by: Lu Baolu
---
drivers/iommu/iommu.c | 58
+++
include/linux/iommu.h |
17 + 2 files changed, 75 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e1fdd3531d65..cad5a19ebf22 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -45,6 +45,7 @@ struct iommu_group {
struct iommu_domain *default_domain;
struct iommu_domain *domain;
struct list_head entry;
+ unsigned int aux_domain_attached:1;
};
struct group_device {
@@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
iommu_domain
*domain, struct device *dev) }
EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
+/**
+ * iommu_aux_attach_group - attach an aux-domain to an
iommu_group
which
+ * contains sub-devices (for example
mdevs) derived
+ * from @dev.
+ * @domain: an aux-domain;
+ * @group: an iommu_group which contains sub-devices derived
from
@dev;
+ * @dev:the physical device which supports
IOMMU_DEV_FEAT_AUX.
+ *
+ * Returns 0 on success, or an error value.
+ */
+int iommu_aux_attach_group(struct iommu_domain *domain,
+ struct iommu_group *group, struct
device *dev) +{
+ int ret = -EBUSY;
+
+ mutex_lock(&group->mutex);
+ if (group->domain)
+ goto out_unlock;
+
Perhaps I missed something but are we assuming only one mdev per
mdev group? That seems to change the logic where vfio does:
iommu_group_for_each_dev()
iommu_aux_attach_device()
It has been changed in PATCH 4/4:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
if (group->mdev_group)
return iommu_aux_attach_group(domain->domain,
group->iommu_group,
group->iommu_device);
else
return iommu_attach_group(domain->domain,
group->iommu_group);
}
So, for both normal domain and aux-domain, we use the same concept:
attach a domain to a group.
I get that, but don't you have to attach all the devices within the
This iommu_group includes only mediated devices derived from an
IOMMU_DEV_FEAT_AUX-capable device. Different from
iommu_attach_group(),
iommu_aux_attach_group() doesn't need to attach the domain to each
device in group, instead it only needs to attach the domain to the
physical device where the mdev's were created from.
group? Here you see the group already has a domain and exit.
If the (group->domain) has been set, that means a domain has already
attached to the group, so it returns -EBUSY.
I agree with Jacob, singleton groups should not be built into the IOMMU
API, we're not building an interface just for mdevs or current
limitations of mdevs. This also means that setting a flag on the group
and passing a device that's assumed to be common for all devices within
the group, don't really make sense here. Thanks,
Alex
Baolu and I discussed about this
RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
> From: Tian, Kevin
> Sent: Friday, July 31, 2020 8:26 AM
>
> > From: Alex Williamson
> > Sent: Friday, July 31, 2020 4:17 AM
> >
> > On Wed, 29 Jul 2020 23:49:20 +
> > "Tian, Kevin" wrote:
> >
> > > > From: Alex Williamson
> > > > Sent: Thursday, July 30, 2020 4:25 AM
> > > >
> > > > On Tue, 14 Jul 2020 13:57:02 +0800
> > > > Lu Baolu wrote:
> > > >
> > > > > The device driver needs an API to get its aux-domain. A typical usage
> > > > > scenario is:
> > > > >
> > > > > unsigned long pasid;
> > > > > struct iommu_domain *domain;
> > > > > struct device *dev = mdev_dev(mdev);
> > > > > struct device *iommu_device =
> vfio_mdev_get_iommu_device(dev);
> > > > >
> > > > > domain = iommu_aux_get_domain_for_dev(dev);
> > > > > if (!domain)
> > > > > return -ENODEV;
> > > > >
> > > > > pasid = iommu_aux_get_pasid(domain, iommu_device);
> > > > > if (pasid <= 0)
> > > > > return -EINVAL;
> > > > >
> > > > > /* Program the device context */
> > > > >
> > > > >
> > > > > This adds an API for such use case.
> > > > >
> > > > > Suggested-by: Alex Williamson
> > > > > Signed-off-by: Lu Baolu
> > > > > ---
> > > > > drivers/iommu/iommu.c | 18 ++
> > > > > include/linux/iommu.h | 7 +++
> > > > > 2 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > > index cad5a19ebf22..434bf42b6b9b 100644
> > > > > --- a/drivers/iommu/iommu.c
> > > > > +++ b/drivers/iommu/iommu.c
> > > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
> > > > iommu_domain *domain,
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
> > > > >
> > > > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct
> > device
> > > > *dev)
> > > > > +{
> > > > > + struct iommu_domain *domain = NULL;
> > > > > + struct iommu_group *group;
> > > > > +
> > > > > + group = iommu_group_get(dev);
> > > > > + if (!group)
> > > > > + return NULL;
> > > > > +
> > > > > + if (group->aux_domain_attached)
> > > > > + domain = group->domain;
> > > >
> > > > Why wouldn't the aux domain flag be on the domain itself rather than
> > > > the group? Then if we wanted sanity checking in patch 1/ we'd only
> > > > need to test the flag on the object we're provided.
> > > >
> > > > If we had such a flag, we could create an iommu_domain_is_aux()
> > > > function and then simply use iommu_get_domain_for_dev() and test
> that
> > > > it's an aux domain in the example use case. It seems like that would
> > >
> > > IOMMU layer manages domains per parent device. Here given a
> >
> > Is this the IOMMU layer or the VT-d driver? I don't see any notion of
> > managing domains relative to a parent in the IOMMU layer. Please point
> > to something more specific if I'm wrong here.
>
> it's maintained in VT-d driver (include/linux/intel-iommu.h)
>
> struct device_domain_info {
> struct list_head link; /* link to domain siblings */
> struct list_head global; /* link to global list */
> struct list_head table; /* link to pasid table */
> struct list_head auxiliary_domains; /* auxiliary domains
> * attached to this device
> */
> ...
>
> >
> > > dev (of mdev), we need a way to find its associated domain under its
> > > parent device. And we cannot simply use iommu_get_domain_for_dev
> > > on the parent device of the mdev, as it will give us the primary domain
> > > of parent device.
> >
> > Not the parent device of the mdev, but the mdev_dev(mdev) device.
> > Isn't that what this series is enabling, being able to return the
> > domain from the group that contains the mdev_dev? We shouldn't need
> to
> > leave breadcrumbs on the group to know about the domain, the domain
> > itself should be the source of knowledge, or provide a mechanism/ops to
> > learn that knowledge. Thanks,
> >
> > Alex
>
> It's the tradeoff between leaving breadcrumb in domain or in group.
> Today the domain has no knowledge of mdev. It just includes a list
> of physical devices which are attached to the domain (either due to
> the device is assigned in a whole or as the parent device of an assigned
> mdev). Then we have two choices. One is to save the mdev_dev info
> in device_domain_info and maintain a mapping between mdev_dev
> and related aux domain. The other is to record the domain info directly
> in group. Earlier we choose the latter one as it looks simpler. If you
> prefer to the former one, we can think more and have a try.
>
Please skip this comment. I have a wrong understanding of the problem
and is discussing with Baolu. He will reply with our conclusion later.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.li
Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
Hi Yi,
On 7/30/20 5:36 PM, Liu, Yi L wrote:
From: Lu Baolu
Sent: Tuesday, July 14, 2020 1:57 PM
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group
data structure so that it could be reused in other places.
Signed-off-by: Lu Baolu
---
drivers/vfio/vfio_iommu_type1.c | 44 ++---
1 file changed, 7 insertions(+), 37 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..f8812e68de77 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -100,6 +100,7 @@ struct vfio_dma {
struct vfio_group {
struct iommu_group *iommu_group;
struct list_headnext;
+ struct device *iommu_device;
I know mdev group has only one device, so such a group has a single
iommu_device. But I guess may be helpful to add a comment here or in
commit message. Otherwise, it looks weird that a group structure
contains a single iommu_device field instead of a list of iommu_device.
Right! I will add some comments if this is still needed in the next
version.
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
Hi Alex,
On 7/31/20 5:17 AM, Alex Williamson wrote:
On Thu, 30 Jul 2020 10:41:32 +0800
Lu Baolu wrote:
Hi Alex,
On 7/30/20 4:32 AM, Alex Williamson wrote:
On Tue, 14 Jul 2020 13:57:03 +0800
Lu Baolu wrote:
Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the
vfio_group data structure so that it could be reused in other places.
Signed-off-by: Lu Baolu
---
drivers/vfio/vfio_iommu_type1.c | 44 ++---
1 file changed, 7 insertions(+), 37 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..f8812e68de77 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -100,6 +100,7 @@ struct vfio_dma {
struct vfio_group {
struct iommu_group *iommu_group;
struct list_headnext;
+ struct device *iommu_device;
boolmdev_group; /* An mdev group */
boolpinned_page_dirty_scope;
};
@@ -1627,45 +1628,13 @@ static struct device *vfio_mdev_get_iommu_device(struct
device *dev)
return NULL;
}
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = vfio_mdev_get_iommu_device(dev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- return iommu_aux_attach_device(domain, iommu_device);
- else
- return iommu_attach_device(domain, iommu_device);
- }
-
- return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = vfio_mdev_get_iommu_device(dev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- iommu_aux_detach_device(domain, iommu_device);
- else
- iommu_detach_device(domain, iommu_device);
- }
-
- return 0;
-}
-
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
if (group->mdev_group)
- return iommu_group_for_each_dev(group->iommu_group,
- domain->domain,
- vfio_mdev_attach_domain);
+ return iommu_aux_attach_group(domain->domain,
+ group->iommu_group,
+ group->iommu_device);
No, we previously iterated all devices in the group and used the aux
interface only when we have an iommu_device supporting aux. If we
simply assume an mdev group only uses an aux domain we break existing
users, ex. SR-IOV VF backed mdevs. Thanks,
Oh, yes. Sorry! I didn't consider the physical device backed mdevs
cases.
Looked into this part of code, it seems that there's a lock issue here.
The group->mutex is held in iommu_group_for_each_dev() and will be
acquired again in iommu_attach_device().
These are two different groups. We walk the devices in the mdev's
group with iommu_group_for_each_dev(), holding the mdev's group lock,
but we call iommu_attach_device() with iommu_device, which results in
acquiring the lock for the iommu_device's group.
You are right. Sorry for the noise. Please ignore it.
How about making it like:
static int vfio_iommu_attach_group(struct vfio_domain *domain,
struct vfio_group *group)
{
if (group->mdev_group) {
struct device *iommu_device = group->iommu_device;
if (WARN_ON(!iommu_device))
return -EINVAL;
if (iommu_dev_feature_enabled(iommu_device,
IOMMU_DEV_FEAT_AUX))
return iommu_aux_attach_device(domain->domain,
iommu_device);
else
return iommu_attach_device(domain->domain,
iommu_device);
} else {
return iommu_attach_group(domain->domain,
group->iommu_group);
}
}
The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs
in an iommu group should be derived from a same physical device.
Have we?
We have done this with below.
static int vfio_mdev_iommu_device(struct device *dev, void *data)
{
struct device **old = data, *new;
new = vfio_mdev_get_iommu_device(dev);
if (!new || (*old && *old != new))
return -EINVAL;
*old = new;
return 0;
}
But I agree that as a generic iommu aux-domain api, we shouldn't put
this limited assumption in it.
iommu_attach_device() will f
Re: [PATCH] dma-pool: Do not allocate pool memory from CMA
On Tue, Jul 28, 2020 at 12:09:18PM +0200, Christoph Hellwig wrote:
> Ok, I found a slight bug that wasn't intended. I wanted to make sure
> we can always fall back to a lower pool, but got that wrong. Should be
> fixed in the next version.
Hi Christoph and Nicolas,
Did a version of that series ever get send out? I am coming into the
conversation late but I am running into an issue with the Raspberry Pi 4
not booting on linux-next, which appears to be due to this patch now in
mainline as commit d9765e41d8e9 ("dma-pool: do not allocate pool memory
from CMA") combined with
https://lore.kernel.org/lkml/20200725014529.1143208-2-jiaxun.y...@flygoat.com/
in -next:
[1.423163] raspberrypi-firmware soc:firmware: Request 0x0001 returned
status 0x
[1.431883] raspberrypi-firmware soc:firmware: Request 0x00030046 returned
status 0x
[1.443888] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.452527] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 0
config (-22 80)
[1.460836] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.469445] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
config (-22 81)
[1.477735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.486350] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
config (-22 82)
[1.494639] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.503246] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 3
config (-22 83)
[1.511529] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.520131] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
config (-22 84)
[1.528414] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.537017] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 5
config (-22 85)
[1.545299] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.553903] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
config (-22 86)
[1.562184] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.570787] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 7
config (-22 87)
[1.579897] raspberrypi-firmware soc:firmware: Request 0x00030030 returned
status 0x
[1.589419] raspberrypi-firmware soc:firmware: Request 0x00028001 returned
status 0x
[1.599391] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.608018] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
config (-22 81)
[1.616313] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.624932] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
config (-22 81)
[1.633195] pwrseq_simple: probe of wifi-pwrseq failed with error -22
[1.643904] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.652544] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
config (-22 82)
[1.660839] raspberrypi-firmware soc:firmware: Request 0x00030041 returned
status 0x
[1.669446] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
state (-22 82)
[1.677727] leds-gpio: probe of leds failed with error -22
[1.683735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.692346] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
config (-22 86)
[1.700636] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.709240] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
config (-22 86)
[1.717496] reg-fixed-voltage: probe of sd_vcc_reg failed with error -22
[1.725546] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.734176] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
config (-22 84)
[1.742465] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
status 0x
[1.751072] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
config (-22 84)
[1.759332] gpio-regulator: probe of sd_io_1v8_reg failed with error -22
[1.768042] raspberrypi-firmware soc:firmware: Request 0x00028001 returned
status 0x
[1.780871] ALSA device list:
[1.783960] No soundcards found.
[1.787633] Waiting for root device PARTUUID=45a8dd8a-02...
I am unsure if it is related to the issue that Amit is having or
if that makes sense at all but I can reliably reproduce it.
v5.8-rc1: OK
v5.8-rc1 + d9765e41d8e9e: OK
v5.8-rc1 + "of_address: Add bus type match for pci ranges parser": OK
v5.8-rc1 + both: BROKEN
I wanted to test the series to see if this fixes anything. If you would
prefer a different thread for this or further i
RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
> From: Alex Williamson
> Sent: Friday, July 31, 2020 4:17 AM
>
> On Wed, 29 Jul 2020 23:49:20 +
> "Tian, Kevin" wrote:
>
> > > From: Alex Williamson
> > > Sent: Thursday, July 30, 2020 4:25 AM
> > >
> > > On Tue, 14 Jul 2020 13:57:02 +0800
> > > Lu Baolu wrote:
> > >
> > > > The device driver needs an API to get its aux-domain. A typical usage
> > > > scenario is:
> > > >
> > > > unsigned long pasid;
> > > > struct iommu_domain *domain;
> > > > struct device *dev = mdev_dev(mdev);
> > > > struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
> > > >
> > > > domain = iommu_aux_get_domain_for_dev(dev);
> > > > if (!domain)
> > > > return -ENODEV;
> > > >
> > > > pasid = iommu_aux_get_pasid(domain, iommu_device);
> > > > if (pasid <= 0)
> > > > return -EINVAL;
> > > >
> > > > /* Program the device context */
> > > >
> > > >
> > > > This adds an API for such use case.
> > > >
> > > > Suggested-by: Alex Williamson
> > > > Signed-off-by: Lu Baolu
> > > > ---
> > > > drivers/iommu/iommu.c | 18 ++
> > > > include/linux/iommu.h | 7 +++
> > > > 2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index cad5a19ebf22..434bf42b6b9b 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
> > > iommu_domain *domain,
> > > > }
> > > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
> > > >
> > > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct
> device
> > > *dev)
> > > > +{
> > > > + struct iommu_domain *domain = NULL;
> > > > + struct iommu_group *group;
> > > > +
> > > > + group = iommu_group_get(dev);
> > > > + if (!group)
> > > > + return NULL;
> > > > +
> > > > + if (group->aux_domain_attached)
> > > > + domain = group->domain;
> > >
> > > Why wouldn't the aux domain flag be on the domain itself rather than
> > > the group? Then if we wanted sanity checking in patch 1/ we'd only
> > > need to test the flag on the object we're provided.
> > >
> > > If we had such a flag, we could create an iommu_domain_is_aux()
> > > function and then simply use iommu_get_domain_for_dev() and test that
> > > it's an aux domain in the example use case. It seems like that would
> >
> > IOMMU layer manages domains per parent device. Here given a
>
> Is this the IOMMU layer or the VT-d driver? I don't see any notion of
> managing domains relative to a parent in the IOMMU layer. Please point
> to something more specific if I'm wrong here.
it's maintained in VT-d driver (include/linux/intel-iommu.h)
struct device_domain_info {
struct list_head link; /* link to domain siblings */
struct list_head global; /* link to global list */
struct list_head table; /* link to pasid table */
struct list_head auxiliary_domains; /* auxiliary domains
* attached to this device
*/
...
>
> > dev (of mdev), we need a way to find its associated domain under its
> > parent device. And we cannot simply use iommu_get_domain_for_dev
> > on the parent device of the mdev, as it will give us the primary domain
> > of parent device.
>
> Not the parent device of the mdev, but the mdev_dev(mdev) device.
> Isn't that what this series is enabling, being able to return the
> domain from the group that contains the mdev_dev? We shouldn't need to
> leave breadcrumbs on the group to know about the domain, the domain
> itself should be the source of knowledge, or provide a mechanism/ops to
> learn that knowledge. Thanks,
>
> Alex
It's the tradeoff between leaving breadcrumb in domain or in group.
Today the domain has no knowledge of mdev. It just includes a list
of physical devices which are attached to the domain (either due to
the device is assigned in a whole or as the parent device of an assigned
mdev). Then we have two choices. One is to save the mdev_dev info
in device_domain_info and maintain a mapping between mdev_dev
and related aux domain. The other is to record the domain info directly
in group. Earlier we choose the latter one as it looks simpler. If you
prefer to the former one, we can think more and have a try.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
On Thu, 30 Jul 2020 10:41:32 +0800
Lu Baolu wrote:
> Hi Alex,
>
> On 7/30/20 4:32 AM, Alex Williamson wrote:
> > On Tue, 14 Jul 2020 13:57:03 +0800
> > Lu Baolu wrote:
> >
> >> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
> >> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the
> >> vfio_group data structure so that it could be reused in other places.
> >>
> >> Signed-off-by: Lu Baolu
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 44 ++---
> >> 1 file changed, 7 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c
> >> b/drivers/vfio/vfio_iommu_type1.c
> >> index 5e556ac9102a..f8812e68de77 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -100,6 +100,7 @@ struct vfio_dma {
> >> struct vfio_group {
> >>struct iommu_group *iommu_group;
> >>struct list_headnext;
> >> + struct device *iommu_device;
> >>boolmdev_group; /* An mdev group */
> >>boolpinned_page_dirty_scope;
> >> };
> >> @@ -1627,45 +1628,13 @@ static struct device
> >> *vfio_mdev_get_iommu_device(struct device *dev)
> >>return NULL;
> >> }
> >>
> >> -static int vfio_mdev_attach_domain(struct device *dev, void *data)
> >> -{
> >> - struct iommu_domain *domain = data;
> >> - struct device *iommu_device;
> >> -
> >> - iommu_device = vfio_mdev_get_iommu_device(dev);
> >> - if (iommu_device) {
> >> - if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> >> - return iommu_aux_attach_device(domain, iommu_device);
> >> - else
> >> - return iommu_attach_device(domain, iommu_device);
> >> - }
> >> -
> >> - return -EINVAL;
> >> -}
> >> -
> >> -static int vfio_mdev_detach_domain(struct device *dev, void *data)
> >> -{
> >> - struct iommu_domain *domain = data;
> >> - struct device *iommu_device;
> >> -
> >> - iommu_device = vfio_mdev_get_iommu_device(dev);
> >> - if (iommu_device) {
> >> - if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
> >> - iommu_aux_detach_device(domain, iommu_device);
> >> - else
> >> - iommu_detach_device(domain, iommu_device);
> >> - }
> >> -
> >> - return 0;
> >> -}
> >> -
> >> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> >> struct vfio_group *group)
> >> {
> >>if (group->mdev_group)
> >> - return iommu_group_for_each_dev(group->iommu_group,
> >> - domain->domain,
> >> - vfio_mdev_attach_domain);
> >> + return iommu_aux_attach_group(domain->domain,
> >> +group->iommu_group,
> >> +group->iommu_device);
> >
> > No, we previously iterated all devices in the group and used the aux
> > interface only when we have an iommu_device supporting aux. If we
> > simply assume an mdev group only uses an aux domain we break existing
> > users, ex. SR-IOV VF backed mdevs. Thanks,
>
> Oh, yes. Sorry! I didn't consider the physical device backed mdevs
> cases.
>
> Looked into this part of code, it seems that there's a lock issue here.
> The group->mutex is held in iommu_group_for_each_dev() and will be
> acquired again in iommu_attach_device().
These are two different groups. We walk the devices in the mdev's
group with iommu_group_for_each_dev(), holding the mdev's group lock,
but we call iommu_attach_device() with iommu_device, which results in
acquiring the lock for the iommu_device's group.
> How about making it like:
>
> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> struct vfio_group *group)
> {
> if (group->mdev_group) {
> struct device *iommu_device = group->iommu_device;
>
> if (WARN_ON(!iommu_device))
> return -EINVAL;
>
> if (iommu_dev_feature_enabled(iommu_device,
> IOMMU_DEV_FEAT_AUX))
> return iommu_aux_attach_device(domain->domain,
> iommu_device);
> else
> return iommu_attach_device(domain->domain,
> iommu_device);
> } else {
> return iommu_attach_group(domain->domain,
> group->iommu_group);
> }
> }
>
> The caller (vfio_iommu_type1_attach_group) has guaranteed that all mdevs
> in an iommu group should be derived from a same physical device.
Have we? iommu_attach_device() will fail if the group is not
singleton, but that's just encouraging us to use the _attach_group()
interface where the _attach_device() interface is relegated to special
cases. Ideally we'd get out of those special cases and create an
_attach_group() for aux that doesn't
Re: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
On Wed, 29 Jul 2020 23:49:20 +
"Tian, Kevin" wrote:
> > From: Alex Williamson
> > Sent: Thursday, July 30, 2020 4:25 AM
> >
> > On Tue, 14 Jul 2020 13:57:02 +0800
> > Lu Baolu wrote:
> >
> > > The device driver needs an API to get its aux-domain. A typical usage
> > > scenario is:
> > >
> > > unsigned long pasid;
> > > struct iommu_domain *domain;
> > > struct device *dev = mdev_dev(mdev);
> > > struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
> > >
> > > domain = iommu_aux_get_domain_for_dev(dev);
> > > if (!domain)
> > > return -ENODEV;
> > >
> > > pasid = iommu_aux_get_pasid(domain, iommu_device);
> > > if (pasid <= 0)
> > > return -EINVAL;
> > >
> > > /* Program the device context */
> > >
> > >
> > > This adds an API for such use case.
> > >
> > > Suggested-by: Alex Williamson
> > > Signed-off-by: Lu Baolu
> > > ---
> > > drivers/iommu/iommu.c | 18 ++
> > > include/linux/iommu.h | 7 +++
> > > 2 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index cad5a19ebf22..434bf42b6b9b 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
> > iommu_domain *domain,
> > > }
> > > EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
> > >
> > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
> > *dev)
> > > +{
> > > + struct iommu_domain *domain = NULL;
> > > + struct iommu_group *group;
> > > +
> > > + group = iommu_group_get(dev);
> > > + if (!group)
> > > + return NULL;
> > > +
> > > + if (group->aux_domain_attached)
> > > + domain = group->domain;
> >
> > Why wouldn't the aux domain flag be on the domain itself rather than
> > the group? Then if we wanted sanity checking in patch 1/ we'd only
> > need to test the flag on the object we're provided.
> >
> > If we had such a flag, we could create an iommu_domain_is_aux()
> > function and then simply use iommu_get_domain_for_dev() and test that
> > it's an aux domain in the example use case. It seems like that would
>
> IOMMU layer manages domains per parent device. Here given a
Is this the IOMMU layer or the VT-d driver? I don't see any notion of
managing domains relative to a parent in the IOMMU layer. Please point
to something more specific if I'm wrong here.
> dev (of mdev), we need a way to find its associated domain under its
> parent device. And we cannot simply use iommu_get_domain_for_dev
> on the parent device of the mdev, as it will give us the primary domain
> of parent device.
Not the parent device of the mdev, but the mdev_dev(mdev) device.
Isn't that what this series is enabling, being able to return the
domain from the group that contains the mdev_dev? We shouldn't need to
leave breadcrumbs on the group to know about the domain, the domain
itself should be the source of knowledge, or provide a mechanism/ops to
learn that knowledge. Thanks,
Alex
> > resolve the jump from a domain to an aux-domain just as well as adding
> > this separate iommu_aux_get_domain_for_dev() interface. The is_aux
> > test might also be useful in other cases too. Thanks,
> >
> > Alex
> >
> > > +
> > > + iommu_group_put(group);
> > > +
> > > + return domain;
> > > +}
> > > +EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
> > > +
> > > /**
> > > * iommu_sva_bind_device() - Bind a process address space to a device
> > > * @dev: the device
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index 9506551139ab..cda6cef7579e 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -639,6 +639,7 @@ int iommu_aux_attach_group(struct
> > iommu_domain *domain,
> > > struct iommu_group *group, struct device *dev);
> > > void iommu_aux_detach_group(struct iommu_domain *domain,
> > > struct iommu_group *group, struct device *dev);
> > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct device
> > *dev);
> > >
> > > struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> > > struct mm_struct *mm,
> > > @@ -1040,6 +1041,12 @@ iommu_aux_detach_group(struct
> > iommu_domain *domain,
> > > {
> > > }
> > >
> > > +static inline struct iommu_domain *
> > > +iommu_aux_get_domain_for_dev(struct device *dev)
> > > +{
> > > + return NULL;
> > > +}
> > > +
> > > static inline struct iommu_sva *
> > > iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void
> > *drvdata)
> > > {
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
On Wed, 29 Jul 2020 23:34:40 +
"Tian, Kevin" wrote:
> > From: Alex Williamson
> > Sent: Thursday, July 30, 2020 4:04 AM
> >
> > On Thu, 16 Jul 2020 09:07:46 +0800
> > Lu Baolu wrote:
> >
> > > Hi Jacob,
> > >
> > > On 7/16/20 12:01 AM, Jacob Pan wrote:
> > > > On Wed, 15 Jul 2020 08:47:36 +0800
> > > > Lu Baolu wrote:
> > > >
> > > >> Hi Jacob,
> > > >>
> > > >> On 7/15/20 12:39 AM, Jacob Pan wrote:
> > > >>> On Tue, 14 Jul 2020 13:57:01 +0800
> > > >>> Lu Baolu wrote:
> > > >>>
> > > This adds two new aux-domain APIs for a use case like vfio/mdev
> > > where sub-devices derived from an aux-domain capable device are
> > > created and put in an iommu_group.
> > >
> > > /**
> > > * iommu_aux_attach_group - attach an aux-domain to an
> > iommu_group
> > > which
> > > * contains sub-devices (for example
> > > mdevs) derived
> > > * from @dev.
> > > * @domain: an aux-domain;
> > > * @group: an iommu_group which contains sub-devices derived
> > from
> > > @dev;
> > > * @dev:the physical device which supports
> > IOMMU_DEV_FEAT_AUX.
> > > *
> > > * Returns 0 on success, or an error value.
> > > */
> > > int iommu_aux_attach_group(struct iommu_domain *domain,
> > > struct iommu_group *group,
> > > struct device *dev)
> > >
> > > /**
> > > * iommu_aux_detach_group - detach an aux-domain from an
> > > iommu_group *
> > > * @domain: an aux-domain;
> > > * @group: an iommu_group which contains sub-devices derived
> > from
> > > @dev;
> > > * @dev:the physical device which supports
> > IOMMU_DEV_FEAT_AUX.
> > > *
> > > * @domain must have been attached to @group via
> > > iommu_aux_attach_group(). */
> > > void iommu_aux_detach_group(struct iommu_domain *domain,
> > > struct iommu_group *group,
> > > struct device *dev)
> > >
> > > It also adds a flag in the iommu_group data structure to identify
> > > an iommu_group with aux-domain attached from those normal ones.
> > >
> > > Signed-off-by: Lu Baolu
> > > ---
> > > drivers/iommu/iommu.c | 58
> > > +++
> > include/linux/iommu.h |
> > > 17 + 2 files changed, 75 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index e1fdd3531d65..cad5a19ebf22 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -45,6 +45,7 @@ struct iommu_group {
> > > struct iommu_domain *default_domain;
> > > struct iommu_domain *domain;
> > > struct list_head entry;
> > > +unsigned int aux_domain_attached:1;
> > > };
> > >
> > > struct group_device {
> > > @@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct
> > iommu_domain
> > > *domain, struct device *dev) }
> > > EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
> > >
> > > +/**
> > > + * iommu_aux_attach_group - attach an aux-domain to an
> > iommu_group
> > > which
> > > + * contains sub-devices (for example
> > > mdevs) derived
> > > + * from @dev.
> > > + * @domain: an aux-domain;
> > > + * @group: an iommu_group which contains sub-devices derived
> > from
> > > @dev;
> > > + * @dev:the physical device which supports
> > IOMMU_DEV_FEAT_AUX.
> > > + *
> > > + * Returns 0 on success, or an error value.
> > > + */
> > > +int iommu_aux_attach_group(struct iommu_domain *domain,
> > > + struct iommu_group *group, struct
> > > device *dev) +{
> > > +int ret = -EBUSY;
> > > +
> > > +mutex_lock(&group->mutex);
> > > +if (group->domain)
> > > +goto out_unlock;
> > > +
> > > >>> Perhaps I missed something but are we assuming only one mdev per
> > > >>> mdev group? That seems to change the logic where vfio does:
> > > >>> iommu_group_for_each_dev()
> > > >>> iommu_aux_attach_device()
> > > >>>
> > > >>
> > > >> It has been changed in PATCH 4/4:
> > > >>
> > > >> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> > > >> struct vfio_group *group)
> > > >> {
> > > >> if (group->mdev_group)
> > > >> return iommu_aux_attach_group(domain->domain,
> > > >> group->iommu_group,
> > > >> group->iommu_device);
> > > >> else
> > > >>
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Thu, 2020-07-30 at 13:25 -0400, Jim Quinlan wrote:
> On Thu, Jul 30, 2020 at 1:05 PM Nicolas Saenz Julienne
> wrote:
> > Hi Jim,
> >
> > On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote:
> > > static void __init of_unittest_pci_dma_ranges(void)
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > > b/drivers/pci/controller/pcie-brcmstb.c
> > > index bfc2542d54a8..8dacb9d3b7b6 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1197,6 +1197,7 @@ static int brcm_pcie_probe(struct platform_device
> > > *pdev)
> > > ret = brcm_phy_start(pcie);
> > > if (ret) {
> > > dev_err(pcie->dev, "failed to start phy\n");
> > > + reset_control_assert(pcie->rescal);
> > > return ret;
> > > }
> >
> > I think this sneaked in from another patch.
> Thanks Nicolas. BTW, at some point will you be able to test my
> patchset on the RP4 to see if I broke anything?
Yes of course, I'll have a go at it tomorrow.
Regards,
Nicolas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Thu, Jul 30, 2020 at 1:05 PM Nicolas Saenz Julienne
wrote:
>
> Hi Jim,
>
> On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote:
> > static void __init of_unittest_pci_dma_ranges(void)
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c
> > b/drivers/pci/controller/pcie-brcmstb.c
> > index bfc2542d54a8..8dacb9d3b7b6 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1197,6 +1197,7 @@ static int brcm_pcie_probe(struct platform_device
> > *pdev)
> > ret = brcm_phy_start(pcie);
> > if (ret) {
> > dev_err(pcie->dev, "failed to start phy\n");
> > + reset_control_assert(pcie->rescal);
> > return ret;
> > }
>
> I think this sneaked in from another patch.
Thanks Nicolas. BTW, at some point will you be able to test my
patchset on the RP4 to see if I broke anything?
Thanks again,
Jim
>
> Regards,
> Nicolas
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
Hi Jim,
On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote:
> static void __init of_unittest_pci_dma_ranges(void)
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index bfc2542d54a8..8dacb9d3b7b6 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1197,6 +1197,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> ret = brcm_phy_start(pcie);
> if (ret) {
> dev_err(pcie->dev, "failed to start phy\n");
> + reset_control_assert(pcie->rescal);
> return ret;
> }
I think this sneaked in from another patch.
Regards,
Nicolas
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Thu, 2020-07-30 at 12:44 -0400, Jim Quinlan wrote: > On Wed, Jul 29, 2020 at 10:28 AM Rob Herring wrote: > > On Wed, Jul 29, 2020 at 12:19 AM Christoph Hellwig wrote: > > > On Tue, Jul 28, 2020 at 02:24:51PM -0400, Jim Quinlan wrote: > > > > I started using devm_kcalloc() but at least two reviewers convinced me > > > > to just use kcalloc(). In addition, when I was using devm_kcalloc() > > > > it was awkward because 'dev' is not available to this function. > > > > > > > > It comes down to whether unbind/binding the device N times is actually > > > > a reasonable usage. As for my experience I've seen two cases: (1) my > > > > overnight "bind/unbind the PCIe RC driver" script, and we have a > > > > customer who does an unbind/bind as a hail mary to bring back life to > > > > their dead EP device. If the latter case happens repeatedly, there > > > > are bigger problems. > > > > > > We can't just leak the allocations. Do you have a pointer to the > > > arguments against managed resources? I'm generally not a huge fan > > > of the managed resources, but for a case like this they actually seem > > > useful. If we don't use the managed resources we'll at leat need > > > to explicitly free the resources when freeing the device. > > > > The lifetime for devm_kcalloc may not be what we want here. devm > > allocs are freed on probe fail or remove, not on freeing the device > > (there is a just in case free there too though). > > What do you suggest doing as an alternative? I haven't given the idea much thought, but how about using a kref in the first element of your bus_dma_region array. It should be increased upon assigning it to a device and decreased it upon destroying it (triggering the free once it hits 0). It would take care of this memory leak, but also useful where you're sharing bus_dma_regions between devices. Regards, Nicolas ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 08/12] device core: Introduce DMA range map, supplanting dma_pfn_offset
On Wed, Jul 29, 2020 at 10:28 AM Rob Herring wrote: > > On Wed, Jul 29, 2020 at 12:19 AM Christoph Hellwig wrote: > > > > On Tue, Jul 28, 2020 at 02:24:51PM -0400, Jim Quinlan wrote: > > > I started using devm_kcalloc() but at least two reviewers convinced me > > > to just use kcalloc(). In addition, when I was using devm_kcalloc() > > > it was awkward because 'dev' is not available to this function. > > > > > > It comes down to whether unbind/binding the device N times is actually > > > a reasonable usage. As for my experience I've seen two cases: (1) my > > > overnight "bind/unbind the PCIe RC driver" script, and we have a > > > customer who does an unbind/bind as a hail mary to bring back life to > > > their dead EP device. If the latter case happens repeatedly, there > > > are bigger problems. > > > > We can't just leak the allocations. Do you have a pointer to the > > arguments against managed resources? I'm generally not a huge fan > > of the managed resources, but for a case like this they actually seem > > useful. If we don't use the managed resources we'll at leat need > > to explicitly free the resources when freeing the device. > > The lifetime for devm_kcalloc may not be what we want here. devm > allocs are freed on probe fail or remove, not on freeing the device > (there is a just in case free there too though). What do you suggest doing as an alternative? Jim > > > Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 06/15] powerpc: fadamp: simplify fadump_reserve_crash_area()
Mike Rapoport writes:
> From: Mike Rapoport
>
> fadump_reserve_crash_area() reserves memory from a specified base address
> till the end of the RAM.
>
> Replace iteration through the memblock.memory with a single call to
> memblock_reserve() with appropriate that will take care of proper memory
^
parameters?
> reservation.
>
> Signed-off-by: Mike Rapoport
> ---
> arch/powerpc/kernel/fadump.c | 20 +---
> 1 file changed, 1 insertion(+), 19 deletions(-)
I think this looks OK to me, but I don't have a setup to test it easily.
I've added Hari to Cc who might be able to.
But I'll give you an ack in the hope that it works :)
Acked-by: Michael Ellerman
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 78ab9a6ee6ac..2446a61e3c25 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1658,25 +1658,7 @@ int __init fadump_reserve_mem(void)
> /* Preserve everything above the base address */
> static void __init fadump_reserve_crash_area(u64 base)
> {
> - struct memblock_region *reg;
> - u64 mstart, msize;
> -
> - for_each_memblock(memory, reg) {
> - mstart = reg->base;
> - msize = reg->size;
> -
> - if ((mstart + msize) < base)
> - continue;
> -
> - if (mstart < base) {
> - msize -= (base - mstart);
> - mstart = base;
> - }
> -
> - pr_info("Reserving %lluMB of memory at %#016llx for preserving
> crash data",
> - (msize >> 20), mstart);
> - memblock_reserve(mstart, msize);
> - }
> + memblock_reserve(base, memblock_end_of_DRAM() - base);
> }
>
> unsigned long __init arch_reserved_kernel_pages(void)
> --
> 2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 04/15] arm64: numa: simplify dummy_numa_init()
On Tue, Jul 28, 2020 at 08:11:42AM +0300, Mike Rapoport wrote: > From: Mike Rapoport > > dummy_numa_init() loops over memblock.memory and passes nid=0 to > numa_add_memblk() which essentially wraps memblock_set_node(). However, > memblock_set_node() can cope with entire memory span itself, so the loop > over memblock.memory regions is redundant. > > Replace the loop with a single call to memblock_set_node() to the entire > memory. > > Signed-off-by: Mike Rapoport Acked-by: Catalin Marinas ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
> From: Lu Baolu
> Sent: Tuesday, July 14, 2020 1:57 PM
>
> Replace iommu_aux_at(de)tach_device() with iommu_aux_at(de)tach_group().
> It also saves the IOMMU_DEV_FEAT_AUX-capable physcail device in the vfio_group
> data structure so that it could be reused in other places.
>
> Signed-off-by: Lu Baolu
> ---
> drivers/vfio/vfio_iommu_type1.c | 44 ++---
> 1 file changed, 7 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 5e556ac9102a..f8812e68de77 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -100,6 +100,7 @@ struct vfio_dma {
> struct vfio_group {
> struct iommu_group *iommu_group;
> struct list_headnext;
> + struct device *iommu_device;
I know mdev group has only one device, so such a group has a single
iommu_device. But I guess may be helpful to add a comment here or in
commit message. Otherwise, it looks weird that a group structure
contains a single iommu_device field instead of a list of iommu_device.
Regards,
Yi Liu
> boolmdev_group; /* An mdev group */
> boolpinned_page_dirty_scope;
> };
> @@ -1627,45 +1628,13 @@ static struct device
> *vfio_mdev_get_iommu_device(struct device *dev)
> return NULL;
> }
>
> -static int vfio_mdev_attach_domain(struct device *dev, void *data) -{
> - struct iommu_domain *domain = data;
> - struct device *iommu_device;
> -
> - iommu_device = vfio_mdev_get_iommu_device(dev);
> - if (iommu_device) {
> - if (iommu_dev_feature_enabled(iommu_device,
> IOMMU_DEV_FEAT_AUX))
> - return iommu_aux_attach_device(domain, iommu_device);
> - else
> - return iommu_attach_device(domain, iommu_device);
> - }
> -
> - return -EINVAL;
> -}
> -
> -static int vfio_mdev_detach_domain(struct device *dev, void *data) -{
> - struct iommu_domain *domain = data;
> - struct device *iommu_device;
> -
> - iommu_device = vfio_mdev_get_iommu_device(dev);
> - if (iommu_device) {
> - if (iommu_dev_feature_enabled(iommu_device,
> IOMMU_DEV_FEAT_AUX))
> - iommu_aux_detach_device(domain, iommu_device);
> - else
> - iommu_detach_device(domain, iommu_device);
> - }
> -
> - return 0;
> -}
> -
> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> struct vfio_group *group)
> {
> if (group->mdev_group)
> - return iommu_group_for_each_dev(group->iommu_group,
> - domain->domain,
> - vfio_mdev_attach_domain);
> + return iommu_aux_attach_group(domain->domain,
> + group->iommu_group,
> + group->iommu_device);
> else
> return iommu_attach_group(domain->domain, group-
> >iommu_group); } @@ -1674,8 +1643,8 @@ static void
> vfio_iommu_detach_group(struct vfio_domain *domain,
> struct vfio_group *group)
> {
> if (group->mdev_group)
> - iommu_group_for_each_dev(group->iommu_group, domain-
> >domain,
> - vfio_mdev_detach_domain);
> + iommu_aux_detach_group(domain->domain, group->iommu_group,
> +group->iommu_device);
> else
> iommu_detach_group(domain->domain, group->iommu_group); }
> @@ -2007,6 +1976,7 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> return 0;
> }
>
> + group->iommu_device = iommu_device;
> bus = iommu_device->bus;
> }
>
> --
> 2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: amd: Fix IO_PAGE_FAULT due to __unmap_single() size overflow
On Wed, Jun 24, 2020 at 01:58:27PM +0200, Joerg Roedel wrote: > Hi Greg, > > On Wed, Jun 24, 2020 at 11:09:06AM +0200, Greg KH wrote: > > So what exact stable kernel version(s) do you want to see this patch > > applied to? > > It is needed in kernels <= v5.4. Linux v5.5 has replaced the code with > the bug. This doesn't apply to any older kernels that I can see :( ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
