LinuxPPC-Dev Archive on lore.kernel.org
 help / Atom feed
* [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table
@ 2019-02-11  7:49 Alexey Kardashevskiy
  2019-02-11 23:44 ` David Gibson
  2019-02-12 20:52 ` Alex Williamson
  0 siblings, 2 replies; 4+ messages in thread
From: Alexey Kardashevskiy @ 2019-02-11  7:49 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: Alexey Kardashevskiy, Alex Williamson, kvm, kvm-ppc, David Gibson

VFIO TCE IOMMU v2 owns IOMMU tables so when detach a IOMMU group from
a container, we need to unset those from a group so we call unset_window()
so do we unconditionally. We also unset tables when removing a DMA window
via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.

The window removal checks if the table actually exists (hidden inside
tce_iommu_find_table()) but the group detaching does not so the user
may see duplicating messages:
pci 0009:03     : [PE# fd] Removing DMA window #0
pci 0009:03     : [PE# fd] Removing DMA window #1
pci 0009:03     : [PE# fd] Removing DMA window #0
pci 0009:03     : [PE# fd] Removing DMA window #1

At the moment this is not a problem as the second invocation
of unset_window() writes zeroes to the HW registers again and exits early
as there is no table.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

When doing VFIO PCI hot unplug, first we remove the DMA window and
set container->tables[num] - this is a first couple of messages.
Then we detach the group and then we see another couple of the same
messages which confused myself.
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index c424913..8dbb270 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -1235,7 +1235,8 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
 	}
 
 	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
-		table_group->ops->unset_window(table_group, i);
+		if (container->tables[i])
+			table_group->ops->unset_window(table_group, i);
 
 	table_group->ops->release_ownership(table_group);
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table
  2019-02-11  7:49 [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table Alexey Kardashevskiy
@ 2019-02-11 23:44 ` David Gibson
  2019-02-12 20:52 ` Alex Williamson
  1 sibling, 0 replies; 4+ messages in thread
From: David Gibson @ 2019-02-11 23:44 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: Alex Williamson, linuxppc-dev, kvm, kvm-ppc

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

On Mon, Feb 11, 2019 at 06:49:17PM +1100, Alexey Kardashevskiy wrote:
> VFIO TCE IOMMU v2 owns IOMMU tables so when detach a IOMMU group from
> a container, we need to unset those from a group so we call unset_window()
> so do we unconditionally. We also unset tables when removing a DMA window
> via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.
> 
> The window removal checks if the table actually exists (hidden inside
> tce_iommu_find_table()) but the group detaching does not so the user
> may see duplicating messages:
> pci 0009:03     : [PE# fd] Removing DMA window #0
> pci 0009:03     : [PE# fd] Removing DMA window #1
> pci 0009:03     : [PE# fd] Removing DMA window #0
> pci 0009:03     : [PE# fd] Removing DMA window #1
> 
> At the moment this is not a problem as the second invocation
> of unset_window() writes zeroes to the HW registers again and exits early
> as there is no table.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> 
> When doing VFIO PCI hot unplug, first we remove the DMA window and
> set container->tables[num] - this is a first couple of messages.
> Then we detach the group and then we see another couple of the same
> messages which confused myself.
> ---
>  drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index c424913..8dbb270 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -1235,7 +1235,8 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>  	}
>  
>  	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
> -		table_group->ops->unset_window(table_group, i);
> +		if (container->tables[i])
> +			table_group->ops->unset_window(table_group, i);
>  
>  	table_group->ops->release_ownership(table_group);
>  }

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table
  2019-02-11  7:49 [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table Alexey Kardashevskiy
  2019-02-11 23:44 ` David Gibson
@ 2019-02-12 20:52 ` Alex Williamson
  2019-02-13  0:18   ` Alexey Kardashevskiy
  1 sibling, 1 reply; 4+ messages in thread
From: Alex Williamson @ 2019-02-12 20:52 UTC (permalink / raw)
  To: Alexey Kardashevskiy; +Cc: linuxppc-dev, kvm, kvm-ppc, David Gibson

On Mon, 11 Feb 2019 18:49:17 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> VFIO TCE IOMMU v2 owns IOMMU tables so when detach a IOMMU group from
> a container, we need to unset those from a group so we call unset_window()
> so do we unconditionally. We also unset tables when removing a DMA window

Patch looks ok, but this first sentence trails off into a bit of a word
salad.  Care to refine a bit?  Thanks,

Alex

> via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.
> 
> The window removal checks if the table actually exists (hidden inside
> tce_iommu_find_table()) but the group detaching does not so the user
> may see duplicating messages:
> pci 0009:03     : [PE# fd] Removing DMA window #0
> pci 0009:03     : [PE# fd] Removing DMA window #1
> pci 0009:03     : [PE# fd] Removing DMA window #0
> pci 0009:03     : [PE# fd] Removing DMA window #1
> 
> At the moment this is not a problem as the second invocation
> of unset_window() writes zeroes to the HW registers again and exits early
> as there is no table.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> When doing VFIO PCI hot unplug, first we remove the DMA window and
> set container->tables[num] - this is a first couple of messages.
> Then we detach the group and then we see another couple of the same
> messages which confused myself.
> ---
>  drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index c424913..8dbb270 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -1235,7 +1235,8 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>  	}
>  
>  	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
> -		table_group->ops->unset_window(table_group, i);
> +		if (container->tables[i])
> +			table_group->ops->unset_window(table_group, i);
>  
>  	table_group->ops->release_ownership(table_group);
>  }


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table
  2019-02-12 20:52 ` Alex Williamson
@ 2019-02-13  0:18   ` Alexey Kardashevskiy
  0 siblings, 0 replies; 4+ messages in thread
From: Alexey Kardashevskiy @ 2019-02-13  0:18 UTC (permalink / raw)
  To: Alex Williamson; +Cc: linuxppc-dev, kvm, kvm-ppc, David Gibson



On 13/02/2019 07:52, Alex Williamson wrote:
> On Mon, 11 Feb 2019 18:49:17 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> VFIO TCE IOMMU v2 owns IOMMU tables so when detach a IOMMU group from
>> a container, we need to unset those from a group so we call unset_window()
>> so do we unconditionally. We also unset tables when removing a DMA window
> 
> Patch looks ok, but this first sentence trails off into a bit of a word
> salad.  Care to refine a bit?  Thanks,

Fair comment, sorry for the salad. How about this?

===
VFIO TCE IOMMU v2 owns IOMMU tables. When we detach an IOMMU group from
a container, we need to unset these tables from the group which we do by
calling unset_window(). We also unset tables when removing a DMA window
via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.
===


> 
> Alex
> 
>> via the VFIO_IOMMU_SPAPR_TCE_REMOVE ioctl.
>>
>> The window removal checks if the table actually exists (hidden inside
>> tce_iommu_find_table()) but the group detaching does not so the user
>> may see duplicating messages:
>> pci 0009:03     : [PE# fd] Removing DMA window #0
>> pci 0009:03     : [PE# fd] Removing DMA window #1
>> pci 0009:03     : [PE# fd] Removing DMA window #0
>> pci 0009:03     : [PE# fd] Removing DMA window #1
>>
>> At the moment this is not a problem as the second invocation
>> of unset_window() writes zeroes to the HW registers again and exits early
>> as there is no table.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>
>> When doing VFIO PCI hot unplug, first we remove the DMA window and
>> set container->tables[num] - this is a first couple of messages.
>> Then we detach the group and then we see another couple of the same
>> messages which confused myself.
>> ---
>>  drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index c424913..8dbb270 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -1235,7 +1235,8 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>>  	}
>>  
>>  	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
>> -		table_group->ops->unset_window(table_group, i);
>> +		if (container->tables[i])
>> +			table_group->ops->unset_window(table_group, i);
>>  
>>  	table_group->ops->release_ownership(table_group);
>>  }
> 

-- 
Alexey

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-11  7:49 [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table Alexey Kardashevskiy
2019-02-11 23:44 ` David Gibson
2019-02-12 20:52 ` Alex Williamson
2019-02-13  0:18   ` Alexey Kardashevskiy

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org linuxppc-dev@archiver.kernel.org
	public-inbox-index linuxppc-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox