Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 06/21/2013 12:55 AM, Alex Williamson wrote: > On Thu, 2013-06-20 at 18:48 +1000, Alexey Kardashevskiy wrote: >> On 06/20/2013 05:47 PM, Benjamin Herrenschmidt wrote: >>> On Thu, 2013-06-20 at 15:28 +1000, David Gibson wrote: > Just out of curiosity - would not get_file() and fput_atomic() on a group's > file* do the right job instead of vfio_group_add_external_user() and > vfio_group_del_external_user()? I was thinking that too. Grabbing a file reference would certainly be the usual way of handling this sort of thing. >>> >>> But that wouldn't prevent the group ownership to be returned to >>> the kernel or another user would it ? >> >> >> Holding the file pointer does not let the group->container_users counter go >> to zero > > How so? Holding the file pointer means the file won't go away, which > means the group release function won't be called. That means the group > won't go away, but that doesn't mean it's attached to an IOMMU. A user > could call UNSET_CONTAINER. > >> and this is exactly what vfio_group_add_external_user() and >> vfio_group_del_external_user() do. The difference is only in absolute value >> - 2 vs. 3. >> >> No change in behaviour whether I use new vfio API or simply hold file* till >> KVM closes fd created when IOMMU was connected to LIOBN. > > By that notion you could open(/dev/vfio/$GROUP) and you're safe, right? > But what about SET_CONTAINER & SET_IOMMU? All that you guarantee > holding the file pointer is that the vfio_group exists. > >> And while this counter is not zero, QEMU cannot take ownership over the >> group. >> >> I am definitely still missing the bigger picture... > > The bigger picture is that the group needs to exist AND it needs to be > setup and maintained to have IOMMU protection. Actually, my first stab > at add_external_user doesn't look sufficient, it needs to look more like > vfio_group_get_device_fd, checking group->container->iommu and > group_viable(). This makes sense. If you did this, that would be great. Without it, I really cannot see how the proposed inc/dec of container_users is better than simple holding file*. Thanks. > As written it would allow an external user after > SET_CONTAINER without SET_IOMMU. It should also be part of the API that > the external user must hold the file reference between add_external_use > and del_external_user and do cleanup on any exit paths. Thanks, -- Alexey ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] ADB: Replace __WAITQUEUE_INITIALIZER with more standard DECLARE_WAITQUEUE.
Signed-off-by: Robert P. J. Day --- just FYI, that invocation of __WAITQUEUE_INITIALIER() is the only one in the entire current source tree so it seems a bit out of place. not tested, but there *should* be no functional change. diff --git a/drivers/macintosh/adb.c b/drivers/macintosh/adb.c index b026896..04a5049 100644 --- a/drivers/macintosh/adb.c +++ b/drivers/macintosh/adb.c @@ -697,7 +697,7 @@ static ssize_t adb_read(struct file *file, char __user *buf, int ret = 0; struct adbdev_state *state = file->private_data; struct adb_request *req; - wait_queue_t wait = __WAITQUEUE_INITIALIZER(wait,current); + DECLARE_WAITQUEUE(wait,current); unsigned long flags; if (count < 2) rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On Thu, Jun 20, 2013 at 08:55:13AM -0600, Alex Williamson wrote: > On Thu, 2013-06-20 at 18:48 +1000, Alexey Kardashevskiy wrote: > > On 06/20/2013 05:47 PM, Benjamin Herrenschmidt wrote: > > > On Thu, 2013-06-20 at 15:28 +1000, David Gibson wrote: > > >>> Just out of curiosity - would not get_file() and fput_atomic() on a > > >> group's > > >>> file* do the right job instead of vfio_group_add_external_user() and > > >>> vfio_group_del_external_user()? > > >> > > >> I was thinking that too. Grabbing a file reference would certainly be > > >> the usual way of handling this sort of thing. > > > > > > But that wouldn't prevent the group ownership to be returned to > > > the kernel or another user would it ? > > > > > > Holding the file pointer does not let the group->container_users counter go > > to zero > > How so? Holding the file pointer means the file won't go away, which > means the group release function won't be called. That means the group > won't go away, but that doesn't mean it's attached to an IOMMU. A user > could call UNSET_CONTAINER. Uhh... *thinks*. Ah, I see. I think the interface should not take the group fd, but the container fd. Holding a reference to *that* would keep the necessary things around. But more to the point, it's the right thing semantically: The container is essentially the handle on a host iommu address space, and so that's what should be bound by the KVM call to a particular guest iommu address space. e.g. it would make no sense to bind two different groups to different guest iommu address spaces, if they were in the same container - the guest thinks they are different spaces, but if they're in the same container they must be the same space. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson pgprV1_klnAd2.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On Sat, 2013-06-22 at 22:03 +1000, David Gibson wrote: > On Thu, Jun 20, 2013 at 08:55:13AM -0600, Alex Williamson wrote: > > On Thu, 2013-06-20 at 18:48 +1000, Alexey Kardashevskiy wrote: > > > On 06/20/2013 05:47 PM, Benjamin Herrenschmidt wrote: > > > > On Thu, 2013-06-20 at 15:28 +1000, David Gibson wrote: > > > >>> Just out of curiosity - would not get_file() and fput_atomic() on a > > > >> group's > > > >>> file* do the right job instead of vfio_group_add_external_user() and > > > >>> vfio_group_del_external_user()? > > > >> > > > >> I was thinking that too. Grabbing a file reference would certainly be > > > >> the usual way of handling this sort of thing. > > > > > > > > But that wouldn't prevent the group ownership to be returned to > > > > the kernel or another user would it ? > > > > > > > > > Holding the file pointer does not let the group->container_users counter > > > go > > > to zero > > > > How so? Holding the file pointer means the file won't go away, which > > means the group release function won't be called. That means the group > > won't go away, but that doesn't mean it's attached to an IOMMU. A user > > could call UNSET_CONTAINER. > > Uhh... *thinks*. Ah, I see. > > I think the interface should not take the group fd, but the container > fd. Holding a reference to *that* would keep the necessary things > around. But more to the point, it's the right thing semantically: > > The container is essentially the handle on a host iommu address space, > and so that's what should be bound by the KVM call to a particular > guest iommu address space. e.g. it would make no sense to bind two > different groups to different guest iommu address spaces, if they were > in the same container - the guest thinks they are different spaces, > but if they're in the same container they must be the same space. While the container is the gateway to the iommu, what empowers the container to maintain an iommu is the group. What happens to a container when all the groups are disconnected or closed? Groups are the unit that indicates hardware access, not containers. Thanks, Alex ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On Sat, 2013-06-22 at 22:03 +1000, David Gibson wrote: > I think the interface should not take the group fd, but the container > fd. Holding a reference to *that* would keep the necessary things > around. But more to the point, it's the right thing semantically: > > The container is essentially the handle on a host iommu address space, > and so that's what should be bound by the KVM call to a particular > guest iommu address space. e.g. it would make no sense to bind two > different groups to different guest iommu address spaces, if they were > in the same container - the guest thinks they are different spaces, > but if they're in the same container they must be the same space. Interestingly, how are we going to extend that when/if we implement DDW ? DDW means an API by which the guest can request the creation of additional iommus for a given device (typically, in addition to the default smallish 32-bit one using 4k pages, the guest can request a larger window in 64-bit space using a larger page size). Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Inbound PCI and Memory Corruption
On Fri, 2013-06-21 at 10:14 -0700, Peter LaDow wrote: > After a (finally!) successful search of the list archive, I did find > this: > > http://web.archiveorange.com/archive/v/9IQA26gPvdf4foaTcmCV > > Which seems very related to my problem. However, the patch that is > ultimately referenced is in place in 3.0.80 (see > https://lists.ozlabs.org/pipermail/linuxppc-dev/2006-February/021267.html). > > Hmmm... perhaps our FDT is bad? Afaik e300 is slightly out of order, maybe it's missing a memory barrier somewhere One thing to try is to add some to the dma_map/unmap ops. Also audit the driver to ensure that it properly uses barriers when populating descriptors (and maybe compare to a more recent version of the driver upstream). Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
