[PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[Alexey Kardashevskiy]

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

Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[David Gibson]

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

Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[Alex Williamson]

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);
>  }

Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[Alexey Kardashevskiy]

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

Re: [PATCH kernel] vfio/spapr_tce: Skip unsetting already unset table

[Alex Williamson]

On Wed, 13 Feb 2019 11:18:21 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

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


Applied to vfio next branch with updated commit log and David's R-b.
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);
> >>  }  
> >   
>