Re: [PATCH v2 2/2] iommu/iova: Free global iova rcache on iova alloc failure
On 9/28/2020 6:11 PM, Vijayanand Jitta wrote:
>
>
> On 9/18/2020 8:11 PM, Robin Murphy wrote:
>> On 2020-08-20 13:49, vji...@codeaurora.org wrote:
>>> From: Vijayanand Jitta
>>>
>>> When ever an iova alloc request fails we free the iova
>>> ranges present in the percpu iova rcaches and then retry
>>> but the global iova rcache is not freed as a result we could
>>> still see iova alloc failure even after retry as global
>>> rcache is holding the iova's which can cause fragmentation.
>>> So, free the global iova rcache as well and then go for the
>>> retry.
>>>
>>> Signed-off-by: Vijayanand Jitta
>>> ---
>>> drivers/iommu/iova.c | 23 +++
>>> include/linux/iova.h | 6 ++
>>> 2 files changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index 4e77116..5836c87 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -442,6 +442,7 @@ struct iova *find_iova(struct iova_domain *iovad,
>>> unsigned long pfn)
>>> flush_rcache = false;
>>> for_each_online_cpu(cpu)
>>> free_cpu_cached_iovas(cpu, iovad);
>>> + free_global_cached_iovas(iovad);
>>> goto retry;
>>> }
>>> @@ -1055,5 +1056,27 @@ void free_cpu_cached_iovas(unsigned int cpu,
>>> struct iova_domain *iovad)
>>> }
>>> }
>>> +/*
>>> + * free all the IOVA ranges of global cache
>>> + */
>>> +void free_global_cached_iovas(struct iova_domain *iovad)
>>
>> As John pointed out last time, this should be static and the header
>> changes dropped.
>>
>> (TBH we should probably register our own hotplug notifier instance for a
>> flush queue, so that external code has no need to poke at the per-CPU
>> caches either)
>>
>> Robin.
>>
>
> Right, I have made it static and dropped header changes in v3.
> can you please review that.
>
> Thanks,
> Vijay
Please review v4 instead of v3, I have updated other patch as well in v4.
Thanks,
Vijay
>>> +{
>>> + struct iova_rcache *rcache;
>>> + unsigned long flags;
>>> + int i, j;
>>> +
>>> + for (i = 0; i < IOVA_RANGE_CACHE_MAX_SIZE; ++i) {
>>> + rcache = >rcaches[i];
>>> + spin_lock_irqsave(>lock, flags);
>>> + for (j = 0; j < rcache->depot_size; ++j) {
>>> + iova_magazine_free_pfns(rcache->depot[j], iovad);
>>> + iova_magazine_free(rcache->depot[j]);
>>> + rcache->depot[j] = NULL;
>>> + }
>>> + rcache->depot_size = 0;
>>> + spin_unlock_irqrestore(>lock, flags);
>>> + }
>>> +}
>>> +
>>> MODULE_AUTHOR("Anil S Keshavamurthy ");
>>> MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>>> index a0637ab..a905726 100644
>>> --- a/include/linux/iova.h
>>> +++ b/include/linux/iova.h
>>> @@ -163,6 +163,7 @@ int init_iova_flush_queue(struct iova_domain *iovad,
>>> struct iova *split_and_remove_iova(struct iova_domain *iovad,
>>> struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
>>> void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain
>>> *iovad);
>>> +void free_global_cached_iovas(struct iova_domain *iovad);
>>> #else
>>> static inline int iova_cache_get(void)
>>> {
>>> @@ -270,6 +271,11 @@ static inline void free_cpu_cached_iovas(unsigned
>>> int cpu,
>>> struct iova_domain *iovad)
>>> {
>>> }
>>> +
>>> +static inline void free_global_cached_iovas(struct iova_domain *iovad)
>>> +{
>>> +}
>>> +
>>> #endif
>>> #endif
>>>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:41, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>>> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>>> struct device *dev)
>>> {
>>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>> struct tegra_smmu_as *as = to_smmu_as(domain);
>>> - struct device_node *np = dev->of_node;
>>> - struct of_phandle_args args;
>>> unsigned int index = 0;
>>> int err = 0;
>>>
>>> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - )) {
>>> - unsigned int swgroup = args.args[0];
>>> -
>>> - if (args.np != smmu->dev->of_node) {
>>> - of_node_put(args.np);
>>> - continue;
>>> - }
>>> -
>>> - of_node_put(args.np);
>>> + if (!fwspec || fwspec->ops != _smmu_ops)
>>> + return -ENOENT;
>>
>> s/_smmu_ops/smmu->iommu.ops/
>>
>> Secondly, is it even possible that fwspec could be NULL here or that
>> fwspec->ops != smmu->ops?
>
> I am following what's in the arm-smmu driver, as I think it'd be
> a common practice to do such a check in such a way.
>
Please check whether it's really needed. It looks like it was needed
sometime ago, but that's not true anymore.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
30.09.2020 08:29, Nicolin Chen пишет: > Hi Dmitry, > > On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote: >> 30.09.2020 03:30, Nicolin Chen пишет: >>> - group->group = iommu_group_alloc(); >>> + group->group = pci ? pci_device_group(dev) : iommu_group_alloc(); >> >> This will be nicer to write as: >> >> if (dev_is_pci(dev)) >> group->group = pci_device_group(dev); >> else >> group->group = generic_device_group(dev); > > Why is that nicer? I don't feel mine is hard to read at all... > Previously you said that you're going to add USB support, so I assume there will be something about USB. It's also a kinda common style that majority of Tegra drivers use in upstream kernel. But yours variant is good too if there won't be a need to change it later on. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:20:50AM +0300, Dmitry Osipenko wrote: > 30.09.2020 08:10, Dmitry Osipenko пишет: > > 30.09.2020 03:30, Nicolin Chen пишет: > >> static void tegra_smmu_release_device(struct device *dev) > > > > The tegra_get_memory_controller() uses of_find_device_by_node(), hence > > tegra_smmu_release_device() should put_device(mc) in order to balance > > back the refcounting. > > > > Actually, the put_device(mc) should be right after > tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU > is a part of MC, hence MC can't just go away. Will add it. Thanks for pointing it out! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:11:52AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > > + /* An invalid mc pointer means mc and smmu drivers are not ready */ > > + if (IS_ERR_OR_NULL(mc)) > > tegra_get_memory_controller() doesn't return NULL. Well, I don't want to assume that it'd do that forever, and the NULL check of IS_ERR_OR_NULL is marked "unlikely" so it doesn't hurt to have. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller
On Wed, Sep 30, 2020 at 08:12:10AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > ... > > int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long > > rate); > > unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc); > > +struct tegra_mc *tegra_get_memory_controller(void); > > > > #endif /* __SOC_TEGRA_MC_H__ */ > > > > This will conflict with the tegra20-devfreq driver, you should change it > as well. Will remove that in v3. Thanks ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:39, Nicolin Chen пишет: > On Wed, Sep 30, 2020 at 08:24:02AM +0300, Dmitry Osipenko wrote: >> 30.09.2020 03:30, Nicolin Chen пишет: >>> + /* >>> +* IOMMU core allows -ENODEV return to carry on. So bypass any call >>> +* from bus_set_iommu() during tegra_smmu_probe(), as a device will >>> +* call in again via of_iommu_configure when fwspec is prepared. >>> +*/ >>> + if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops) >>> return ERR_PTR(-ENODEV); >> >> The !mc->smmu can't be true. > > Are you sure? I have removed the "mc->smmu = smmu" in probe() with > this change. So the only time "mc->smmu == !NULL" is after probe() > of SMMU driver is returned. As my comments says, tegra_smmu_probe() > calls in this function via bus_set_iommu(), so mc->smmu can be NULL > in this case. > I missed that. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/2] iommu/iova: Retry from last rb tree node if iova search fails
From: Vijayanand Jitta
When ever a new iova alloc request comes iova is always searched
from the cached node and the nodes which are previous to cached
node. So, even if there is free iova space available in the nodes
which are next to the cached node iova allocation can still fail
because of this approach.
Consider the following sequence of iova alloc and frees on
1GB of iova space
1) alloc - 500MB
2) alloc - 12MB
3) alloc - 499MB
4) free - 12MB which was allocated in step 2
5) alloc - 13MB
After the above sequence we will have 12MB of free iova space and
cached node will be pointing to the iova pfn of last alloc of 13MB
which will be the lowest iova pfn of that iova space. Now if we get an
alloc request of 2MB we just search from cached node and then look
for lower iova pfn's for free iova and as they aren't any, iova alloc
fails though there is 12MB of free iova space.
To avoid such iova search failures do a retry from the last rb tree node
when iova search fails, this will search the entire tree and get an iova
if its available.
Signed-off-by: Vijayanand Jitta
Reviewed-by: Robin Murphy
---
drivers/iommu/iova.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 30d969a..c3a1a8e 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -184,8 +184,9 @@ static int __alloc_and_insert_iova_range(struct iova_domain
*iovad,
struct rb_node *curr, *prev;
struct iova *curr_iova;
unsigned long flags;
- unsigned long new_pfn;
+ unsigned long new_pfn, retry_pfn;
unsigned long align_mask = ~0UL;
+ unsigned long high_pfn = limit_pfn, low_pfn = iovad->start_pfn;
if (size_aligned)
align_mask <<= fls_long(size - 1);
@@ -198,15 +199,25 @@ static int __alloc_and_insert_iova_range(struct
iova_domain *iovad,
curr = __get_cached_rbnode(iovad, limit_pfn);
curr_iova = rb_entry(curr, struct iova, node);
+ retry_pfn = curr_iova->pfn_hi + 1;
+
+retry:
do {
- limit_pfn = min(limit_pfn, curr_iova->pfn_lo);
- new_pfn = (limit_pfn - size) & align_mask;
+ high_pfn = min(high_pfn, curr_iova->pfn_lo);
+ new_pfn = (high_pfn - size) & align_mask;
prev = curr;
curr = rb_prev(curr);
curr_iova = rb_entry(curr, struct iova, node);
- } while (curr && new_pfn <= curr_iova->pfn_hi);
-
- if (limit_pfn < size || new_pfn < iovad->start_pfn) {
+ } while (curr && new_pfn <= curr_iova->pfn_hi && new_pfn >= low_pfn);
+
+ if (high_pfn < size || new_pfn < low_pfn) {
+ if (low_pfn == iovad->start_pfn && retry_pfn < limit_pfn) {
+ high_pfn = limit_pfn;
+ low_pfn = retry_pfn;
+ curr = >anchor.node;
+ curr_iova = rb_entry(curr, struct iova, node);
+ goto retry;
+ }
iovad->max32_alloc_size = size;
goto iova32_full;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/2] iommu/iova: Retry from last rb tree node if iova search fails
On 9/18/2020 7:48 PM, Robin Murphy wrote:
> On 2020-08-20 13:49, vji...@codeaurora.org wrote:
>> From: Vijayanand Jitta
>>
>> When ever a new iova alloc request comes iova is always searched
>> from the cached node and the nodes which are previous to cached
>> node. So, even if there is free iova space available in the nodes
>> which are next to the cached node iova allocation can still fail
>> because of this approach.
>>
>> Consider the following sequence of iova alloc and frees on
>> 1GB of iova space
>>
>> 1) alloc - 500MB
>> 2) alloc - 12MB
>> 3) alloc - 499MB
>> 4) free - 12MB which was allocated in step 2
>> 5) alloc - 13MB
>>
>> After the above sequence we will have 12MB of free iova space and
>> cached node will be pointing to the iova pfn of last alloc of 13MB
>> which will be the lowest iova pfn of that iova space. Now if we get an
>> alloc request of 2MB we just search from cached node and then look
>> for lower iova pfn's for free iova and as they aren't any, iova alloc
>> fails though there is 12MB of free iova space.
>>
>> To avoid such iova search failures do a retry from the last rb tree node
>> when iova search fails, this will search the entire tree and get an iova
>> if its available.
>>
>> Signed-off-by: Vijayanand Jitta
>> ---
>> drivers/iommu/iova.c | 23 +--
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 49fc01f..4e77116 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -184,8 +184,9 @@ static int __alloc_and_insert_iova_range(struct
>> iova_domain *iovad,
>> struct rb_node *curr, *prev;
>> struct iova *curr_iova;
>> unsigned long flags;
>> - unsigned long new_pfn;
>> + unsigned long new_pfn, low_pfn_new;
>> unsigned long align_mask = ~0UL;
>> + unsigned long high_pfn = limit_pfn, low_pfn = iovad->start_pfn;
>> if (size_aligned)
>> align_mask <<= fls_long(size - 1);
>> @@ -198,15 +199,25 @@ static int __alloc_and_insert_iova_range(struct
>> iova_domain *iovad,
>> curr = __get_cached_rbnode(iovad, limit_pfn);
>> curr_iova = rb_entry(curr, struct iova, node);
>> + low_pfn_new = curr_iova->pfn_hi + 1;
>
> Could we call "low_pfn_new" something like "retry_pfn" instead? This
> code already has unavoidable readability struggles with so many
> different "pfn"s in play, so having two different meanings of "new"
> really doesn't help.
>
> Other than that, I think this looks OK (IIRC it's basically what I
> originally suggested), so with the naming tweaked,
>
> Reviewed-by: Robin Murphy
>
Thanks for review, I have renamed it to retry_pfn in v4.
Thanks,
Vijay
>> +
>> +retry:
>> do {
>> - limit_pfn = min(limit_pfn, curr_iova->pfn_lo);
>> - new_pfn = (limit_pfn - size) & align_mask;
>> + high_pfn = min(high_pfn, curr_iova->pfn_lo);
>> + new_pfn = (high_pfn - size) & align_mask;
>> prev = curr;
>> curr = rb_prev(curr);
>> curr_iova = rb_entry(curr, struct iova, node);
>> - } while (curr && new_pfn <= curr_iova->pfn_hi);
>> -
>> - if (limit_pfn < size || new_pfn < iovad->start_pfn) {
>> + } while (curr && new_pfn <= curr_iova->pfn_hi && new_pfn >=
>> low_pfn);
>> +
>> + if (high_pfn < size || new_pfn < low_pfn) {
>> + if (low_pfn == iovad->start_pfn && low_pfn_new < limit_pfn) {
>> + high_pfn = limit_pfn;
>> + low_pfn = low_pfn_new;
>> + curr = >anchor.node;
>> + curr_iova = rb_entry(curr, struct iova, node);
>> + goto retry;
>> + }
>> iovad->max32_alloc_size = size;
>> goto iova32_full;
>> }
>>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/2] iommu/iova: Free global iova rcache on iova alloc failure
From: Vijayanand Jitta
When ever an iova alloc request fails we free the iova
ranges present in the percpu iova rcaches and then retry
but the global iova rcache is not freed as a result we could
still see iova alloc failure even after retry as global
rcache is holding the iova's which can cause fragmentation.
So, free the global iova rcache as well and then go for the
retry.
Signed-off-by: Vijayanand Jitta
---
drivers/iommu/iova.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index c3a1a8e..64ce082 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -442,6 +442,7 @@ alloc_iova_fast(struct iova_domain *iovad, unsigned long
size,
flush_rcache = false;
for_each_online_cpu(cpu)
free_cpu_cached_iovas(cpu, iovad);
+ free_global_cached_iovas(iovad);
goto retry;
}
@@ -1057,5 +1058,26 @@ void free_cpu_cached_iovas(unsigned int cpu, struct
iova_domain *iovad)
}
}
+/*
+ * free all the IOVA ranges of global cache
+ */
+static void free_global_cached_iovas(struct iova_domain *iovad)
+{
+ struct iova_rcache *rcache;
+ unsigned long flags;
+ int i, j;
+
+ for (i = 0; i < IOVA_RANGE_CACHE_MAX_SIZE; ++i) {
+ rcache = >rcaches[i];
+ spin_lock_irqsave(>lock, flags);
+ for (j = 0; j < rcache->depot_size; ++j) {
+ iova_magazine_free_pfns(rcache->depot[j], iovad);
+ iova_magazine_free(rcache->depot[j]);
+ rcache->depot[j] = NULL;
+ }
+ rcache->depot_size = 0;
+ spin_unlock_irqrestore(>lock, flags);
+ }
+}
MODULE_AUTHOR("Anil S Keshavamurthy ");
MODULE_LICENSE("GPL");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:39:54AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > static int tegra_smmu_attach_dev(struct iommu_domain *domain,
> > struct device *dev)
> > {
> > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> > struct tegra_smmu_as *as = to_smmu_as(domain);
> > - struct device_node *np = dev->of_node;
> > - struct of_phandle_args args;
> > unsigned int index = 0;
> > int err = 0;
> >
> > - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > - )) {
> > - unsigned int swgroup = args.args[0];
> > -
> > - if (args.np != smmu->dev->of_node) {
> > - of_node_put(args.np);
> > - continue;
> > - }
> > -
> > - of_node_put(args.np);
> > + if (!fwspec || fwspec->ops != _smmu_ops)
> > + return -ENOENT;
>
> s/_smmu_ops/smmu->iommu.ops/
>
> Secondly, is it even possible that fwspec could be NULL here or that
> fwspec->ops != smmu->ops?
I am following what's in the arm-smmu driver, as I think it'd be
a common practice to do such a check in such a way.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
On Wed, Sep 30, 2020 at 08:24:02AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > > + /* > > +* IOMMU core allows -ENODEV return to carry on. So bypass any call > > +* from bus_set_iommu() during tegra_smmu_probe(), as a device will > > +* call in again via of_iommu_configure when fwspec is prepared. > > +*/ > > + if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops) > > return ERR_PTR(-ENODEV); > > The !mc->smmu can't be true. Are you sure? I have removed the "mc->smmu = smmu" in probe() with this change. So the only time "mc->smmu == !NULL" is after probe() of SMMU driver is returned. As my comments says, tegra_smmu_probe() calls in this function via bus_set_iommu(), so mc->smmu can be NULL in this case. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет:
> static int tegra_smmu_attach_dev(struct iommu_domain *domain,
>struct device *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> struct tegra_smmu_as *as = to_smmu_as(domain);
> - struct device_node *np = dev->of_node;
> - struct of_phandle_args args;
> unsigned int index = 0;
> int err = 0;
>
> - while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> -)) {
> - unsigned int swgroup = args.args[0];
> -
> - if (args.np != smmu->dev->of_node) {
> - of_node_put(args.np);
> - continue;
> - }
> -
> - of_node_put(args.np);
> + if (!fwspec || fwspec->ops != _smmu_ops)
> + return -ENOENT;
s/_smmu_ops/smmu->iommu.ops/
Secondly, is it even possible that fwspec could be NULL here or that
fwspec->ops != smmu->ops?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > void tegra_smmu_remove(struct tegra_smmu *smmu)
> > {
> > + bus_set_iommu(_bus_type, NULL);
>
> Why only platform_bus? Is this really needed at all?
I see qcom_iommu.c file set to NULL in remove(), Probably should
have added pci_bus_type too though.
Or are you sure that there's no need at all?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
Hi Dmitry, On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote: > 30.09.2020 03:30, Nicolin Chen пишет: > > - group->group = iommu_group_alloc(); > > + group->group = pci ? pci_device_group(dev) : iommu_group_alloc(); > > This will be nicer to write as: > > if (dev_is_pci(dev)) > group->group = pci_device_group(dev); > else > group->group = generic_device_group(dev); Why is that nicer? I don't feel mine is hard to read at all... ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
On Wed, Sep 30, 2020 at 08:10:00AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> ...
> > +#ifdef CONFIG_PCI
> > + if (!iommu_present(_bus_type)) {
>
> Could you please explain why this check is needed?
That's referencing what's in the arm-smmu.c file, since it does
not hurt to have a check.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > + /* > + * IOMMU core allows -ENODEV return to carry on. So bypass any call > + * from bus_set_iommu() during tegra_smmu_probe(), as a device will > + * call in again via of_iommu_configure when fwspec is prepared. > + */ > + if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops) > return ERR_PTR(-ENODEV); The !mc->smmu can't be true. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 08:10, Dmitry Osipenko пишет: > 30.09.2020 03:30, Nicolin Chen пишет: >> static void tegra_smmu_release_device(struct device *dev) > > The tegra_get_memory_controller() uses of_find_device_by_node(), hence > tegra_smmu_release_device() should put_device(mc) in order to balance > back the refcounting. > Actually, the put_device(mc) should be right after tegra_get_memory_controller() in tegra_smmu_probe_device() because SMMU is a part of MC, hence MC can't just go away. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller
30.09.2020 03:30, Nicolin Chen пишет: ... > int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long > rate); > unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc); > +struct tegra_mc *tegra_get_memory_controller(void); > > #endif /* __SOC_TEGRA_MC_H__ */ > This will conflict with the tegra20-devfreq driver, you should change it as well. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > + /* An invalid mc pointer means mc and smmu drivers are not ready */ > + if (IS_ERR_OR_NULL(mc)) tegra_get_memory_controller() doesn't return NULL. > + return ERR_PTR(-EPROBE_DEFER); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
30.09.2020 03:30, Nicolin Chen пишет:
> void tegra_smmu_remove(struct tegra_smmu *smmu)
> {
> + bus_set_iommu(_bus_type, NULL);
Why only platform_bus? Is this really needed at all?
> iommu_device_unregister(>iommu);
> iommu_device_sysfs_remove(>iommu);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
30.09.2020 03:30, Nicolin Chen пишет: > static void tegra_smmu_release_device(struct device *dev) The tegra_get_memory_controller() uses of_find_device_by_node(), hence tegra_smmu_release_device() should put_device(mc) in order to balance back the refcounting. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
30.09.2020 03:30, Nicolin Chen пишет:
...
> +#ifdef CONFIG_PCI
> + if (!iommu_present(_bus_type)) {
Could you please explain why this check is needed?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
30.09.2020 03:30, Nicolin Chen пишет: > - group->group = iommu_group_alloc(); > + group->group = pci ? pci_device_group(dev) : iommu_group_alloc(); This will be nicer to write as: if (dev_is_pci(dev)) group->group = pci_device_group(dev); else group->group = generic_device_group(dev); ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On Wed, Sep 30, 2020 at 06:19:57AM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 29, 2020 at 07:47:52PM -0400, Daniel P. Smith wrote: > > TrenchBoot's AMD Secure Loader (LZ). The former is not well supported > > and the latter will be getting maintenance under TB. While this is not > > preferred, we had to weigh this versus trying to convince you and the > > other TPM driver maintainers on a significant refactoring of the TPM > > driver. It was elected for the reuse of a clean implementation that can > > be replaced later if/when the TPM driver was refactored. When we > > explained this during the RFC and it was not rejected, therefore we > > carried it forward into this submission. > > What does it anyway mean when you say "RFC was not rejected"? I don't > get the semantics of that sentence. It probably neither was ack'd, > right? I do not really care what happened with the RFC. All I can say > is that in the current state this totally PoC from top to bottom. > > > > How it is now is never going to fly. > > > > We would gladly work with you and the other TPM maintainers on a > > refactoring of the TPM driver to separate core logic into standalone > > files that both the driver and the compressed kernel can share. > > Yes, exactly. You have to refactor out the common parts. This is way too > big patch to spend time on giving any more specific advice. Should be in > way smaller chunks. For (almost) any possible, this is of unacceptable ^ " patch" > size. > > I think that it'd be best first to keep the common files in > drivers/char/tpm and include them your code with relative paths in the > Makefile. At least up until we have clear view what are the common > parts. > > You might also want to refactor drivers/char/tpm/tpm.h and include/linux > TPM headers to move more stuff into include/linux. > > If you are expecting a quick upstreaming process, please do not. This > will take considerable amount of time to get right. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On Tue, Sep 29, 2020 at 07:47:52PM -0400, Daniel P. Smith wrote: > TrenchBoot's AMD Secure Loader (LZ). The former is not well supported > and the latter will be getting maintenance under TB. While this is not > preferred, we had to weigh this versus trying to convince you and the > other TPM driver maintainers on a significant refactoring of the TPM > driver. It was elected for the reuse of a clean implementation that can > be replaced later if/when the TPM driver was refactored. When we > explained this during the RFC and it was not rejected, therefore we > carried it forward into this submission. What does it anyway mean when you say "RFC was not rejected"? I don't get the semantics of that sentence. It probably neither was ack'd, right? I do not really care what happened with the RFC. All I can say is that in the current state this totally PoC from top to bottom. > > How it is now is never going to fly. > > We would gladly work with you and the other TPM maintainers on a > refactoring of the TPM driver to separate core logic into standalone > files that both the driver and the compressed kernel can share. Yes, exactly. You have to refactor out the common parts. This is way too big patch to spend time on giving any more specific advice. Should be in way smaller chunks. For (almost) any possible, this is of unacceptable size. I think that it'd be best first to keep the common files in drivers/char/tpm and include them your code with relative paths in the Makefile. At least up until we have clear view what are the common parts. You might also want to refactor drivers/char/tpm/tpm.h and include/linux TPM headers to move more stuff into include/linux. If you are expecting a quick upstreaming process, please do not. This will take considerable amount of time to get right. /Jarkko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range
On Tue, Sep 29, 2020 at 08:42:52PM +0300, Dmitry Osipenko wrote: > 29.09.2020 09:13, Nicolin Chen пишет: > > This is used to protect potential race condition at use_count. > > since probes of client drivers, calling attach_dev(), may run > > concurrently. > > > > Signed-off-by: Nicolin Chen > > --- > > It's always better not to mix success and error code paths in order to > keep code readable, but not a big deal in the case of this particular > patch since the changed code is quite simple. Please try to avoid doing > this in the future patches. > > Also, please note that in general it's better to use locked/unlocked > versions for the functions like it's already done for > tegra_smmu_map/unmap, this will remove the need to maintain lockings in > the code. The code touched by this patch doesn't have complicated code > paths, so it's good enough to me. > > Reviewed-by: Dmitry Osipenko Okay. Thanks for the review! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/3] iommu/tegra-smmu: Add PCI support
This series is to add PCI support with three changes: PATCH-1 adds a helper function to get mc pointer PATCH-2 adds support for clients that don't exist in DTB PATCH-3 adds PCI support accordingly Changelog v1->v2 * Added PATCH-1 suggested by Dmitry * Reworked PATCH-2 to unify certain code Prvious versions v1: https://lkml.org/lkml/2020/9/26/66 Nicolin Chen (3): memory: tegra: Add helper function tegra_get_memory_controller iommu/tegra-smmu: Rework .probe_device and .attach_dev iommu/tegra-smmu: Add PCI support drivers/iommu/tegra-smmu.c | 177 + drivers/memory/tegra/mc.c | 23 + include/soc/tegra/mc.h | 1 + 3 files changed, 84 insertions(+), 117 deletions(-) -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
Previously the driver relies on bus_set_iommu() in .probe() to call
in .probe_device() function so each client can poll iommus property
in DTB to configure fwspec via tegra_smmu_configure(). According to
the comments in .probe(), this is a bit of a hack. And this doesn't
work for a client that doesn't exist in DTB, PCI device for example.
Actually when a device/client gets probed, the of_iommu_configure()
will call in .probe_device() function again, with a prepared fwspec
from of_iommu_configure() that reads the SWGROUP id in DTB as we do
in tegra-smmu driver.
Additionally, as new helper function tegra_get_memory_controller()
is introduced, there's no need to poll the iommus property in order
to get mc->smmu pointers or SWGROUP id.
This patch reworks .probe_device() and .attach_dev() by doing:
1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
3) Calling tegra_get_memory_controller() in .probe_device() instead
4) Also dropping the hack in .probe() that's no longer needed.
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2
* Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
with tegra_get_memory_controller call.
* Dropped the hack in tegra_smmu_probe().
drivers/iommu/tegra-smmu.c | 144 ++---
1 file changed, 36 insertions(+), 108 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..cf4981369828 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -61,6 +61,8 @@ struct tegra_smmu_as {
u32 attr;
};
+static const struct iommu_ops tegra_smmu_ops;
+
static struct tegra_smmu_as *to_smmu_as(struct iommu_domain *dom)
{
return container_of(dom, struct tegra_smmu_as, domain);
@@ -484,60 +486,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu
*smmu,
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
- struct of_phandle_args args;
unsigned int index = 0;
int err = 0;
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- )) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec || fwspec->ops != _smmu_ops)
+ return -ENOENT;
+ for (index = 0; index < fwspec->num_ids; index++) {
err = tegra_smmu_as_prepare(smmu, as);
- if (err < 0)
- return err;
+ if (err)
+ goto disable;
- tegra_smmu_enable(smmu, swgroup, as->id);
- index++;
+ tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
}
if (index == 0)
return -ENODEV;
return 0;
+
+disable:
+ while (index--) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+ tegra_smmu_as_unprepare(smmu, as);
+ }
+
+ return err;
}
static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device
*dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct tegra_smmu_as *as = to_smmu_as(domain);
- struct device_node *np = dev->of_node;
struct tegra_smmu *smmu = as->smmu;
- struct of_phandle_args args;
unsigned int index = 0;
- while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- )) {
- unsigned int swgroup = args.args[0];
-
- if (args.np != smmu->dev->of_node) {
- of_node_put(args.np);
- continue;
- }
-
- of_node_put(args.np);
+ if (!fwspec || fwspec->ops != _smmu_ops)
+ return;
- tegra_smmu_disable(smmu, swgroup, as->id);
+ for (index = 0; index < fwspec->num_ids; index++) {
+ tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
tegra_smmu_as_unprepare(smmu, as);
- index++;
}
}
@@ -807,80 +799,26 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct
iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
-
- mc =
[PATCH v2 1/3] memory: tegra: Add helper function tegra_get_memory_controller
This can be used by both tegra-smmu and tegra20-devfreq drivers.
Suggested-by: Dmitry Osipenko
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2
* N/A
drivers/memory/tegra/mc.c | 23 +++
include/soc/tegra/mc.h| 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index ec8403557ed4..09352ad66dcc 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -42,6 +42,29 @@ static const struct of_device_id tegra_mc_of_match[] = {
};
MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
+struct tegra_mc *tegra_get_memory_controller(void)
+{
+ struct platform_device *pdev;
+ struct device_node *np;
+ struct tegra_mc *mc;
+
+ np = of_find_matching_node_and_match(NULL, tegra_mc_of_match, NULL);
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ pdev = of_find_device_by_node(np);
+ of_node_put(np);
+ if (!pdev)
+ return ERR_PTR(-ENODEV);
+
+ mc = platform_get_drvdata(pdev);
+ if (!mc)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return mc;
+}
+EXPORT_SYMBOL_GPL(tegra_get_memory_controller);
+
static int tegra_mc_block_dma_common(struct tegra_mc *mc,
const struct tegra_mc_reset *rst)
{
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..c72718fd589f 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -183,5 +183,6 @@ struct tegra_mc {
int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
+struct tegra_mc *tegra_get_memory_controller(void);
#endif /* __SOC_TEGRA_MC_H__ */
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/3] iommu/tegra-smmu: Add PCI support
This patch simply adds support for PCI devices. Also reverses
bus_set_iommu in tegra_smmu_remove().
Signed-off-by: Nicolin Chen
---
Changelog
v1->v2
* Added error-out labels in tegra_smmu_probe()
* Dropped pci_request_acs() since IOMMU core would call it.
drivers/iommu/tegra-smmu.c | 33 -
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index cf4981369828..74d84908679e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -856,6 +857,7 @@ static struct iommu_group *tegra_smmu_device_group(struct
device *dev)
const struct tegra_smmu_group_soc *soc;
unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
+ bool pci = dev_is_pci(dev);
struct iommu_group *grp;
/* Find group_soc associating with swgroup */
@@ -882,7 +884,7 @@ static struct iommu_group *tegra_smmu_device_group(struct
device *dev)
group->smmu = smmu;
group->soc = soc;
- group->group = iommu_group_alloc();
+ group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(>lock);
@@ -1079,26 +1081,39 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(>iommu, dev->fwnode);
err = iommu_device_register(>iommu);
- if (err) {
- iommu_device_sysfs_remove(>iommu);
- return ERR_PTR(err);
- }
+ if (err)
+ goto err_sysfs;
err = bus_set_iommu(_bus_type, _smmu_ops);
- if (err < 0) {
- iommu_device_unregister(>iommu);
- iommu_device_sysfs_remove(>iommu);
- return ERR_PTR(err);
+ if (err < 0)
+ goto err_unregister;
+
+#ifdef CONFIG_PCI
+ if (!iommu_present(_bus_type)) {
+ err = bus_set_iommu(_bus_type, _smmu_ops);
+ if (err < 0)
+ goto err_bus_set;
}
+#endif
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
return smmu;
+
+err_bus_set:
+ bus_set_iommu(_bus_type, NULL);
+err_unregister:
+ iommu_device_unregister(>iommu);
+err_sysfs:
+ iommu_device_sysfs_remove(>iommu);
+
+ return ERR_PTR(err);
}
void tegra_smmu_remove(struct tegra_smmu *smmu)
{
+ bus_set_iommu(_bus_type, NULL);
iommu_device_unregister(>iommu);
iommu_device_sysfs_remove(>iommu);
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 05/13] x86: Add early TPM1.2/TPM2.0 interface support for Secure Launch
On 9/25/20 1:43 AM, Jarkko Sakkinen wrote: > On Thu, Sep 24, 2020 at 10:58:33AM -0400, Ross Philipson wrote: >> From: "Daniel P. Smith" >> >> This commit introduces an abstraction for TPM1.2 and TPM2.0 devices >> above the TPM hardware interface. >> >> Signed-off-by: Daniel P. Smith >> Signed-off-by: Ross Philipson > > This is way, way too PoC. I wonder why there is no RFC tag. An RFC was sent back in March and we incorporated the feedback we received at that time. > Please also read section 2 of > > https://www.kernel.org/doc/html/v5.8/process/submitting-patches.html > > You should leverage existing TPM code in a way or another. Refine it so > that it scales for your purpose and then compile it into your thing > (just include the necesary C-files with relative paths). We explained during the RFC phase that we took a fair bit of time and a very hard look to see if we could #include sections out the TPM driver but as it is today none of the TPM driver's c files can be included outside of the mainline kernel. If you look at the early boot stub for the compressed kernel you will see that we are interacting with the TPM as the first thing upon leaving the assembly world and entering C. Since we weren't going to be able to get the mainline TPM driver plucked down, we could either 1.) borrow an implementation from a colleague that provides the minimum command strings hard coded in C macros to send measurements to the TPM or 2.) reuse the TPM implementation we wrote for TrenchBoot's AMD Secure Loader (LZ). The former is not well supported and the latter will be getting maintenance under TB. While this is not preferred, we had to weigh this versus trying to convince you and the other TPM driver maintainers on a significant refactoring of the TPM driver. It was elected for the reuse of a clean implementation that can be replaced later if/when the TPM driver was refactored. When we explained this during the RFC and it was not rejected, therefore we carried it forward into this submission. > How it is now is never going to fly. We would gladly work with you and the other TPM maintainers on a refactoring of the TPM driver to separate core logic into standalone files that both the driver and the compressed kernel can share. > /Jarkko > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI
Hi Thomas, On 8/26/2020 4:16 AM, Thomas Gleixner wrote: This is the second version of providing a base to support device MSI (non PCI based) and on top of that support for IMS (Interrupt Message Storm) based devices in a halfways architecture independent way. The first version can be found here: https://lore.kernel.org/r/20200821002424.119492...@linutronix.de It's still a mixed bag of bug fixes, cleanups and general improvements which are worthwhile independent of device MSI. There are quite a bunch of issues to solve: - X86 does not use the device::msi_domain pointer for historical reasons and due to XEN, which makes it impossible to create an architecture agnostic device MSI infrastructure. - X86 has it's own msi_alloc_info data type which is pointlessly different from the generic version and does not allow to share code. - The logic of composing MSI messages in an hierarchy is busted at the core level and of course some (x86) drivers depend on that. - A few minor shortcomings as usual This series addresses that in several steps: 1) Accidental bug fixes iommu/amd: Prevent NULL pointer dereference 2) Janitoring x86/init: Remove unused init ops PCI: vmd: Dont abuse vector irqomain as parent x86/msi: Remove pointless vcpu_affinity callback 3) Sanitizing the composition of MSI messages in a hierarchy genirq/chip: Use the first chip in irq_chip_compose_msi_msg() x86/msi: Move compose message callback where it belongs 4) Simplification of the x86 specific interrupt allocation mechanism x86/irq: Rename X86_IRQ_ALLOC_TYPE_MSI* to reflect PCI dependency x86/irq: Add allocation type for parent domain retrieval iommu/vt-d: Consolidate irq domain getter iommu/amd: Consolidate irq domain getter iommu/irq_remapping: Consolidate irq domain lookup 5) Consolidation of the X86 specific interrupt allocation mechanism to be as close as possible to the generic MSI allocation mechanism which allows to get rid of quite a bunch of x86'isms which are pointless x86/irq: Prepare consolidation of irq_alloc_info x86/msi: Consolidate HPET allocation x86/ioapic: Consolidate IOAPIC allocation x86/irq: Consolidate DMAR irq allocation x86/irq: Consolidate UV domain allocation PCI/MSI: Rework pci_msi_domain_calc_hwirq() x86/msi: Consolidate MSI allocation x86/msi: Use generic MSI domain ops 6) x86 specific cleanups to remove the dependency on arch_*_msi_irqs() x86/irq: Move apic_post_init() invocation to one place x86/pci: Reducde #ifdeffery in PCI init code x86/irq: Initialize PCI/MSI domain at PCI init time irqdomain/msi: Provide DOMAIN_BUS_VMD_MSI PCI: vmd: Mark VMD irqdomain with DOMAIN_BUS_VMD_MSI PCI/MSI: Provide pci_dev_has_special_msi_domain() helper x86/xen: Make xen_msi_init() static and rename it to xen_hvm_msi_init() x86/xen: Rework MSI teardown x86/xen: Consolidate XEN-MSI init irqdomain/msi: Allow to override msi_domain_alloc/free_irqs() x86/xen: Wrap XEN MSI management into irqdomain iommm/vt-d: Store irq domain in struct device iommm/amd: Store irq domain in struct device x86/pci: Set default irq domain in pcibios_add_device() PCI/MSI: Make arch_.*_msi_irq[s] fallbacks selectable x86/irq: Cleanup the arch_*_msi_irqs() leftovers x86/irq: Make most MSI ops XEN private iommu/vt-d: Remove domain search for PCI/MSI[X] iommu/amd: Remove domain search for PCI/MSI 7) X86 specific preparation for device MSI x86/irq: Add DEV_MSI allocation type x86/msi: Rename and rework pci_msi_prepare() to cover non-PCI MSI 8) Generic device MSI infrastructure platform-msi: Provide default irq_chip:: Ack genirq/proc: Take buslock on affinity write genirq/msi: Provide and use msi_domain_set_default_info_flags() platform-msi: Add device MSI infrastructure irqdomain/msi: Provide msi_alloc/free_store() callbacks 9) POC of IMS (Interrupt Message Storm) irq domain and irqchip implementations for both device array and queue storage. irqchip: Add IMS (Interrupt Message Storm) driver - NOT FOR MERGING Changes vs. V1: - Addressed various review comments and addressed the 0day fallout. - Corrected the XEN logic (Jürgen) - Make the arch fallback in PCI/MSI opt-in not opt-out (Bjorn) - Fixed the compose MSI message inconsistency - Ensure that the necessary flags are set for device SMI - Make the irq bus logic work for affinity setting to prepare support for IMS storage in queue memory. It turned out to be less scary than I feared. - Remove leftovers in iommu/intel|amd - Reworked the IMS POC driver to cover queue storage so Jason can have a look whether that fits the needs of
Re: [PATCH v3 0/6] Add virtio-iommu built-in topology
On 24 Sep 2020 11:54, Auger Eric wrote: > Hi, > > Adding Al in the loop > > On 9/24/20 11:38 AM, Michael S. Tsirkin wrote: > > On Thu, Sep 24, 2020 at 11:21:29AM +0200, Joerg Roedel wrote: > >> On Thu, Sep 24, 2020 at 05:00:35AM -0400, Michael S. Tsirkin wrote: > >>> OK so this looks good. Can you pls repost with the minor tweak > >>> suggested and all acks included, and I will queue this? > >> > >> My NACK still stands, as long as a few questions are open: > >> > >>1) The format used here will be the same as in the ACPI table? I > >> think the answer to this questions must be Yes, so this leads > >> to the real question: > > > > I am not sure it's a must. > > We can always tweak the parser if there are slight differences > > between ACPI and virtio formats. > > > > But we do want the virtio format used here to be approved by the virtio > > TC, so it won't change. As long as we can convey the same content to the UEFI ASWG, we're fine. Format/syntax of the submittal is not absolutely critical though it does need translating to what the ASWG expects (see https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process for details -- basically a bugzilla with markdown text. > > Eric, Jean-Philippe, does one of you intend to create a github issue > > and request a ballot for the TC? It's been posted end of August with no > > changes ... > Jean-Philippe, would you? > > > >>2) Has the ACPI table format stabalized already? If and only if > >> the answer is Yes I will Ack these patches. We don't need to > >> wait until the ACPI table format is published in a > >> specification update, but at least some certainty that it > >> will not change in incompatible ways anymore is needed. > >> > > Al, do you have any news about the the VIOT definition submission to > the UEFI ASWG? > > Thank you in advance > > Best Regards > > Eric > > > > > > Not that I know, but I don't see why it's a must. > > > >> So what progress has been made with the ACPI table specification, is it > >> just a matter of time to get it approved or are there concerns? > >> > >> Regards, > >> > >>Joerg My apologies for the delay. No excuses, just not enough hours in the day. I will make the proper submission to the ASWG later today to have it considered later this week -- I had quite a bit of confusion around how the process is supposed to work but I think we've got that cleared up (see the link noted above). The content of the table appears to be in really good shape. Will it change? Possibly, but my expectation is that it will be minor details, nothing wholesale; having the table in use in code tends to act as a pretty fierce restraint on making changes (there's a lot of precedent for that in ASWG). The biggest question is: are there any objections to having this table description licensed under Creative Commons Attribution International 4.0 (see https://spdx.org/licenses/CC-BY-4.0.html)? This is just for the table description, not the code. If there are, that needs to be cleared up first. If not, then the submittal this week should happen. Once submitted to ASWG, there is a very slim chance it will end up in ACPI 6.4 which is mostly done now -- very, very slim, but stranger things have happened. Most likely, once approved it would be in ACPI 6.5, sometime in 2021. I'll post the link to the submittal as soon as I can. Again, my apologies for the delays; approval in the spec can proceed pretty much independent of the implementation, and vice versa. That's really the whole point of this new process with the UEFI Forum. -- ciao, al --- Al Stone Software Engineer Red Hat, Inc. a...@redhat.com --- ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 03/13] x86: Add early SHA support for Secure Launch early measurements
On Thu, Sep 24, 2020 at 11:00 AM Ross Philipson
wrote:
>
> The SHA algorithms are necessary to measure configuration information into
> the TPM as early as possible before using the values. This implementation
> uses the established approach of #including the SHA libraries directly in
> the code since the compressed kernel is not uncompressed at this point.
>
> The SHA code here has its origins in the code from the main kernel. That
> code could not be pulled directly into the setup portion of the compressed
> kernel because of other dependencies it pulls in. The result is this is a
> modified copy of that code that still leverages the core SHA algorithms.
>
> Signed-off-by: Daniel P. Smith
> Signed-off-by: Ross Philipson
> ---
> arch/x86/boot/compressed/Makefile | 4 +
> arch/x86/boot/compressed/early_sha1.c | 104
> arch/x86/boot/compressed/early_sha1.h | 17 +++
> arch/x86/boot/compressed/early_sha256.c | 6 +
> arch/x86/boot/compressed/early_sha512.c | 6 +
> include/linux/sha512.h | 21
> lib/sha1.c | 4 +
> lib/sha512.c| 209
>
> 8 files changed, 371 insertions(+)
> create mode 100644 arch/x86/boot/compressed/early_sha1.c
> create mode 100644 arch/x86/boot/compressed/early_sha1.h
> create mode 100644 arch/x86/boot/compressed/early_sha256.c
> create mode 100644 arch/x86/boot/compressed/early_sha512.c
> create mode 100644 include/linux/sha512.h
> create mode 100644 lib/sha512.c
>
> diff --git a/arch/x86/boot/compressed/Makefile
> b/arch/x86/boot/compressed/Makefile
> index ff7894f..0fd84b9 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -96,6 +96,10 @@ vmlinux-objs-$(CONFIG_ACPI) += $(obj)/acpi.o
> vmlinux-objs-$(CONFIG_EFI_MIXED) += $(obj)/efi_thunk_$(BITS).o
> efi-obj-$(CONFIG_EFI_STUB) = $(objtree)/drivers/firmware/efi/libstub/lib.a
>
> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH) += $(obj)/early_sha1.o
> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH_SHA256) += $(obj)/early_sha256.o
> +vmlinux-objs-$(CONFIG_SECURE_LAUNCH_SHA512) += $(obj)/early_sha512.o
> +
> # The compressed kernel is built with -fPIC/-fPIE so that a boot loader
> # can place it anywhere in memory and it will still run. However, since
> # it is executed as-is without any ELF relocation processing performed
> diff --git a/arch/x86/boot/compressed/early_sha1.c
> b/arch/x86/boot/compressed/early_sha1.c
> new file mode 100644
> index 000..198c46d
> --- /dev/null
> +++ b/arch/x86/boot/compressed/early_sha1.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, Oracle and/or its affiliates.
> + * Copyright (c) 2020 Apertus Solutions, LLC.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "early_sha1.h"
> +
> +#define SHA1_DISABLE_EXPORT
> +#include "../../../../lib/sha1.c"
> +
> +/* The SHA1 implementation in lib/sha1.c was written to get the workspace
> + * buffer as a parameter. This wrapper function provides a container
> + * around a temporary workspace that is cleared after the transform
> completes.
> + */
> +static void __sha_transform(u32 *digest, const char *data)
> +{
> + u32 ws[SHA1_WORKSPACE_WORDS];
> +
> + sha1_transform(digest, data, ws);
> +
> + memset(ws, 0, sizeof(ws));
> + /*
> +* As this is cryptographic code, prevent the memset 0 from being
> +* optimized out potentially leaving secrets in memory.
> +*/
> + wmb();
You can use memzero_explicit instead of open coding it.
Regards,
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/3] iommu: Reserved regions for IOVAs beyond dma_mask and iommu aperture
On Tue, 29 Sep 2020 09:18:22 +0200
Auger Eric wrote:
> Hi all,
>
> [also correcting some outdated email addresses + adding Lorenzo in cc]
>
> On 9/29/20 12:42 AM, Alex Williamson wrote:
> > On Mon, 28 Sep 2020 21:50:34 +0200
> > Eric Auger wrote:
> >
> >> VFIO currently exposes the usable IOVA regions through the
> >> VFIO_IOMMU_GET_INFO ioctl / VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE
> >> capability. However it fails to take into account the dma_mask
> >> of the devices within the container. The top limit currently is
> >> defined by the iommu aperture.
> >
> > I think that dma_mask is traditionally a DMA API interface for a device
> > driver to indicate to the DMA layer which mappings are accessible to the
> > device. On the other hand, vfio makes use of the IOMMU API where the
> > driver is in userspace. That userspace driver has full control of the
> > IOVA range of the device, therefore dma_mask is mostly irrelevant to
> > vfio. I think the issue you're trying to tackle is that the IORT code
> > is making use of the dma_mask to try to describe a DMA address
> > limitation imposed by the PCI root bus, living between the endpoint
> > device and the IOMMU. Therefore, if the IORT code is exposing a
> > topology or system imposed device limitation, this seems much more akin
> > to something like an MSI reserved range, where it's not necessarily the
> > device or the IOMMU with the limitation, but something that sits
> > between them.
>
> First I think I failed to explain the context. I worked on NVMe
> passthrough on ARM. The QEMU NVMe backend uses VFIO to program the
> physical device. The IOVA allocator there currently uses an IOVA range
> within [0x1, 1ULL << 39]. This IOVA layout rather is arbitrary if I
> understand correctly.
39 bits is the minimum available on some VT-d systems, so it was
probably considered a reasonable minimum address width to consider.
> I noticed we rapidly get some VFIO MAP DMA
> failures because the allocated IOVA collide with the ARM MSI reserved
> IOVA window [0x800, 0x810]. Since 9b77e5c79840 ("vfio/type1:
> Check reserved region conflict and update iova list"), such VFIO MAP DMA
> attempts to map IOVAs belonging to host reserved IOVA windows fail. So,
> by using the VFIO_IOMMU_GET_INFO ioctl /
> VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE I can change the IOVA allocator to
> avoid allocating within this range and others. While working on this, I
> tried to automatically compute the min/max IOVAs and change the
> arbitrary [0x1, 1ULL << 39]. My SMMUv2 supports up to 48b so
> naturally the max IOVA was computed as 1ULL << 48. The QEMU NVMe backend
> allocates at the bottom and at the top of the range. I noticed the use
> case was not working as soon as the top IOVA was more than 1ULL << 42.
> And then we noticed the dma_mask was set to 42 by using
> cat /sys/bus/pci/devices/0005:01:00.0/dma_mask_bits. So my
> interpretation is the dma_mask was somehow containing the info the
> device couldn't handle IOVAs beyond a certain limit.
I see that there are both OF and ACPI hooks in pci_dma_configure() and
both modify dev->dma_mask, which is what pci-sysfs is exposing here,
but I'm not convinced this even does what it's intended to do. The
driver core calls this via the bus->dma_configure callback before
probing a driver, but then what happens when the driver calls
pci_set_dma_mask()? This is just a wrapper for dma_set_mask() and I
don't see anywhere that would take into account the existing
dev->dma_mask. It seems for example that pci_dma_configure() could
produce a 42 bit mask as we have here, then the driver could override
that with anything that the dma_ops.dma_supported() callback finds
acceptable, and I don't see any instances where the current
dev->dma_mask is considered. Am I overlooking something?
> In my case the 42b limit is computed in iort_dma_setup() by
> acpi_dma_get_range(dev, , , );
>
> Referring to the comment, it does "Evaluate DMA regions and return
> respectively DMA region start, offset and size in dma_addr, offset and
> size on parsing success". This parses the ACPI table, looking for ACPI
> companions with _DMA methods.
>
> But as Alex mentioned, the IORT also allows to define limits on "the
> number of address bits, starting from the least significant bit that can
> be generated by a device when it accesses memory". See Named component
> node.Device Memory Address Size limit or PCI root complex node. Memory
> address size limit.
>
> ret = acpi_dma_get_range(dev, , , );
> if (ret == -ENODEV)
> ret = dev_is_pci(dev) ? rc_dma_get_range(dev, )
> : nc_dma_get_range(dev, );
>
> So eventually those info collected from the ACPI tables which do impact
> the usable IOVA range seem to be stored in the dma_mask, hence that
> proposal.
As above, it's not clear to me that anyone other than the driver and
the dma_supported() callback on dma_ops have any input on
Re: [PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range
29.09.2020 09:13, Nicolin Chen пишет: > This is used to protect potential race condition at use_count. > since probes of client drivers, calling attach_dev(), may run > concurrently. > > Signed-off-by: Nicolin Chen > --- It's always better not to mix success and error code paths in order to keep code readable, but not a big deal in the case of this particular patch since the changed code is quite simple. Please try to avoid doing this in the future patches. Also, please note that in general it's better to use locked/unlocked versions for the functions like it's already done for tegra_smmu_map/unmap, this will remove the need to maintain lockings in the code. The code touched by this patch doesn't have complicated code paths, so it's good enough to me. Reviewed-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get
29.09.2020 09:13, Nicolin Chen пишет: > The tegra_smmu_group_get was added to group devices in different > SWGROUPs and it'd return a NULL group pointer upon a mismatch at > tegra_smmu_find_group(), so for most of clients/devices, it very > likely would mismatch and need a fallback generic_device_group(). > > But now tegra_smmu_group_get handles devices in same SWGROUP too, > which means that it would allocate a group for every new SWGROUP > or would directly return an existing one upon matching a SWGROUP, > i.e. any device will go through this function. > > So possibility of having a NULL group pointer in device_group() > is upon failure of either devm_kzalloc() or iommu_group_alloc(). > In either case, calling generic_device_group() no longer makes a > sense. Especially for devm_kzalloc() failing case, it'd cause a > problem if it fails at devm_kzalloc() yet succeeds at a fallback > generic_device_group(), because it does not create a group->list > for other devices to match. > > This patch simply unwraps the function to clean it up. > > Signed-off-by: Nicolin Chen > --- Reviewed-by: Dmitry Osipenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2] iommu/qcom: add missing put_device() call in qcom_iommu_of_xlate()
On Tue, 29 Sep 2020 09:40:37 +0800, Yu Kuai wrote: > if of_find_device_by_node() succeed, qcom_iommu_of_xlate() doesn't have > a corresponding put_device(). Thus add put_device() to fix the exception > handling for this function implementation. Applied to will (for-joerg/arm-smmu/updates), thanks! [1/1] iommu/qcom: add missing put_device() call in qcom_iommu_of_xlate() https://git.kernel.org/will/c/e2eae09939a8 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub
On Tue, Sep 29, 2020 at 10:03:47AM -0400, Ross Philipson wrote: > On 9/25/20 3:18 PM, Arvind Sankar wrote: > > You will also need to avoid initializing data with symbol addresses. > > > > .long mle_header > > .long sl_stub_entry > > .long sl_gdt > > ... > > > > The other two are more messy, unfortunately there is no easy way to tell > > the linker what we want here. The other entry point addresses (for the > > EFI stub) are populated in a post-processing step after the compressed > > kernel has been linked, we could teach it to also update kernel_info. > > > > Without that, for kernel_info, you could change it to store the offset > > of the MLE header from kernel_info, instead of from the start of the > > image. Actually, kernel_info is currently placed inside .rodata, which is quite a ways into the compressed kernel, so an offset from kernel_info would end up having to be negative, which would be ugly. I'll see if I can come up with some way to avoid this. > > > > For the MLE header, it could be moved to .head.text in head_64.S, and > > initialized with > > .long rva(sl_stub) > > This will also let it be placed at a fixed offset from startup_32, so > > that kernel_info can just be populated with a constant. > > Thank you for the detailed reply. I am going to start digging into this now. > > Ross > > > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub
On 9/25/20 3:18 PM, Arvind Sankar wrote: > On Fri, Sep 25, 2020 at 10:56:43AM -0400, Ross Philipson wrote: >> On 9/24/20 1:38 PM, Arvind Sankar wrote: >>> On Thu, Sep 24, 2020 at 10:58:35AM -0400, Ross Philipson wrote: >>> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 97d37f0..42043bf 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -279,6 +279,21 @@ SYM_INNER_LABEL(efi32_pe_stub_entry, SYM_L_LOCAL) SYM_FUNC_END(efi32_stub_entry) #endif +#ifdef CONFIG_SECURE_LAUNCH +SYM_FUNC_START(sl_stub_entry) + /* + * On entry, %ebx has the entry abs offset to sl_stub_entry. To + * find the beginning of where we are loaded, sub off from the + * beginning. + */ >>> >>> This requirement should be added to the documentation. Is it necessary >>> or can this stub just figure out the address the same way as the other >>> 32-bit entry points, using the scratch space in bootparams as a little >>> stack? >> >> It is based on the state of the BSP when TXT vectors to the measured >> launch environment. It is documented in the TXT spec and the SDMs. >> > > I think it would be useful to add to the x86 boot documentation how > exactly this new entry point is called, even if it's just adding a link > to some section of those specs. The doc should also say that an > mle_header_offset of 0 means the kernel isn't secure launch enabled. Ok will do. > >>> >>> For the 32-bit assembler code that's being added, tip/master now has >>> changes that prevent the compressed kernel from having any runtime >>> relocations. You'll need to revise some of the code and the data >>> structures initial values to avoid creating relocations. >> >> Could you elaborate on this some more? I am not sure I see places in the >> secure launch asm that would be creating relocations like this. >> >> Thank you, >> Ross >> > > You should see them if you do > readelf -r arch/x86/boot/compressed/vmlinux > > In terms of the code, things like: > > addl%ebx, (sl_gdt_desc + 2)(%ebx) > > will create a relocation, because the linker interprets this as wanting > the runtime address of sl_gdt_desc, rather than just the offset from > startup_32. > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/arch/x86/boot/compressed/head_64.S*n48__;Iw!!GqivPVa7Brio!JpZWv1cCPZdjD2jbCCGT7P9UIVl_lhX7YjckAnUcvi927jwZI7X3nX0MpIAZOyktJds$ > > > has a comment with some explanation and a macro that the 32-bit code in > startup_32 uses to avoid creating relocations. > > Since the SL code is in a different assembler file (and a different > section), you can't directly use the same macro. I would suggest getting > rid of sl_stub_entry and entering directly at sl_stub, and then the code > in sl_stub.S can use sl_stub for the base address, defining the rva() > macro there as > > #define rva(X) ((X) - sl_stub) > > You will also need to avoid initializing data with symbol addresses. > > .long mle_header > .long sl_stub_entry > .long sl_gdt > > will create relocations. The third one is easy, just replace it with > sl_gdt - sl_gdt_desc and initialize it at runtime with > > lealrva(sl_gdt_desc)(%ebx), %eax > addl%eax, 2(%eax) > lgdt(%eax) > > The other two are more messy, unfortunately there is no easy way to tell > the linker what we want here. The other entry point addresses (for the > EFI stub) are populated in a post-processing step after the compressed > kernel has been linked, we could teach it to also update kernel_info. > > Without that, for kernel_info, you could change it to store the offset > of the MLE header from kernel_info, instead of from the start of the > image. > > For the MLE header, it could be moved to .head.text in head_64.S, and > initialized with > .long rva(sl_stub) > This will also let it be placed at a fixed offset from startup_32, so > that kernel_info can just be populated with a constant. Thank you for the detailed reply. I am going to start digging into this now. Ross > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 6/6] iommu/vt-d: Check UAPI data processed by IOMMU core
Hi Jacob,
On 9/25/20 6:32 PM, Jacob Pan wrote:
> IOMMU generic layer already does sanity checks on UAPI data for version
> match and argsz range based on generic information.
>
> This patch adjusts the following data checking responsibilities:
> - removes the redundant version check from VT-d driver
> - removes the check for vendor specific data size
> - adds check for the use of reserved/undefined flags
>
> Signed-off-by: Jacob Pan
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> drivers/iommu/intel/iommu.c | 3 +--
> drivers/iommu/intel/svm.c | 11 +--
> include/uapi/linux/iommu.h | 1 +
> 3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 461f3a6864d4..18ed3b3c70d7 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5408,8 +5408,7 @@ intel_iommu_sva_invalidate(struct iommu_domain *domain,
> struct device *dev,
> int ret = 0;
> u64 size = 0;
>
> - if (!inv_info || !dmar_domain ||
> - inv_info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> + if (!inv_info || !dmar_domain)
> return -EINVAL;
>
> if (!dev || !dev_is_pci(dev))
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 99353d6468fa..0cb9a15f1112 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -284,8 +284,15 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain,
> struct device *dev,
> if (WARN_ON(!iommu) || !data)
> return -EINVAL;
>
> - if (data->version != IOMMU_GPASID_BIND_VERSION_1 ||
> - data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> + if (data->format != IOMMU_PASID_FORMAT_INTEL_VTD)
> + return -EINVAL;
> +
> + /* IOMMU core ensures argsz is more than the start of the union */
> + if (data->argsz < offsetofend(struct iommu_gpasid_bind_data,
> vendor.vtd))
> + return -EINVAL;
> +
> + /* Make sure no undefined flags are used in vendor data */
> + if (data->vendor.vtd.flags & ~(IOMMU_SVA_VTD_GPASID_LAST - 1))
> return -EINVAL;
>
> if (!dev_is_pci(dev))
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index 66d4ca40b40f..e1d9e75f2c94 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -288,6 +288,7 @@ struct iommu_gpasid_bind_data_vtd {
> #define IOMMU_SVA_VTD_GPASID_PWT (1 << 3) /* page-level write through */
> #define IOMMU_SVA_VTD_GPASID_EMTE(1 << 4) /* extended mem type enable */
> #define IOMMU_SVA_VTD_GPASID_CD (1 << 5) /* PASID-level cache
> disable */
> +#define IOMMU_SVA_VTD_GPASID_LAST(1 << 6)
> __u64 flags;
> __u32 pat;
> __u32 emt;
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 5/6] iommu/uapi: Handle data and argsz filled by users
Hi Jacob,
On 9/25/20 6:32 PM, Jacob Pan wrote:
> IOMMU user APIs are responsible for processing user data. This patch
> changes the interface such that user pointers can be passed into IOMMU
> code directly. Separate kernel APIs without user pointers are introduced
> for in-kernel users of the UAPI functionality.
>
> IOMMU UAPI data has a user filled argsz field which indicates the data
> length of the structure. User data is not trusted, argsz must be
> validated based on the current kernel data size, mandatory data size,
> and feature flags.
>
> User data may also be extended, resulting in possible argsz increase.
> Backward compatibility is ensured based on size and flags (or
> the functional equivalent fields) checking.
>
> This patch adds sanity checks in the IOMMU layer. In addition to argsz,
> reserved/unused fields in padding, flags, and version are also checked.
> Details are documented in Documentation/userspace-api/iommu.rst
>
> Reviewed-by: Jean-Philippe Brucker
> Signed-off-by: Liu Yi L
> Signed-off-by: Jacob Pan
Reviewed-by: Eric Auger
Thanks
Eric
> ---
> drivers/iommu/iommu.c | 194
> +++--
> include/linux/iommu.h | 28 ---
> include/uapi/linux/iommu.h | 1 +
> 3 files changed, 207 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4ae02291ccc2..a11f2733dc54 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1961,34 +1961,214 @@ int iommu_attach_device(struct iommu_domain *domain,
> struct device *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_attach_device);
>
> +/*
> + * Check flags and other user provided data for valid combinations. We also
> + * make sure no reserved fields or unused flags are set. This is to ensure
> + * not breaking userspace in the future when these fields or flags are used.
> + */
> +static int iommu_check_cache_invl_data(struct iommu_cache_invalidate_info
> *info)
> +{
> + u32 mask;
> + int i;
> +
> + if (info->version != IOMMU_CACHE_INVALIDATE_INFO_VERSION_1)
> + return -EINVAL;
> +
> + mask = (1 << IOMMU_CACHE_INV_TYPE_NR) - 1;
> + if (info->cache & ~mask)
> + return -EINVAL;
> +
> + if (info->granularity >= IOMMU_INV_GRANU_NR)
> + return -EINVAL;
> +
> + switch (info->granularity) {
> + case IOMMU_INV_GRANU_ADDR:
> + if (info->cache & IOMMU_CACHE_INV_TYPE_PASID)
> + return -EINVAL;
> +
> + mask = IOMMU_INV_ADDR_FLAGS_PASID |
> + IOMMU_INV_ADDR_FLAGS_ARCHID |
> + IOMMU_INV_ADDR_FLAGS_LEAF;
> +
> + if (info->granu.addr_info.flags & ~mask)
> + return -EINVAL;
> + break;
> + case IOMMU_INV_GRANU_PASID:
> + mask = IOMMU_INV_PASID_FLAGS_PASID |
> + IOMMU_INV_PASID_FLAGS_ARCHID;
> + if (info->granu.pasid_info.flags & ~mask)
> + return -EINVAL;
> +
> + break;
> + case IOMMU_INV_GRANU_DOMAIN:
> + if (info->cache & IOMMU_CACHE_INV_TYPE_DEV_IOTLB)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /* Check reserved padding fields */
> + for (i = 0; i < sizeof(info->padding); i++) {
> + if (info->padding[i])
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> int iommu_uapi_cache_invalidate(struct iommu_domain *domain, struct device
> *dev,
> - struct iommu_cache_invalidate_info *inv_info)
> + void __user *uinfo)
> {
> + struct iommu_cache_invalidate_info inv_info = { 0 };
> + u32 minsz;
> + int ret;
> +
> if (unlikely(!domain->ops->cache_invalidate))
> return -ENODEV;
>
> - return domain->ops->cache_invalidate(domain, dev, inv_info);
> + /*
> + * No new spaces can be added before the variable sized union, the
> + * minimum size is the offset to the union.
> + */
> + minsz = offsetof(struct iommu_cache_invalidate_info, granu);
> +
> + /* Copy minsz from user to get flags and argsz */
> + if (copy_from_user(_info, uinfo, minsz))
> + return -EFAULT;
> +
> + /* Fields before the variable size union are mandatory */
> + if (inv_info.argsz < minsz)
> + return -EINVAL;
> +
> + /* PASID and address granu require additional info beyond minsz */
> + if (inv_info.granularity == IOMMU_INV_GRANU_PASID &&
> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
> granu.pasid_info))
> + return -EINVAL;
> +
> + if (inv_info.granularity == IOMMU_INV_GRANU_ADDR &&
> + inv_info.argsz < offsetofend(struct iommu_cache_invalidate_info,
> granu.addr_info))
> + return -EINVAL;
> +
> + /*
> + * User might be using a
Re: [PATCH] iommu/arm-smmu-v3: Add rmb after reading event queue prod_reg
On 2020/9/29 6:13, Will Deacon wrote: > On Mon, 28 Sep 2020 16:32:02 +0800, Zhou Wang wrote: >> In arm_smmu_evtq_thread, reading event queue is from consumer pointer, >> which has no address dependency on producer pointer, prog_reg(MMIO) and >> event queue memory(Normal memory) can disorder. So the load for event queue >> can be done before the load of prod_reg, then perhaps wrong event entry >> value will be got. >> >> Add rmb to make sure to get correct event queue entry value. > > Applied to will (for-joerg/arm-smmu/updates), thanks! > > [1/1] iommu/arm-smmu-v3: Add rmb after reading event queue prod_reg > https://git.kernel.org/will/c/a76a3f2c > > (please note that I changed the patch to use readl() instead of an rmb() > in conjunction with the _relaxed() accessor, and then adjusted the cons > side to match in terms of DMB vs DSB). Thanks for taking this patch! Best, Zhou > > Cheers, > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 2/3] iommu: Account for dma_mask and iommu aperture in IOVA reserved regions
Hi Christoph,
On 9/29/20 8:03 AM, Christoph Hellwig wrote:
> On Mon, Sep 28, 2020 at 09:50:36PM +0200, Eric Auger wrote:
>> VFIO currently exposes the usable IOVA regions through the
>> VFIO_IOMMU_GET_INFO ioctl. However it fails to take into account
>> the dma_mask of the devices within the container. The top limit
>> currently is defined by the iommu aperture.
>
> Can we take a step back here? The dma_mask only has a meaning for
> the DMA API, and not the iommu API, it should have no relevance here.
>
> More importantly if we are using vfio no dma_mask should be set to
> start with.
You will find more context in my reply to Alex.
Thanks
Eric
>
>> +if (geo.aperture_end < ULLONG_MAX && geo.aperture_end !=
>> geo.aperture_start) {
>
> Please avoid pointlessly overlong lines.
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 0/3] iommu: Reserved regions for IOVAs beyond dma_mask and iommu aperture
Hi all,
[also correcting some outdated email addresses + adding Lorenzo in cc]
On 9/29/20 12:42 AM, Alex Williamson wrote:
> On Mon, 28 Sep 2020 21:50:34 +0200
> Eric Auger wrote:
>
>> VFIO currently exposes the usable IOVA regions through the
>> VFIO_IOMMU_GET_INFO ioctl / VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE
>> capability. However it fails to take into account the dma_mask
>> of the devices within the container. The top limit currently is
>> defined by the iommu aperture.
>
> I think that dma_mask is traditionally a DMA API interface for a device
> driver to indicate to the DMA layer which mappings are accessible to the
> device. On the other hand, vfio makes use of the IOMMU API where the
> driver is in userspace. That userspace driver has full control of the
> IOVA range of the device, therefore dma_mask is mostly irrelevant to
> vfio. I think the issue you're trying to tackle is that the IORT code
> is making use of the dma_mask to try to describe a DMA address
> limitation imposed by the PCI root bus, living between the endpoint
> device and the IOMMU. Therefore, if the IORT code is exposing a
> topology or system imposed device limitation, this seems much more akin
> to something like an MSI reserved range, where it's not necessarily the
> device or the IOMMU with the limitation, but something that sits
> between them.
First I think I failed to explain the context. I worked on NVMe
passthrough on ARM. The QEMU NVMe backend uses VFIO to program the
physical device. The IOVA allocator there currently uses an IOVA range
within [0x1, 1ULL << 39]. This IOVA layout rather is arbitrary if I
understand correctly. I noticed we rapidly get some VFIO MAP DMA
failures because the allocated IOVA collide with the ARM MSI reserved
IOVA window [0x800, 0x810]. Since 9b77e5c79840 ("vfio/type1:
Check reserved region conflict and update iova list"), such VFIO MAP DMA
attempts to map IOVAs belonging to host reserved IOVA windows fail. So,
by using the VFIO_IOMMU_GET_INFO ioctl /
VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE I can change the IOVA allocator to
avoid allocating within this range and others. While working on this, I
tried to automatically compute the min/max IOVAs and change the
arbitrary [0x1, 1ULL << 39]. My SMMUv2 supports up to 48b so
naturally the max IOVA was computed as 1ULL << 48. The QEMU NVMe backend
allocates at the bottom and at the top of the range. I noticed the use
case was not working as soon as the top IOVA was more than 1ULL << 42.
And then we noticed the dma_mask was set to 42 by using
cat /sys/bus/pci/devices/0005:01:00.0/dma_mask_bits. So my
interpretation is the dma_mask was somehow containing the info the
device couldn't handle IOVAs beyond a certain limit.
In my case the 42b limit is computed in iort_dma_setup() by
acpi_dma_get_range(dev, , , );
Referring to the comment, it does "Evaluate DMA regions and return
respectively DMA region start, offset and size in dma_addr, offset and
size on parsing success". This parses the ACPI table, looking for ACPI
companions with _DMA methods.
But as Alex mentioned, the IORT also allows to define limits on "the
number of address bits, starting from the least significant bit that can
be generated by a device when it accesses memory". See Named component
node.Device Memory Address Size limit or PCI root complex node. Memory
address size limit.
ret = acpi_dma_get_range(dev, , , );
if (ret == -ENODEV)
ret = dev_is_pci(dev) ? rc_dma_get_range(dev, )
: nc_dma_get_range(dev, );
So eventually those info collected from the ACPI tables which do impact
the usable IOVA range seem to be stored in the dma_mask, hence that
proposal.
>
>> So, for instance, if the IOMMU supports up to 48bits, it may give
>> the impression the max IOVA is 48b while a device may have a
>> dma_mask of 42b. So this API cannot really be used to compute
>> the max usable IOVA.
>>
>> This patch removes the IOVA region beyond the dma_mask's.
>
> Rather it adds a reserved region accounting for the range above the
> device's dma_mask.
Yep. It adds new reserved regions in
/sys/kernel/iommu_groups//reserved_regions and remove those from the
usable regions exposed by VFIO GET_INFO.
I don't think the IOMMU API should be consuming
> dma_mask like this though. For example, what happens in
> pci_dma_configure() when there are no OF or ACPI DMA restrictions?
My guess was that the dma_mask was set to the max range but I did not
test it.
It
> appears to me that the dma_mask from whatever previous driver had the
> device carries over to the new driver. That's generally ok for the DMA
> API because a driver is required to set the device's DMA mask. It
> doesn't make sense however to blindly consume that dma_mask and export
> it via an IOMMU API. For example I would expect to see different
> results depending on whether a host driver has been bound to a device.
> It seems the correct IOMMU API
"dma_alloc_coherent" is Failing in Memory Allocation for VFs
Hi I am facing below problem in CMA allocation in below scenario. CMA memory allocation doesnt happen for SRIOV device(VF device).we have enable below in configmake menuconfig-- CMA Reserved 400 MB IOMMU Hardware Support--IOMMU Passthrough by defaultEnable Intel DMA Remapping devices by defaultSupport for INTEL IOMMU using DMA Remapping DevicesSupport for Shared Virtual Memory with Inel IOMMU Please help in solving this problem. Thanks and Regards, Ankit Soni ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/2] iommu/tegra-smmu: Unwrap tegra_smmu_group_get
The tegra_smmu_group_get was added to group devices in different
SWGROUPs and it'd return a NULL group pointer upon a mismatch at
tegra_smmu_find_group(), so for most of clients/devices, it very
likely would mismatch and need a fallback generic_device_group().
But now tegra_smmu_group_get handles devices in same SWGROUP too,
which means that it would allocate a group for every new SWGROUP
or would directly return an existing one upon matching a SWGROUP,
i.e. any device will go through this function.
So possibility of having a NULL group pointer in device_group()
is upon failure of either devm_kzalloc() or iommu_group_alloc().
In either case, calling generic_device_group() no longer makes a
sense. Especially for devm_kzalloc() failing case, it'd cause a
problem if it fails at devm_kzalloc() yet succeeds at a fallback
generic_device_group(), because it does not create a group->list
for other devices to match.
This patch simply unwraps the function to clean it up.
Signed-off-by: Nicolin Chen
---
Changelog
v2->v4:
* N/A
v1->v2:
* Changed type of swgroup to "unsigned int", following Thierry's
commnets.
drivers/iommu/tegra-smmu.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 0becdbfea306..ec4c9dafff95 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -903,10 +903,12 @@ static void tegra_smmu_group_release(void *iommu_data)
mutex_unlock(>lock);
}
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
- unsigned int swgroup)
+static struct iommu_group *tegra_smmu_device_group(struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
const struct tegra_smmu_group_soc *soc;
+ unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
struct iommu_group *grp;
@@ -950,19 +952,6 @@ static struct iommu_group *tegra_smmu_group_get(struct
tegra_smmu *smmu,
return group->group;
}
-static struct iommu_group *tegra_smmu_device_group(struct device *dev)
-{
- struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
- struct iommu_group *group;
-
- group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
- if (!group)
- group = generic_device_group(dev);
-
- return group;
-}
-
static int tegra_smmu_of_xlate(struct device *dev,
struct of_phandle_args *args)
{
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 2/2] iommu/tegra-smmu: Expand mutex protection range
This is used to protect potential race condition at use_count.
since probes of client drivers, calling attach_dev(), may run
concurrently.
Signed-off-by: Nicolin Chen
---
Changelog
v3->v4:
* Fixed typo "Expend" => "Expand"
v2->v3:
* Renamed label "err_unlock" to "unlock"
v1->v2:
* N/A
drivers/iommu/tegra-smmu.c | 34 +-
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec4c9dafff95..6a3ecc334481 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -256,26 +256,19 @@ static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu,
unsigned int *idp)
{
unsigned long id;
- mutex_lock(>lock);
-
id = find_first_zero_bit(smmu->asids, smmu->soc->num_asids);
- if (id >= smmu->soc->num_asids) {
- mutex_unlock(>lock);
+ if (id >= smmu->soc->num_asids)
return -ENOSPC;
- }
set_bit(id, smmu->asids);
*idp = id;
- mutex_unlock(>lock);
return 0;
}
static void tegra_smmu_free_asid(struct tegra_smmu *smmu, unsigned int id)
{
- mutex_lock(>lock);
clear_bit(id, smmu->asids);
- mutex_unlock(>lock);
}
static bool tegra_smmu_capable(enum iommu_cap cap)
@@ -420,17 +413,21 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
{
u32 value;
- int err;
+ int err = 0;
+
+ mutex_lock(>lock);
if (as->use_count > 0) {
as->use_count++;
- return 0;
+ goto unlock;
}
as->pd_dma = dma_map_page(smmu->dev, as->pd, 0, SMMU_SIZE_PD,
DMA_TO_DEVICE);
- if (dma_mapping_error(smmu->dev, as->pd_dma))
- return -ENOMEM;
+ if (dma_mapping_error(smmu->dev, as->pd_dma)) {
+ err = -ENOMEM;
+ goto unlock;
+ }
/* We can't handle 64-bit DMA addresses */
if (!smmu_dma_addr_valid(smmu, as->pd_dma)) {
@@ -453,24 +450,35 @@ static int tegra_smmu_as_prepare(struct tegra_smmu *smmu,
as->smmu = smmu;
as->use_count++;
+ mutex_unlock(>lock);
+
return 0;
err_unmap:
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
+unlock:
+ mutex_unlock(>lock);
+
return err;
}
static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
struct tegra_smmu_as *as)
{
- if (--as->use_count > 0)
+ mutex_lock(>lock);
+
+ if (--as->use_count > 0) {
+ mutex_unlock(>lock);
return;
+ }
tegra_smmu_free_asid(smmu, as->id);
dma_unmap_page(smmu->dev, as->pd_dma, SMMU_SIZE_PD, DMA_TO_DEVICE);
as->smmu = NULL;
+
+ mutex_unlock(>lock);
}
static int tegra_smmu_attach_dev(struct iommu_domain *domain,
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 0/2] iommu/tegra-smmu: Two followup changes
Two followup patches for tegra-smmu: PATCH-1 is a clean-up patch for the recently applied SWGROUP change. PATCH-2 fixes a potential race condition Changelog v3->v4: * PATCH-2: Fixed typo in subject v2->v3: * PATCH-2: renamed "err_unlock" to "unlock" v1->v2: * Separated first two changs of V1 so they may get applied first, since the other three changes need some extra time to finalize. Nicolin Chen (2): iommu/tegra-smmu: Unwrap tegra_smmu_group_get iommu/tegra-smmu: Expand mutex protection range drivers/iommu/tegra-smmu.c | 53 ++ 1 file changed, 25 insertions(+), 28 deletions(-) -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range
On Tue, Sep 29, 2020 at 07:03:36AM +0100, Christoph Hellwig wrote: > On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > > This is used to protect potential race condition at use_count. > > since probes of client drivers, calling attach_dev(), may run > > concurrently. > > Shouldn't this read "expand" instead of "expend"? Oops...my poor English :) Fixing Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 2/3] iommu: Account for dma_mask and iommu aperture in IOVA reserved regions
On Mon, Sep 28, 2020 at 09:50:36PM +0200, Eric Auger wrote:
> VFIO currently exposes the usable IOVA regions through the
> VFIO_IOMMU_GET_INFO ioctl. However it fails to take into account
> the dma_mask of the devices within the container. The top limit
> currently is defined by the iommu aperture.
Can we take a step back here? The dma_mask only has a meaning for
the DMA API, and not the iommu API, it should have no relevance here.
More importantly if we are using vfio no dma_mask should be set to
start with.
> + if (geo.aperture_end < ULLONG_MAX && geo.aperture_end !=
> geo.aperture_start) {
Please avoid pointlessly overlong lines.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/tegra-smmu: Expend mutex protection range
On Mon, Sep 28, 2020 at 09:52:47PM -0700, Nicolin Chen wrote: > This is used to protect potential race condition at use_count. > since probes of client drivers, calling attach_dev(), may run > concurrently. Shouldn't this read "expand" instead of "expend"? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
