Re: [RFC/PATCH 2/7] iommu-api: Add map_range/unmap_range functions
On 7/11/2014 3:20 AM, Joerg Roedel wrote:
> On Mon, Jun 30, 2014 at 09:51:51AM -0700, Olav Haugan wrote:
>> +int iommu_map_range(struct iommu_domain *domain, unsigned int iova,
>> +struct scatterlist *sg, unsigned int len, int prot)
>> +{
>> +if (unlikely(domain->ops->map_range == NULL))
>> +return -ENODEV;
>> +
>> +BUG_ON(iova & (~PAGE_MASK));
>> +
>> +return domain->ops->map_range(domain, iova, sg, len, prot);
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_map_range);
>> +
>> +int iommu_unmap_range(struct iommu_domain *domain, unsigned int iova,
>> + unsigned int len)
>> +{
>> +if (unlikely(domain->ops->unmap_range == NULL))
>> +return -ENODEV;
>> +
>> +BUG_ON(iova & (~PAGE_MASK));
>> +
>> +return domain->ops->unmap_range(domain, iova, len);
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_unmap_range);
>
> Before introducing these new API functions there should be a fall-back
> for IOMMU drivers that do (not yet) implement the map_range and
> unmap_range call-backs.
>
> The last thing we want is this kind of functional partitioning between
> different IOMMU drivers.
Yes, I can definitely add a fallback instead of returning -ENODEV.
Thanks,
Olav
--
The 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: [RFC PATCHv2 0/3] Support for nesting IOMMUs in VFIO
On Thu, Jul 10, 2014 at 05:14:05PM +0100, Alex Williamson wrote: > On Thu, 2014-07-10 at 16:45 +0100, Will Deacon wrote: > > On Thu, Jul 10, 2014 at 04:38:18PM +0100, Alex Williamson wrote: > > > On Thu, 2014-07-10 at 12:34 +0100, Will Deacon wrote: > > > > Do you think that returning something like -EOPNOTSUPP from an attach is > > > > sufficient for userspace to figure out what's gone wrong? > > > > > > It seems like this would all be a little easier if we had some sort of > > > bus_type iterator, then when we want to see if nesting is supported we'd > > > iterate the bus_types, test iommu_present(), iommu_domain_alloc(), > > > iommu_domain_set_attr(), iommu_domain_free(). We'd only allow a nesting > > > domain to be set if the IOMMU driver for at least one bus_type supports > > > it. Then I think enforcing it would be a little more obvious to > > > userspace. There is a bus_kset, but we'd need an iterator or at least > > > to enable find_bus() and have type1 walk a list of bus names known to > > > possibly support nesting (uglier, but functional). Thanks, > > > > That would work fine if we only had one IOMMU instance per bus. The problem > > is, the ARM SMMU driver (at least) can handle multiple SMMU instances on a > > single bus, only some of which may be capable of nesting. Until we've done > > an attach, it's impossible to know what the capabilities are (since the > > attach is what binds the domain to a physical IOMMU). > > > > So you'd actually need to iterate the bus_type, test iommu_present(), > > iommu_domain_alloc(), iommu_domain_attach(), iommu_domain_free(). The > > attach, of course, then requires devices which means you ultimately end up > > rolling VFIO_SET_IOMMU and VFIO_SET_CONTAINER into a single operation. I > > don't think that helps the group abstraction much! > > I don't really see how that diminishes the value. It still means that > it's only possible to set the IOMMU type of a container to nesting if > it's possible to support a nesting IOMMU. The user would get an error > trying to attach any group to the container that can't be supported as > nesting, whether that's behind the wrong bus_type or behind the wrong > IOMMU in the correct bus_type. The iommu-sysfs extensions that recently > went into the iommu next branch would help to expose this kind of > information to the user. > > It seems like an improvement versus statically advertising support for > something that actually cannot be enabled. Thanks, Ok, I suppose it's an improvement. Given that we don't have a bus_type iterator, would you object if I only do this for the pci_bus_type for now (i.e. the only bus that's VFIO currently supports). Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4] devicetree: Add generic IOMMU device tree bindings
On Mon, Jul 14, 2014 at 2:24 AM, Thierry Reding
wrote:
> On Sat, Jul 12, 2014 at 08:57:31AM -0400, Rob Clark wrote:
>> On Sat, Jul 12, 2014 at 8:22 AM, Arnd Bergmann wrote:
> [...]
>> > The way that Thierry's binding does that is the obvious solution to this,
>> > and it mirrors what we do in practically every other subsystem. I
>> > definitely
>> > want the SMMU to change before anybody starts using it in a real system,
>> > which we fortunately do not have yet.
>>
>> hmm, well if some of the things I need for (like this or batching
>> mappings) are too weird and gpu specific, I'm willing to duplicate the
>> IOMMU driver in drm/msm. It really isn't so much code, and that gives
>> me a lot more more flexibility to do crazy things... at some point I'm
>> probably going to want to do context switches by banging the IOMMU
>> registers directly from the gpu.
>
> If the IOMMU API doesn't provide for what you need, then perhaps it's
> time to enhance it? We do that all the time in other parts of the
> kernel, why should IOMMU be special?
sure.. and my comment was also about the map/unmap batching.
Bypassing IOMMU wouldn't be my first choice. (Especially because I'd
then get to implement it twice.) But if some of the things I need are
too specific to one driver (or worse, problematic for other IOMMU
use-cases which I don't know about), then it is an option I'd be
willing to consider. If nothing else, it would get me out of
allocating sglists for every buffer.. I wonder how much memory
scatterlists take up for 500M of gfx buffers?
> It seems to me like context switching for per-process address space
> isolation is one of the important features of an IOMMU. If the current
> API doesn't let you do that then we should think of ways how it can be
> improved. And if it doesn't do it fast enough, then we should equally
> find ways to speed it up.
>
> This is part of why I think it would be good to have explicit objects
> associated with IOMMU contexts. That would give us a good place to add
> caching for this kind of situation. Currently we're required to handle
> most of this in drivers (map from struct device to context, ...).
well, it is at least awkward that the current api conflates attaching
device and attaching context. I think we could get some use out of an
iommu_swap() API which conceptually acts as:
iommu_swap(olddomain, newdomain, dev)
{
iommu_detach_device(olddomain, dev);
iommu_attach_device(newdomain, dev);
}
BR,
-R
> Thierry
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4] devicetree: Add generic IOMMU device tree bindings
Hi Thierry, On Mon, Jul 14, 2014 at 07:44:53AM +0100, Thierry Reding wrote: > On Sun, Jul 13, 2014 at 10:43:41AM +0100, Will Deacon wrote: > > My plan for the ARM SMMU driver is: > > > > (1) Change ->probe() to walk the device-tree looking for all masters with > > phandles back to the SMMU instance being probed > > You and Rob mentioned this several times and I don't understand why the > SMMU needs to know all masters up front. Is this necessary because it > needs to program all registers at .probe() time and they can't be > reprogrammed subsequently? Or is this just some kind of optimization? It's an optimization to reduce resource usage in the IOMMU, but one that is *required* by certain platforms (e.g. Olav mentioned his 43-ID master and Calxeda had something similar). Basically, in order to perform an ->attach() for a master to a domain, we need complete knowledge of the system so that we can avoid accidentally attaching other masters to the same domain. The programming is done using a form of StreamID wildcarding, so at this point we would need to have parsed the entire DT to ensure our wildcard doesn't match other masters. > > (2) For each master, extract the Stream IDs and add them to the internal > > SMMU driver data structures (an rbtree per SMMU instance). For > > hotpluggable buses, we'll need a way for the bus controller to > > reserve a range of IDs -- this will likely be a later extension to > > the binding. > > > > (3) When we get an ->add() call, warn if it's a device we haven't seen > > and reject the addition. > > It seems to me like this would be the logical place to parse stream IDs. We could do that only if we were guaranteed to have an ->add() call for *every* master before an ->attach() call for *any* master. I don't think that is necessarily true. > You could for example have a case where some device tree contains a node > for which no driver will ever be loaded (for example because it hasn't > been built-in, or the device is never used and the module is therefore > never loaded). That's a situation that you cannot determine by simply > walking the device tree in the IOMMU's .probe(). Why not? If we're simply searching for phandles to the IOMMU, why does it matter whether a driver is bound to the master? > I've always thought about IOMMU masters much in the same way as other > types of resources, such as memory or interrupts. In the rest of the > kernel we do carefully try to postpone allocation of these resources > until they are required, specifically so we don't waste resources when > they're unused. See above, they are all required the moment anybody tries an ->attach(). > That's also one of the reasons why I think associating an IOMMU with the > bus type is bad. Currently if an IOMMU driver thinks it should enable > translation for a given device, then there's no way for that device's > driver to opt out again. There may be reasons (performance, hardware > bugs, ...) for the driver to decide against using the IOMMU for > translation, but there's currently no way to do that if the IOMMU driver > disagrees. Yes, we need a way to associate an IOMMU with a bus instance, but I think that's a separate topic, no? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/10] iommu/ipmmu-vmsa: Refactor micro-TLB lookup
Hi Laurent,
On 7/14/2014 6:01 PM, Laurent Pinchart wrote:
> On Monday 14 July 2014 09:19:23 Khiem Nguyen wrote:
>> On 7/10/2014 7:37 PM, Laurent Pinchart wrote:
>>> On Thursday 10 July 2014 09:03:26 Khiem Nguyen wrote:
On 5/15/2014 7:40 PM, Laurent Pinchart wrote:
> Cache the micro-TLB number in archdata allocated in the .add_device
> handler instead of looking it up when the deviced is attached and
> detached. This simplifies the .attach_dev and .detach_dev operations and
> prepares for DT support.
[snip]
> Signed-off-by: Laurent Pinchart
>
[snip]
> +static int ipmmu_find_utlb(struct ipmmu_vmsa_device *mmu, struct device
> *dev)
> +{
> + const struct ipmmu_vmsa_master *master = mmu->pdata->masters;
> + const char *devname = dev_name(dev);
> + unsigned int i;
> +
> + for (i = 0; i < mmu->pdata->num_masters; ++i, ++master) {
> + if (strcmp(master->name, devname) == 0)
> + return master->utlb;
> + }
> +
> + return -1;
> +}
[snip]
> static int ipmmu_add_device(struct device *dev)
[snip]
> list_for_each_entry(mmu, &ipmmu_devices, list) {
> - master = ipmmu_find_master(mmu, dev);
> - if (master) {
> + utlb = ipmmu_find_utlb(mmu, dev);
> + if (utlb >= 0) {
> /*
> - * TODO Take a reference to the master to protect
> + * TODO Take a reference to the MMU to protect
>* against device removal.
>*/
> break;
[snip]
> + archdata->mmu = mmu;
> + archdata->utlb = utlb;
[snip]
I have one question for ipmmu_add_device().
In my understanding, your code will find utlb for device
base on device name.
For any device, it will /only/ return utlb number of first match.
How about the case that a device name connected with more than 1 utlb ?
e.g DU device (rcar-du-r8a7790) in Lager
Was that case already covered in your code ?
>>>
>>> For the DU case, the R8A7790 contains /two DU devices/, each connected to
>>> a single utlb. The IPMMU driver will thus work fine in that case.
>>
>> As my understanding, in board-lager-reference.c,
>> DU devices are registered with 1 device name "rcar-du-r8a7790".
>>
>> I also added some logs in ipmmu_add_device() to observe
>> the mapping between device name and utlb number.
>> And I saw that only one utlb is mapped with DU.
>>
>> Do I miss anything here ?
>
> No, you're right, my bad. This is definitely a limitation of the driver at
> the
> moment.
OK. Thanks for your confirmation.
>
>>> I agree that this is a problem in general though, other devices (such as
>>> the DMAC) are connected to more than one utlb. This is currently not
>>> supported by the driver.
>>
>> Thanks for your confirmation.
>> I guess the driver will be improved in near future.
>
> Yes, it will.
>
I'm happy to wait for your update. :)
--
Best regards,
KHIEM Nguyen
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 01/10] iommu/ipmmu-vmsa: Refactor micro-TLB lookup
Hi Khiem,
On Monday 14 July 2014 09:19:23 Khiem Nguyen wrote:
> On 7/10/2014 7:37 PM, Laurent Pinchart wrote:
> > On Thursday 10 July 2014 09:03:26 Khiem Nguyen wrote:
> >> On 5/15/2014 7:40 PM, Laurent Pinchart wrote:
> >>> Cache the micro-TLB number in archdata allocated in the .add_device
> >>> handler instead of looking it up when the deviced is attached and
> >>> detached. This simplifies the .attach_dev and .detach_dev operations and
> >>> prepares for DT support.
> >>
> >> [snip]
> >>
> >>> Signed-off-by: Laurent Pinchart
> >>>
> >>
> >> [snip]
> >>
> >>> +static int ipmmu_find_utlb(struct ipmmu_vmsa_device *mmu, struct device
> >>> *dev)
> >>> +{
> >>> + const struct ipmmu_vmsa_master *master = mmu->pdata->masters;
> >>> + const char *devname = dev_name(dev);
> >>> + unsigned int i;
> >>> +
> >>> + for (i = 0; i < mmu->pdata->num_masters; ++i, ++master) {
> >>> + if (strcmp(master->name, devname) == 0)
> >>> + return master->utlb;
> >>> + }
> >>> +
> >>> + return -1;
> >>> +}
> >>
> >> [snip]
> >>
> >>> static int ipmmu_add_device(struct device *dev)
> >>
> >> [snip]
> >>
> >>> list_for_each_entry(mmu, &ipmmu_devices, list) {
> >>> - master = ipmmu_find_master(mmu, dev);
> >>> - if (master) {
> >>> + utlb = ipmmu_find_utlb(mmu, dev);
> >>> + if (utlb >= 0) {
> >>> /*
> >>> - * TODO Take a reference to the master to protect
> >>> + * TODO Take a reference to the MMU to protect
> >>>* against device removal.
> >>>*/
> >>> break;
> >>
> >> [snip]
> >>
> >>> + archdata->mmu = mmu;
> >>> + archdata->utlb = utlb;
> >>
> >> [snip]
> >>
> >> I have one question for ipmmu_add_device().
> >>
> >> In my understanding, your code will find utlb for device
> >> base on device name.
> >> For any device, it will /only/ return utlb number of first match.
> >>
> >> How about the case that a device name connected with more than 1 utlb ?
> >> e.g DU device (rcar-du-r8a7790) in Lager
> >>
> >> Was that case already covered in your code ?
> >
> > For the DU case, the R8A7790 contains /two DU devices/, each connected to
> > a single utlb. The IPMMU driver will thus work fine in that case.
>
> As my understanding, in board-lager-reference.c,
> DU devices are registered with 1 device name "rcar-du-r8a7790".
>
> I also added some logs in ipmmu_add_device() to observe
> the mapping between device name and utlb number.
> And I saw that only one utlb is mapped with DU.
>
> Do I miss anything here ?
No, you're right, my bad. This is definitely a limitation of the driver at the
moment.
> > I agree that this is a problem in general though, other devices (such as
> > the DMAC) are connected to more than one utlb. This is currently not
> > supported by the driver.
>
> Thanks for your confirmation.
> I guess the driver will be improved in near future.
Yes, it will.
--
Regards,
Laurent Pinchart
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
