[PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

When a VM is terminated, the VFIO driver detaches all pass-through
devices from VFIO domain by clearing domain id and page table root
pointer from each device table entry (DTE), and then invalidates
the DTE. Then, the VFIO driver unmap pages and invalidate IOMMU pages.

Currently, the IOMMU driver keeps track of which IOMMU and how many
devices are attached to the domain. When invalidate IOMMU pages,
the driver checks if the IOMMU is still attached to the domain before
issuing the invalidate page command.

However, since VFIO has already detached all devices from the domain,
the subsequent INVALIDATE_IOMMU_PAGES commands are being skipped as
there is no IOMMU attached to the domain. This results in data
corruption and could cause the PCI device to end up in indeterministic
state.

Fix this by always issuing the IOMMU pages invalidate command when
device count is zero, which is the case when detaching all the devices
from the domain.

Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd_iommu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 525659b88ade..ab31ba75da1b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1248,7 +1248,13 @@ static void __domain_flush_pages(struct protection_domain *domain,
 	build_inv_iommu_pages(&cmd, address, size, domain->id, pde);
 
 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
-		if (!domain->dev_iommu[i])
+		/*
+		 * The dev_cnt is zero when all devices are detached
+		 * from the domain. This is the case when VFIO detaches
+		 * all devices from the group before flushing IOMMU pages.
+		 * So, always issue the flush command.
+		 */
+		if (domain->dev_cnt && !domain->dev_iommu[i])
 			continue;
 
 		/*
-- 
2.17.1

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

On Wed, Jan 16, 2019 at 04:16:25AM +0000, Suthikulpanit, Suravee wrote:
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 525659b88ade..ab31ba75da1b 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1248,7 +1248,13 @@ static void __domain_flush_pages(struct protection_domain *domain,
>  	build_inv_iommu_pages(&cmd, address, size, domain->id, pde);
>  
>  	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
> -		if (!domain->dev_iommu[i])
> +		/*
> +		 * The dev_cnt is zero when all devices are detached
> +		 * from the domain. This is the case when VFIO detaches
> +		 * all devices from the group before flushing IOMMU pages.
> +		 * So, always issue the flush command.
> +		 */
> +		if (domain->dev_cnt && !domain->dev_iommu[i])
>  			continue;

This doesn't look like the correct fix. We still miss the flush if we
detach the last device from the domain. How about the attached diff? If
I understand the problem correctly, it should fix the problem more
reliably.

Thanks,

	Joerg

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 87ba23a75b38..dc1e2a8a19d7 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
 
 static void do_detach(struct iommu_dev_data *dev_data)
 {
+	struct protection_domain *domain = dev_data->domain;
 	struct amd_iommu *iommu;
 	u16 alias;
 
 	iommu = amd_iommu_rlookup_table[dev_data->devid];
 	alias = dev_data->alias;
 
-	/* decrease reference counters */
-	dev_data->domain->dev_iommu[iommu->index] -= 1;
-	dev_data->domain->dev_cnt                 -= 1;
-
 	/* Update data structures */
 	dev_data->domain = NULL;
 	list_del(&dev_data->list);
-	clear_dte_entry(dev_data->devid);
-	if (alias != dev_data->devid)
-		clear_dte_entry(alias);
 
+	clear_dte_entry(dev_data->devid);
 	/* Flush the DTE entry */
 	device_flush_dte(dev_data);
+
+	if (alias != dev_data->devid) {
+		clear_dte_entry(alias);
+		/* Flush the Alias DTE entry */
+		device_flush_dte(alias);
+	}
+
+	/* Flush IOTLB */
+	domain_flush_tlb_pde(domain);
+
+	/* Wait for the flushes to finish */
+	domain_flush_complete(domain);
+
+	/* decrease reference counters - needs to happen after the flushes */
+	domain->dev_iommu[iommu->index] -= 1;
+	domain->dev_cnt                 -= 1;
 }
 
 /*

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/16/19 8:26 PM, joro@8bytes.org wrote:
> On Wed, Jan 16, 2019 at 04:16:25AM +0000, Suthikulpanit, Suravee wrote:
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> index 525659b88ade..ab31ba75da1b 100644
>> --- a/drivers/iommu/amd_iommu.c
>> +++ b/drivers/iommu/amd_iommu.c
>> @@ -1248,7 +1248,13 @@ static void __domain_flush_pages(struct protection_domain *domain,
>>   	build_inv_iommu_pages(&cmd, address, size, domain->id, pde);
>>   
>>   	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
>> -		if (!domain->dev_iommu[i])
>> +		/*
>> +		 * The dev_cnt is zero when all devices are detached
>> +		 * from the domain. This is the case when VFIO detaches
>> +		 * all devices from the group before flushing IOMMU pages.
>> +		 * So, always issue the flush command.
>> +		 */
>> +		if (domain->dev_cnt && !domain->dev_iommu[i])
>>   			continue;
> 
> This doesn't look like the correct fix. We still miss the flush if we
> detach the last device from the domain. 

Actually, I am not sure how we would be missing the flush on the last device.
In my test, I am seeing the flush command being issued correctly during
vfio_unmap_unpin(), which is after all devices are detached.
Although, I might be missing your point here. Could you please elaborate?

> How about the attached diff? If
> I understand the problem correctly, it should fix the problem more
> reliably.
> 
> Thanks,
> 
> 	Joerg
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 87ba23a75b38..dc1e2a8a19d7 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
>   
>   static void do_detach(struct iommu_dev_data *dev_data)
>   {
> +	struct protection_domain *domain = dev_data->domain;
>   	struct amd_iommu *iommu;
>   	u16 alias;
>   
>   	iommu = amd_iommu_rlookup_table[dev_data->devid];
>   	alias = dev_data->alias;
>   
> -	/* decrease reference counters */
> -	dev_data->domain->dev_iommu[iommu->index] -= 1;
> -	dev_data->domain->dev_cnt                 -= 1;
> -
>   	/* Update data structures */
>   	dev_data->domain = NULL;
>   	list_del(&dev_data->list);
> -	clear_dte_entry(dev_data->devid);
> -	if (alias != dev_data->devid)
> -		clear_dte_entry(alias);
>   
> +	clear_dte_entry(dev_data->devid);
>   	/* Flush the DTE entry */
>   	device_flush_dte(dev_data);
> +
> +	if (alias != dev_data->devid) {
> +		clear_dte_entry(alias);
> +		/* Flush the Alias DTE entry */
> +		device_flush_dte(alias);
> +	}
> +
> +	/* Flush IOTLB */
> +	domain_flush_tlb_pde(domain);
> +
> +	/* Wait for the flushes to finish */
> +	domain_flush_complete(domain);
> +
> +	/* decrease reference counters - needs to happen after the flushes */
> +	domain->dev_iommu[iommu->index] -= 1;
> +	domain->dev_cnt                 -= 1;
>   }

I have also considered this. This would also work. But since we are already
doing page flushes during page unmapping later on after all devices are detached.
So, would this be necessary? Please see vfio_iommu_type1_detach_group().

Also, if we consider the case where there are more than one devices sharing
the domain. This would issue page flush every time we detach a device,
and while other devices still attached to the domain.

Regards,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

On Wed, Jan 16, 2019 at 02:08:55PM +0000, Suthikulpanit, Suravee wrote:
> Actually, I am not sure how we would be missing the flush on the last device.
> In my test, I am seeing the flush command being issued correctly during
> vfio_unmap_unpin(), which is after all devices are detached.
> Although, I might be missing your point here. Could you please elaborate?

Okay, you are right, the patch effectivly adds an unconditional flush of
the domain on all iommus when the last device is detached. So it is
correct in that regard. But that code path is also used in the
iommu_unmap() path.

The problem now is, that with your change we send flush commands to all
IOMMUs in the unmap path when no device is attached to the domain.
This will hurt performance there, no?

Regards,

	Joerg

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/17/19 12:08 AM, joro@8bytes.org wrote:
> On Wed, Jan 16, 2019 at 02:08:55PM +0000, Suthikulpanit, Suravee wrote:
>> Actually, I am not sure how we would be missing the flush on the last device.
>> In my test, I am seeing the flush command being issued correctly during
>> vfio_unmap_unpin(), which is after all devices are detached.
>> Although, I might be missing your point here. Could you please elaborate?
> 
> Okay, you are right, the patch effectivly adds an unconditional flush of
> the domain on all iommus when the last device is detached. So it is
> correct in that regard. But that code path is also used in the
> iommu_unmap() path.
> 
> The problem now is, that with your change we send flush commands to all
> IOMMUs in the unmap path when no device is attached to the domain.
> This will hurt performance there, no?
> 
> Regards,
> 
> 	Joerg
> 

Sounds like we need a way to track state of each IOMMU for a domain.
What if we define the following:

   enum IOMMU_DOMAIN_STATES {
     DOMAIN_FREE = -1,
     DOMAIN_DETACHED = 0,
     DOMAIN_ATTACHED >= 1
   }

We should be able to use the dev_iommu[] to help track the state.
     - In amd_iommu_domain_alloc, we initialize the array to DOMAIN_FREE
     - In do_attach(), we change to DOMAIN_ATTACH or we can increment the count
       if it is already in DOMAIN_ATTACH state.
     - In do_detach(). we change to DOMAIN_DETACH.

Then, in __domain_flush_pages, we issue command when the dev_iommu[] >= 0.
This should preserve previous behavior, and only add flushing condition to
the specific IOMMU in detached state. Please let me know what you think.

Regards,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/17/19 3:44 PM, Suravee Suthikulpanit wrote:
> Joerg,
> 
> On 1/17/19 12:08 AM, joro@8bytes.org wrote:
>> On Wed, Jan 16, 2019 at 02:08:55PM +0000, Suthikulpanit, Suravee wrote:
>>> Actually, I am not sure how we would be missing the flush on the last device.
>>> In my test, I am seeing the flush command being issued correctly during
>>> vfio_unmap_unpin(), which is after all devices are detached.
>>> Although, I might be missing your point here. Could you please elaborate?
>>
>> Okay, you are right, the patch effectivly adds an unconditional flush of
>> the domain on all iommus when the last device is detached. So it is
>> correct in that regard. But that code path is also used in the
>> iommu_unmap() path.
>>
>> The problem now is, that with your change we send flush commands to all
>> IOMMUs in the unmap path when no device is attached to the domain.
>> This will hurt performance there, no?
>>
>> Regards,
>>
>>     Joerg
>>
> 
> Sounds like we need a way to track state of each IOMMU for a domain.
> What if we define the following:
> 
>    enum IOMMU_DOMAIN_STATES {
>      DOMAIN_FREE = -1,
>      DOMAIN_DETACHED = 0,
>      DOMAIN_ATTACHED >= 1
>    }
> 
> We should be able to use the dev_iommu[] to help track the state.
>      - In amd_iommu_domain_alloc, we initialize the array to DOMAIN_FREE
>      - In do_attach(), we change to DOMAIN_ATTACH or we can increment the count
>        if it is already in DOMAIN_ATTACH state.
>      - In do_detach(). we change to DOMAIN_DETACH.
> 
> Then, in __domain_flush_pages, we issue command when the dev_iommu[] >= 0.
> This should preserve previous behavior, and only add flushing condition to
> the specific IOMMU in detached state. Please let me know what you think.
> 
> Regards,
> Suravee

By the way, I just sent V2 of this patch since it might be more clear.

Regards,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

Hi Suravee,

On Thu, Jan 17, 2019 at 08:44:36AM +0000, Suthikulpanit, Suravee wrote:
> Then, in __domain_flush_pages, we issue command when the dev_iommu[] >= 0.
> This should preserve previous behavior, and only add flushing condition to
> the specific IOMMU in detached state. Please let me know what you think.

I think the whole point why VFIO is detaching all devices first and then
goes into unmapping pages is that there are no flushes needed anymore
when devices are detached.

But when we follow your proposal we would still do IOTLB flushes even
when no devices are attached anymore. So I'd like to avoid this, given
the implications on unmapping performance. We should just flush the
IOMMU TLB at detach time and be done with it.

Makes sense?

Regards,

	Joerg

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/22/19 5:44 PM, joro@8bytes.org wrote:
> Hi Suravee,
> 
> On Thu, Jan 17, 2019 at 08:44:36AM +0000, Suthikulpanit, Suravee wrote:
>> Then, in __domain_flush_pages, we issue command when the dev_iommu[] >= 0.
>> This should preserve previous behavior, and only add flushing condition to
>> the specific IOMMU in detached state. Please let me know what you think.
> 
> I think the whole point why VFIO is detaching all devices first and then
> goes into unmapping pages is that there are no flushes needed anymore
> when devices are detached.
> 
> But when we follow your proposal we would still do IOTLB flushes even
> when no devices are attached anymore. So I'd like to avoid this, given
> the implications on unmapping performance. We should just flush the
> IOMMU TLB at detach time and be done with it.
> 
> Makes sense?
> 
> Regards,
> 
> 	Joerg
> 

Thanks for the detail. Alright then, let's just go with the version you
sent on 1/16/19. Do you want me to resend V3 with that changes, or
would you be taking care of that?

Thanks,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

Hi Suravee,

On Tue, Jan 22, 2019 at 03:53:18PM +0000, Suthikulpanit, Suravee wrote:
> Thanks for the detail. Alright then, let's just go with the version you
> sent on 1/16/19. Do you want me to resend V3 with that changes, or
> would you be taking care of that?

Please send me a v3 based on the diff I sent last week. Also add a
separate patch which adds the missing dte flush for the alias entry.

Thanks,

	Joerg

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/23/19 2:56 PM, joro@8bytes.org wrote:
> Hi Suravee,
> 
> On Tue, Jan 22, 2019 at 03:53:18PM +0000, Suthikulpanit, Suravee wrote:
>> Thanks for the detail. Alright then, let's just go with the version you
>> sent on 1/16/19. Do you want me to resend V3 with that changes, or
>> would you be taking care of that?
> 
> Please send me a v3 based on the diff I sent last week. Also add a
> separate patch which adds the missing dte flush for the alias entry.
> 
> Thanks,
> 
> 	Joerg
> 

Actually, I just noticed that device_flush_dte() has already handled flushing the DTE
for alias device as well. Please see the link below.

https://elixir.bootlin.com/linux/latest/source/drivers/iommu/amd_iommu.c#L1219

So, we don't need to call device_flush_dte() for alias device in do_detach().

Regards,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

Hi Suravee,

On Thu, Jan 24, 2019 at 03:25:19AM +0000, Suthikulpanit, Suravee wrote:
> Actually, I just noticed that device_flush_dte() has already handled flushing the DTE
> for alias device as well. Please see the link below.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/iommu/amd_iommu.c#L1219
> 
> So, we don't need to call device_flush_dte() for alias device in do_detach().

You are right, I missed that.

Thanks,

	Joerg

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[Suthikulpanit, Suravee]

Joerg,

On 1/16/19 8:26 PM, joro@8bytes.org wrote:
> How about the attached diff? If
> I understand the problem correctly, it should fix the problem more
> reliably.
> 
> Thanks,
> 
> 	Joerg
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 87ba23a75b38..dc1e2a8a19d7 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -1991,25 +1991,36 @@ static void do_attach(struct iommu_dev_data *dev_data,
>   
>   static void do_detach(struct iommu_dev_data *dev_data)
>   {
> +	struct protection_domain *domain = dev_data->domain;
>   	struct amd_iommu *iommu;
>   	u16 alias;
>   
>   	iommu = amd_iommu_rlookup_table[dev_data->devid];
>   	alias = dev_data->alias;
>   
> -	/* decrease reference counters */
> -	dev_data->domain->dev_iommu[iommu->index] -= 1;
> -	dev_data->domain->dev_cnt                 -= 1;
> -
>   	/* Update data structures */
>   	dev_data->domain = NULL;
>   	list_del(&dev_data->list);
> -	clear_dte_entry(dev_data->devid);
> -	if (alias != dev_data->devid)
> -		clear_dte_entry(alias);
>   
> +	clear_dte_entry(dev_data->devid);
>   	/* Flush the DTE entry */
>   	device_flush_dte(dev_data);
> +
> +	if (alias != dev_data->devid) {
> +		clear_dte_entry(alias);
> +		/* Flush the Alias DTE entry */
> +		device_flush_dte(alias);
> +	}
> +

Actually, device_flush_dte(alias) should be needed regardless of this patch.
Are you planning to add this?

Regards,
Suravee

Re: [PATCH] iommu/amd: Fix IOMMU page flush when detach all devices from a domain

[joro]

On Wed, Jan 16, 2019 at 02:40:59PM +0000, Suthikulpanit, Suravee wrote:
> Actually, device_flush_dte(alias) should be needed regardless of this patch.
> Are you planning to add this?

Yes, I stumbled over this while writing the diff. I'll submit that as a
separate patch.

Thanks,

	Joerg