2015-02-03 00:15:05

by Alex Williamson

Subject: Re: [PATCH v3 08/24] powerpc/spapr: vfio: Switch from iommu_table to new powerpc_iommu

On Thu, 2015-01-29 at 20:21 +1100, Alexey Kardashevskiy wrote:
> Modern IBM POWERPC systems support multiple (currently two) TCE tables
> per IOMMU group (a.k.a. PE). This adds a powerpc_iommu container
> for TCE tables. Right now just one table is supported.
>
> Signed-off-by: Alexey Kardashevskiy <[email protected]>
> ---
> arch/powerpc/include/asm/iommu.h | 18 ++--
> arch/powerpc/kernel/eeh.c | 2 +-
> arch/powerpc/kernel/iommu.c | 34 ++++----
> arch/powerpc/platforms/powernv/pci-ioda.c | 37 +++++---
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 16 ++--
> arch/powerpc/platforms/powernv/pci.c | 2 +-
> arch/powerpc/platforms/powernv/pci.h | 4 +-
> arch/powerpc/platforms/pseries/iommu.c | 9 +-
> drivers/vfio/vfio_iommu_spapr_tce.c | 131 ++++++++++++++++++++--------
> 9 files changed, 170 insertions(+), 83 deletions(-)
[snip]
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 29d5708..28909e1 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -84,7 +84,7 @@ static void decrement_locked_vm(long npages)
> */
> struct tce_container {
> struct mutex lock;
> - struct iommu_table *tbl;
> + struct iommu_group *grp;
> bool enabled;
> };
>
> @@ -104,16 +104,40 @@ static bool tce_check_page_size(struct page *page, unsigned page_shift)
> return false;
> }
>
> +static struct iommu_table *spapr_tce_find_table(
> + struct tce_container *container,
> + phys_addr_t ioba)
> +{
> + long i;
> + struct iommu_table *ret = NULL;
> + struct powerpc_iommu *iommu = iommu_group_get_iommudata(container->grp);
> +
> + mutex_lock(&container->lock);
> + for (i = 0; i < POWERPC_IOMMU_MAX_TABLES; ++i) {
> + struct iommu_table *tbl = &iommu->tables[i];
> + unsigned long entry = ioba >> tbl->it_page_shift;
> + unsigned long start = tbl->it_offset;
> + unsigned long end = start + tbl->it_size;
> +
> + if ((start <= entry) && (entry < end)) {
> + ret = tbl;
> + break;
> + }
> + }
> + mutex_unlock(&container->lock);
> +
> + return ret;
> +}
> +
> static int tce_iommu_enable(struct tce_container *container)
> {
> int ret = 0;
> + struct powerpc_iommu *iommu;
> + struct iommu_table *tbl;
>
> - if (!container->tbl)
> + if (!container->grp)
> return -ENXIO;
>
> - if (!current->mm)
> - return -ESRCH; /* process exited */
> -
> if (container->enabled)
> return -EBUSY;
>
> @@ -142,7 +166,12 @@ static int tce_iommu_enable(struct tce_container *container)
> * as this information is only available from KVM and VFIO is
> * KVM agnostic.
> */
> - ret = try_increment_locked_vm(IOMMU_TABLE_PAGES(container->tbl));
> + iommu = iommu_group_get_iommudata(container->grp);
> + if (!iommu)
> + return -EFAULT;
> +
> + tbl = &iommu->tables[0];


There should probably be a comment somewhere documenting that tables[0]
is the small window and presumably [1] will be the DDW.

2015-02-04 13:32:32

by Alexander Graf

Subject: Re: [PATCH v3 08/24] powerpc/spapr: vfio: Switch from iommu_table to new powerpc_iommu

On 03.02.15 01:12, Alex Williamson wrote:
> On Thu, 2015-01-29 at 20:21 +1100, Alexey Kardashevskiy wrote:
>> Modern IBM POWERPC systems support multiple (currently two) TCE tables
>> per IOMMU group (a.k.a. PE). This adds a powerpc_iommu container
>> for TCE tables. Right now just one table is supported.
>>
>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>> ---
>> arch/powerpc/include/asm/iommu.h | 18 ++--
>> arch/powerpc/kernel/eeh.c | 2 +-
>> arch/powerpc/kernel/iommu.c | 34 ++++----
>> arch/powerpc/platforms/powernv/pci-ioda.c | 37 +++++---
>> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 16 ++--
>> arch/powerpc/platforms/powernv/pci.c | 2 +-
>> arch/powerpc/platforms/powernv/pci.h | 4 +-
>> arch/powerpc/platforms/pseries/iommu.c | 9 +-
>> drivers/vfio/vfio_iommu_spapr_tce.c | 131 ++++++++++++++++++++--------
>> 9 files changed, 170 insertions(+), 83 deletions(-)
> [snip]
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index 29d5708..28909e1 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -84,7 +84,7 @@ static void decrement_locked_vm(long npages)
>> */
>> struct tce_container {
>> struct mutex lock;
>> - struct iommu_table *tbl;
>> + struct iommu_group *grp;
>> bool enabled;
>> };
>>
>> @@ -104,16 +104,40 @@ static bool tce_check_page_size(struct page *page, unsigned page_shift)
>> return false;
>> }
>>
>> +static struct iommu_table *spapr_tce_find_table(
>> + struct tce_container *container,
>> + phys_addr_t ioba)
>> +{
>> + long i;
>> + struct iommu_table *ret = NULL;
>> + struct powerpc_iommu *iommu = iommu_group_get_iommudata(container->grp);
>> +
>> + mutex_lock(&container->lock);
>> + for (i = 0; i < POWERPC_IOMMU_MAX_TABLES; ++i) {
>> + struct iommu_table *tbl = &iommu->tables[i];
>> + unsigned long entry = ioba >> tbl->it_page_shift;
>> + unsigned long start = tbl->it_offset;
>> + unsigned long end = start + tbl->it_size;
>> +
>> + if ((start <= entry) && (entry < end)) {
>> + ret = tbl;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&container->lock);
>> +
>> + return ret;
>> +}
>> +
>> static int tce_iommu_enable(struct tce_container *container)
>> {
>> int ret = 0;
>> + struct powerpc_iommu *iommu;
>> + struct iommu_table *tbl;
>>
>> - if (!container->tbl)
>> + if (!container->grp)
>> return -ENXIO;
>>
>> - if (!current->mm)
>> - return -ESRCH; /* process exited */
>> -
>> if (container->enabled)
>> return -EBUSY;
>>
>> @@ -142,7 +166,12 @@ static int tce_iommu_enable(struct tce_container *container)
>> * as this information is only available from KVM and VFIO is
>> * KVM agnostic.
>> */
>> - ret = try_increment_locked_vm(IOMMU_TABLE_PAGES(container->tbl));
>> + iommu = iommu_group_get_iommudata(container->grp);
>> + if (!iommu)
>> + return -EFAULT;
>> +
>> + tbl = &iommu->tables[0];
>
>
> There should probably be a comment somewhere documenting that tables[0]
> is the small window and presumably [1] will be the DDW.

Rather than a comment, how about an enum?


Alex

2015-02-05 04:59:01

by Alexey Kardashevskiy

Subject: Re: [PATCH v3 08/24] powerpc/spapr: vfio: Switch from iommu_table to new powerpc_iommu

On 02/05/2015 12:32 AM, Alexander Graf wrote:
>
>
> On 03.02.15 01:12, Alex Williamson wrote:
>> On Thu, 2015-01-29 at 20:21 +1100, Alexey Kardashevskiy wrote:
>>> Modern IBM POWERPC systems support multiple (currently two) TCE tables
>>> per IOMMU group (a.k.a. PE). This adds a powerpc_iommu container
>>> for TCE tables. Right now just one table is supported.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>>> ---
>>> arch/powerpc/include/asm/iommu.h | 18 ++--
>>> arch/powerpc/kernel/eeh.c | 2 +-
>>> arch/powerpc/kernel/iommu.c | 34 ++++----
>>> arch/powerpc/platforms/powernv/pci-ioda.c | 37 +++++---
>>> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 16 ++--
>>> arch/powerpc/platforms/powernv/pci.c | 2 +-
>>> arch/powerpc/platforms/powernv/pci.h | 4 +-
>>> arch/powerpc/platforms/pseries/iommu.c | 9 +-
>>> drivers/vfio/vfio_iommu_spapr_tce.c | 131 ++++++++++++++++++++--------
>>> 9 files changed, 170 insertions(+), 83 deletions(-)
>> [snip]
>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> index 29d5708..28909e1 100644
>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> @@ -84,7 +84,7 @@ static void decrement_locked_vm(long npages)
>>> */
>>> struct tce_container {
>>> struct mutex lock;
>>> - struct iommu_table *tbl;
>>> + struct iommu_group *grp;
>>> bool enabled;
>>> };
>>>
>>> @@ -104,16 +104,40 @@ static bool tce_check_page_size(struct page *page, unsigned page_shift)
>>> return false;
>>> }
>>>
>>> +static struct iommu_table *spapr_tce_find_table(
>>> + struct tce_container *container,
>>> + phys_addr_t ioba)
>>> +{
>>> + long i;
>>> + struct iommu_table *ret = NULL;
>>> + struct powerpc_iommu *iommu = iommu_group_get_iommudata(container->grp);
>>> +
>>> + mutex_lock(&container->lock);
>>> + for (i = 0; i < POWERPC_IOMMU_MAX_TABLES; ++i) {
>>> + struct iommu_table *tbl = &iommu->tables[i];
>>> + unsigned long entry = ioba >> tbl->it_page_shift;
>>> + unsigned long start = tbl->it_offset;
>>> + unsigned long end = start + tbl->it_size;
>>> +
>>> + if ((start <= entry) && (entry < end)) {
>>> + ret = tbl;
>>> + break;
>>> + }
>>> + }
>>> + mutex_unlock(&container->lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static int tce_iommu_enable(struct tce_container *container)
>>> {
>>> int ret = 0;
>>> + struct powerpc_iommu *iommu;
>>> + struct iommu_table *tbl;
>>>
>>> - if (!container->tbl)
>>> + if (!container->grp)
>>> return -ENXIO;
>>>
>>> - if (!current->mm)
>>> - return -ESRCH; /* process exited */
>>> -
>>> if (container->enabled)
>>> return -EBUSY;
>>>
>>> @@ -142,7 +166,12 @@ static int tce_iommu_enable(struct tce_container *container)
>>> * as this information is only available from KVM and VFIO is
>>> * KVM agnostic.
>>> */
>>> - ret = try_increment_locked_vm(IOMMU_TABLE_PAGES(container->tbl));
>>> + iommu = iommu_group_get_iommudata(container->grp);
>>> + if (!iommu)
>>> + return -EFAULT;
>>> +
>>> + tbl = &iommu->tables[0];
>>
>>
>> There should probably be a comment somewhere documenting that tables[0]
>> is the small window and presumably [1] will be the DDW.
>
> Rather than a comment, how about an enum?


[0] could be DDW if the guest decides to remove the default window and
create one huge in its place - older guests (sles11sp3) did that (but they
could not cope with the huge window starting from zero), newer guests do
not try removing the default window but they might want to do this later.

So I am not so sure what kind of comment would be good here...



--
Alexey