All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Joerg Roedel <joro@8bytes.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	David Woodhouse <dwmw2@infradead.org>,
	Jiang Liu <jiang.liu@linux.intel.com>,
	iommu@lists.linux-foundation.org, jroedel@suse.de,
	linux-kernel@vger.kernel.org, Myron Stowe <mstowe@redhat.com>
Subject: Re: [PATCH 2/2] iommu/vt-d: Only remove domain when device is removed
Date: Tue, 04 Nov 2014 09:12:17 -0700	[thread overview]
Message-ID: <1415117537.27420.428.camel@ul30vt.home> (raw)
In-Reply-To: <1412074923-6342-3-git-send-email-joro@8bytes.org>

On Tue, 2014-09-30 at 13:02 +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This makes sure any RMRR mappings stay in place when the
> driver is unbound from the device.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/intel-iommu.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 5619f26..eaf825a 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3865,16 +3865,7 @@ static int device_notifier(struct notifier_block *nb,
>  	if (iommu_dummy(dev))
>  		return 0;
>  
> -	if (action != BUS_NOTIFY_UNBOUND_DRIVER &&
> -	    action != BUS_NOTIFY_DEL_DEVICE)
> -		return 0;
> -
> -	/*
> -	 * If the device is still attached to a device driver we can't
> -	 * tear down the domain yet as DMA mappings may still be in use.
> -	 * Wait for the BUS_NOTIFY_UNBOUND_DRIVER event to do that.
> -	 */
> -	if (action == BUS_NOTIFY_DEL_DEVICE && dev->driver != NULL)
> +	if (action != BUS_NOTIFY_REMOVED_DEVICE)

I haven't tested it, but I'm concerned whether this has introduced a
domain leak.  If we think about the case of unbinding a device from a
host driver and attaching it to a domain through the IOMMU API, I think
we used to count on this path to call domain_exit(), which made the
domain_context_mapped() in intel_iommu_attach_device() "unlikely".  With
this change, isn't the test in intel_iommu_attach_device() now neither
likely nor unlikely and we're only removing the dev_info from the domain
and not destroying the domain itself?  Thanks,

Alex


WARNING: multiple messages have this Message-ID (diff)
From: Alex Williamson <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Cc: jroedel-l3A5Bk7waGM@public.gmane.org,
	Greg Kroah-Hartman
	<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	Myron Stowe <mstowe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>,
	Jiang Liu <jiang.liu-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Subject: Re: [PATCH 2/2] iommu/vt-d: Only remove domain when device is removed
Date: Tue, 04 Nov 2014 09:12:17 -0700	[thread overview]
Message-ID: <1415117537.27420.428.camel@ul30vt.home> (raw)
In-Reply-To: <1412074923-6342-3-git-send-email-joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>

On Tue, 2014-09-30 at 13:02 +0200, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> This makes sure any RMRR mappings stay in place when the
> driver is unbound from the device.
> 
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/intel-iommu.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 5619f26..eaf825a 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -3865,16 +3865,7 @@ static int device_notifier(struct notifier_block *nb,
>  	if (iommu_dummy(dev))
>  		return 0;
>  
> -	if (action != BUS_NOTIFY_UNBOUND_DRIVER &&
> -	    action != BUS_NOTIFY_DEL_DEVICE)
> -		return 0;
> -
> -	/*
> -	 * If the device is still attached to a device driver we can't
> -	 * tear down the domain yet as DMA mappings may still be in use.
> -	 * Wait for the BUS_NOTIFY_UNBOUND_DRIVER event to do that.
> -	 */
> -	if (action == BUS_NOTIFY_DEL_DEVICE && dev->driver != NULL)
> +	if (action != BUS_NOTIFY_REMOVED_DEVICE)

I haven't tested it, but I'm concerned whether this has introduced a
domain leak.  If we think about the case of unbinding a device from a
host driver and attaching it to a domain through the IOMMU API, I think
we used to count on this path to call domain_exit(), which made the
domain_context_mapped() in intel_iommu_attach_device() "unlikely".  With
this change, isn't the test in intel_iommu_attach_device() now neither
likely nor unlikely and we're only removing the dev_info from the domain
and not destroying the domain itself?  Thanks,

Alex

  reply	other threads:[~2014-11-04 16:12 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-30 11:02 [PATCH 0/2] iommu/vt-d: Keep RMRR mappings around on driver unbind Joerg Roedel
2014-09-30 11:02 ` Joerg Roedel
2014-09-30 11:02 ` [PATCH 1/2] driver core: Add BUS_NOTIFY_REMOVED_DEVICE event Joerg Roedel
2014-09-30 11:02   ` Joerg Roedel
2014-09-30 11:02 ` [PATCH 2/2] iommu/vt-d: Only remove domain when device is removed Joerg Roedel
2014-09-30 11:02   ` Joerg Roedel
2014-11-04 16:12   ` Alex Williamson [this message]
2014-11-04 16:12     ` Alex Williamson
2014-11-06 12:54     ` Joerg Roedel
2014-11-06 12:54       ` Joerg Roedel
2014-11-06 16:16       ` Alex Williamson
2014-11-06 16:16         ` Alex Williamson
2014-11-06 16:43         ` Alex Williamson
2014-11-06 16:43           ` Alex Williamson
2014-12-09 12:15         ` Joerg Roedel
2014-12-09 12:15           ` Joerg Roedel
2014-12-11 16:35           ` Jerry Hoemann
2014-12-11 16:35             ` Jerry Hoemann
2014-12-12 15:56             ` Joerg Roedel
2014-12-12 15:56               ` Joerg Roedel
2014-10-01 22:35 ` [PATCH 0/2] iommu/vt-d: Keep RMRR mappings around on driver unbind Greg Kroah-Hartman
2014-10-01 22:35   ` Greg Kroah-Hartman
2014-10-02  9:20   ` Joerg Roedel
2014-10-02  9:20     ` Joerg Roedel
2014-10-02  0:30 ` Jerry Hoemann
2014-10-02  0:30   ` Jerry Hoemann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1415117537.27420.428.camel@ul30vt.home \
    --to=alex.williamson@redhat.com \
    --cc=dwmw2@infradead.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jiang.liu@linux.intel.com \
    --cc=joro@8bytes.org \
    --cc=jroedel@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mstowe@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.