by Alexey Kardashevskiy

[permalink] [raw]

Subject: Re: [PATCH kernel v8 21/31] powerpc/iommu: Split iommu_free_table into 2 helpers

On 04/16/2015 04:46 PM, David Gibson wrote:
> On Fri, Apr 10, 2015 at 04:31:03PM +1000, Alexey Kardashevskiy wrote:
>> The iommu_free_table helper release memory it is using (the TCE table and
>> @it_map) and release the iommu_table struct as well. We might not want
>> the very last step as we store iommu_table in parent structures.
>
> Yeah, as I commented on the earlier patch, freeing the surrounding
> group from a function taking just the individual table is wrong.

This is iommu tables created by the old code which stores these iommu_table
struct pointers in device nodes. I believe there is a plan to get rid of
iommu tables there and when this is done, this workaround will be gone.

>
>>
>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>> ---
>> arch/powerpc/include/asm/iommu.h | 1 +
>> arch/powerpc/kernel/iommu.c | 57 ++++++++++++++++++++++++----------------
>> 2 files changed, 35 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
>> index bde7ee7..8ed4648 100644
>> --- a/arch/powerpc/include/asm/iommu.h
>> +++ b/arch/powerpc/include/asm/iommu.h
>> @@ -127,6 +127,7 @@ static inline void *get_iommu_table_base(struct device *dev)
>>
>> extern struct iommu_table *iommu_table_alloc(int node);
>> /* Frees table for an individual device node */
>> +extern void iommu_reset_table(struct iommu_table *tbl, const char *node_name);
>> extern void iommu_free_table(struct iommu_table *tbl, const char *node_name);
>>
>> /* Initializes an iommu_table based in values set in the passed-in
>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> index 501e8ee..0bcd988 100644
>> --- a/arch/powerpc/kernel/iommu.c
>> +++ b/arch/powerpc/kernel/iommu.c
>> @@ -721,24 +721,46 @@ struct iommu_table *iommu_table_alloc(int node)
>> return &table_group->tables[0];
>> }
>>
>> +void iommu_reset_table(struct iommu_table *tbl, const char *node_name)
>> +{
>> + if (!tbl)
>> + return;
>> +
>> + if (tbl->it_map) {
>> + unsigned long bitmap_sz;
>> + unsigned int order;
>> +
>> + /*
>> + * In case we have reserved the first bit, we should not emit
>> + * the warning below.
>> + */
>> + if (tbl->it_offset == 0)
>> + clear_bit(0, tbl->it_map);
>> +
>> + /* verify that table contains no entries */
>> + if (!bitmap_empty(tbl->it_map, tbl->it_size))
>> + pr_warn("%s: Unexpected TCEs for %s\n", __func__,
>> + node_name);
>> +
>> + /* calculate bitmap size in bytes */
>> + bitmap_sz = BITS_TO_LONGS(tbl->it_size) * sizeof(unsigned long);
>> +
>> + /* free bitmap */
>> + order = get_order(bitmap_sz);
>> + free_pages((unsigned long) tbl->it_map, order);
>> + }
>> +
>> + memset(tbl, 0, sizeof(*tbl));
>> +}
>> +
>> void iommu_free_table(struct iommu_table *tbl, const char *node_name)
>> {
>> - unsigned long bitmap_sz;
>> - unsigned int order;
>> struct iommu_table_group *table_group = tbl->it_group;
>>
>> - if (!tbl || !tbl->it_map) {
>> - printk(KERN_ERR "%s: expected TCE map for %s\n", __func__,
>> - node_name);
>> + if (!tbl)
>> return;
>> - }
>>
>> - /*
>> - * In case we have reserved the first bit, we should not emit
>> - * the warning below.
>> - */
>> - if (tbl->it_offset == 0)
>> - clear_bit(0, tbl->it_map);
>> + iommu_reset_table(tbl, node_name);
>>
>> #ifdef CONFIG_IOMMU_API
>> if (table_group->group) {
>> @@ -747,17 +769,6 @@ void iommu_free_table(struct iommu_table *tbl, const char *node_name)
>> }
>> #endif
>>
>> - /* verify that table contains no entries */
>> - if (!bitmap_empty(tbl->it_map, tbl->it_size))
>> - pr_warn("%s: Unexpected TCEs for %s\n", __func__, node_name);
>> -
>> - /* calculate bitmap size in bytes */
>> - bitmap_sz = BITS_TO_LONGS(tbl->it_size) * sizeof(unsigned long);
>> -
>> - /* free bitmap */
>> - order = get_order(bitmap_sz);
>> - free_pages((unsigned long) tbl->it_map, order);
>> -
>> /* free table */
>> kfree(table_group);
>> }
>

--
Alexey

2015-04-17 09:46:33

by Alexey Kardashevskiy

[permalink] [raw]

On 04/20/2015 12:46 PM, David Gibson wrote:
> On Fri, Apr 17, 2015 at 08:16:13PM +1000, Alexey Kardashevskiy wrote:
>> On 04/16/2015 04:10 PM, David Gibson wrote:
>>> On Fri, Apr 10, 2015 at 04:30:57PM +1000, Alexey Kardashevskiy wrote:
>>>> This adds missing locks in iommu_take_ownership()/
>>>> iommu_release_ownership().
>>>>
>>>> This marks all pages busy in iommu_table::it_map in order to catch
>>>> errors if there is an attempt to use this table while ownership over it
>>>> is taken.
>>>>
>>>> This only clears TCE content if there is no page marked busy in it_map.
>>>> Clearing must be done outside of the table locks as iommu_clear_tce()
>>>> called from iommu_clear_tces_and_put_pages() does this.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>>>> ---
>>>> Changes:
>>>> v5:
>>>> * do not store bit#0 value, it has to be set for zero-based table
>>>> anyway
>>>> * removed test_and_clear_bit
>>>> ---
>>>> arch/powerpc/kernel/iommu.c | 26 ++++++++++++++++++++++----
>>>> 1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>>> index 7d6089b..068fe4ff 100644
>>>> --- a/arch/powerpc/kernel/iommu.c
>>>> +++ b/arch/powerpc/kernel/iommu.c
>>>> @@ -1052,17 +1052,28 @@ EXPORT_SYMBOL_GPL(iommu_tce_build);
>>>>
>>>> static int iommu_table_take_ownership(struct iommu_table *tbl)
>>>> {
>>>> - unsigned long sz = (tbl->it_size + 7) >> 3;
>>>> + unsigned long flags, i, sz = (tbl->it_size + 7) >> 3;
>>>> + int ret = 0;
>>>> +
>>>> + spin_lock_irqsave(&tbl->large_pool.lock, flags);
>>>> + for (i = 0; i < tbl->nr_pools; i++)
>>>> + spin_lock(&tbl->pools[i].lock);
>>>>
>>>> if (tbl->it_offset == 0)
>>>> clear_bit(0, tbl->it_map);
>>>>
>>>> if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
>>>> pr_err("iommu_tce: it_map is not empty");
>>>> - return -EBUSY;
>>>> + ret = -EBUSY;
>>>> + if (tbl->it_offset == 0)
>>>> + set_bit(0, tbl->it_map);
>>>
>>> This really needs a comment. Why on earth are you changing the it_map
>>> on a failure case?
>>
>>
>> Does this explain?
>>
>> /*
>> * The platform code reserves zero address in iommu_init_table().
>> * As we cleared busy bit for page @0 before using bitmap_empty(),
>> * we are restoring it now.
>> */
>
> Only partly. What's it reserved for, and why do you know it was
> always set on entry?

Because it is only handled in this file and I can see it in the code. Or I
did not understand the question here...

--
Alexey

2015-04-20 06:55:41

by Alexey Kardashevskiy

[permalink] [raw]

Subject: Re: [PATCH kernel v8 14/31] vfio: powerpc/spapr: powerpc/powernv/ioda2: Rework IOMMU ownership control

On 04/20/2015 12:44 PM, David Gibson wrote:
> On Fri, Apr 17, 2015 at 08:09:29PM +1000, Alexey Kardashevskiy wrote:
>> On 04/16/2015 04:07 PM, David Gibson wrote:
>>> On Fri, Apr 10, 2015 at 04:30:56PM +1000, Alexey Kardashevskiy wrote:
>>>> At the moment the iommu_table struct has a set_bypass() which enables/
>>>> disables DMA bypass on IODA2 PHB. This is exposed to POWERPC IOMMU code
>>>> which calls this callback when external IOMMU users such as VFIO are
>>>> about to get over a PHB.
>>>>
>>>> The set_bypass() callback is not really an iommu_table function but
>>>> IOMMU/PE function. This introduces a iommu_table_group_ops struct and
>>>> adds a set_ownership() callback to it which is called when an external
>>>> user takes control over the IOMMU.
>>>
>>> Do you really need separate ops structures at both the single table
>>> and table group level? The different tables in a group will all
>>> belong to the same basic iommu won't they?
>>
>>
>> IOMMU tables exist alone in VIO. Also, the platform code uses just a table
>> (or it is in bypass mode) and does not care about table groups. It looked
>> more clean for myself to keep them separated. Should I still merge
>> those?
>
> Ok, that sounds like a reasonable argument for keeping them separate,
> at least for now.
>
>>>> This renames set_bypass() to set_ownership() as it is not necessarily
>>>> just enabling bypassing, it can be something else/more so let's give it
>>>> more generic name. The bool parameter is inverted.
>>>>
>>>> The callback is implemented for IODA2 only. Other platforms (P5IOC2,
>>>> IODA1) will use the old iommu_take_ownership/iommu_release_ownership API.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <[email protected]>
>>>> ---
>>>> arch/powerpc/include/asm/iommu.h | 14 +++++++++++++-
>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 30 ++++++++++++++++++++++--------
>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 25 +++++++++++++++++++++----
>>>> 3 files changed, 56 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
>>>> index b9e50d3..d1f8c6c 100644
>>>> --- a/arch/powerpc/include/asm/iommu.h
>>>> +++ b/arch/powerpc/include/asm/iommu.h
>>>> @@ -92,7 +92,6 @@ struct iommu_table {
>>>> unsigned long it_page_shift;/* table iommu page size */
>>>> struct iommu_table_group *it_group;
>>>> struct iommu_table_ops *it_ops;
>>>> - void (*set_bypass)(struct iommu_table *tbl, bool enable);
>>>> };
>>>>
>>>> /* Pure 2^n version of get_order */
>>>> @@ -127,11 +126,24 @@ extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
>>>>
>>>> #define IOMMU_TABLE_GROUP_MAX_TABLES 1
>>>>
>>>> +struct iommu_table_group;
>>>> +
>>>> +struct iommu_table_group_ops {
>>>> + /*
>>>> + * Switches ownership from the kernel itself to an external
>>>> + * user. While onwership is enabled, the kernel cannot use IOMMU
>>>> + * for itself.
>>>> + */
>>>> + void (*set_ownership)(struct iommu_table_group *table_group,
>>>> + bool enable);
>>>
>>> The meaning of "enable" in a function called "set_ownership" is
>>> entirely obscure.
>>
>> Suggest something better please :) I have nothing better...
>
> Well, given it's "set_ownershuip" you could have "owner" - that would
> want to be an enum with OWNER_KERNEL and OWNER_VFIO or something
> rather than a bool.

It is iommu_take_ownership() in upstream and it is assumed that the owner
is anything but the platform code (for now and probably for ever - VFIO). I
am not changing this now, just using same naming approach when adding a
callback with a similar name.

> Or you could leave it a bool but call it "allow_bypass".

Commented below.

>
>>
>>
>>>
>>>> +};
>>>> +
>>>> struct iommu_table_group {
>>>> #ifdef CONFIG_IOMMU_API
>>>> struct iommu_group *group;
>>>> #endif
>>>> struct iommu_table tables[IOMMU_TABLE_GROUP_MAX_TABLES];
>>>> + struct iommu_table_group_ops *ops;
>>>> };
>>>>
>>>> #ifdef CONFIG_IOMMU_API
>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> index a964c50..9687731 100644
>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> @@ -1255,10 +1255,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
>>>> __free_pages(tce_mem, get_order(TCE32_TABLE_SIZE * segs));
>>>> }
>>>>
>>>> -static void pnv_pci_ioda2_set_bypass(struct iommu_table *tbl, bool enable)
>>>> +static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable)
>>>> {
>>>> - struct pnv_ioda_pe *pe = container_of(tbl->it_group, struct pnv_ioda_pe,
>>>> - table_group);
>>>> uint16_t window_id = (pe->pe_number << 1 ) + 1;
>>>> int64_t rc;
>>>>
>>>> @@ -1286,7 +1284,8 @@ static void pnv_pci_ioda2_set_bypass(struct iommu_table *tbl, bool enable)
>>>> * host side.
>>>> */
>>>> if (pe->pdev)
>>>> - set_iommu_table_base(&pe->pdev->dev, tbl);
>>>> + set_iommu_table_base(&pe->pdev->dev,
>>>> + &pe->table_group.tables[0]);
>>>> else
>>>> pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
>>>> }
>>>> @@ -1302,13 +1301,27 @@ static void pnv_pci_ioda2_setup_bypass_pe(struct pnv_phb *phb,
>>>> /* TVE #1 is selected by PCI address bit 59 */
>>>> pe->tce_bypass_base = 1ull << 59;
>>>>
>>>> - /* Install set_bypass callback for VFIO */
>>>> - pe->table_group.tables[0].set_bypass = pnv_pci_ioda2_set_bypass;
>>>> -
>>>> /* Enable bypass by default */
>>>> - pnv_pci_ioda2_set_bypass(&pe->table_group.tables[0], true);
>>>> + pnv_pci_ioda2_set_bypass(pe, true);
>>>> }
>>>>
>>>> +static void pnv_ioda2_set_ownership(struct iommu_table_group *table_group,
>>>> + bool enable)
>>>> +{
>>>> + struct pnv_ioda_pe *pe = container_of(table_group, struct pnv_ioda_pe,
>>>> + table_group);
>>>> + if (enable)
>>>> + iommu_take_ownership(table_group);
>>>> + else
>>>> + iommu_release_ownership(table_group);
>>>> +
>>>> + pnv_pci_ioda2_set_bypass(pe, !enable);
>>>> +}
>>>> +
>>>> +static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
>>>> + .set_ownership = pnv_ioda2_set_ownership,
>>>> +};
>>>> +
>>>> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
>>>> struct pnv_ioda_pe *pe)
>>>> {
>>>> @@ -1376,6 +1389,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
>>>> }
>>>> tbl->it_ops = &pnv_iommu_ops;
>>>> iommu_init_table(tbl, phb->hose->node);
>>>> + pe->table_group.ops = &pnv_pci_ioda2_ops;
>>>> iommu_register_group(&pe->table_group, phb->hose->global_number,
>>>> pe->pe_number);
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> index 9f38351..d5d8c50 100644
>>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> @@ -535,9 +535,22 @@ static int tce_iommu_attach_group(void *iommu_data,
>>>> goto unlock_exit;
>>>> }
>>>>
>>>> - ret = iommu_take_ownership(table_group);
>>>> - if (!ret)
>>>> - container->grp = iommu_group;
>>>> + if (!table_group->ops || !table_group->ops->set_ownership) {
>>>> + ret = iommu_take_ownership(table_group);
>>>> + } else {
>>>> + /*
>>>> + * Disable iommu bypass, otherwise the user can DMA to all of
>>>> + * our physical memory via the bypass window instead of just
>>>> + * the pages that has been explicitly mapped into the iommu
>>>> + */
>>>> + table_group->ops->set_ownership(table_group, true);
>>>
>>> And here to disable bypass you call it with enable=true, so it doesn't
>>> even have the same meaning as it used to.
>>
>>
>> I do not disable bypass per se (even if it what set_ownership(true) does) as
>> it is IODA business and VFIO has no idea about it. I do take control over
>> the group. I am not following you here - what used to have the same
>> meaning?
>
> Well, in set_bypass, the enable parameter was whether bypass was
> enabled. Here you're setting enable to true, when you want to
> *disable* bypass (in the existing case). If the "enable" parameter
> isn't about enabling bypass, it's meaning is even more confusing than
> I thought.

Its meaning is "take ownership over the group". In this patch
set_ownership(true) means set_bypass(false).

But later (in 25/31) set_ownership(true) becomes unset(windows0) +
free(table0) + set_bypass(false) = clear DMA setup for the group (i.e.
invalidate both TVTs) so it is not just about bypass (which is TVT#1 but
not TVT#0) anymore.

>>> Plus, you should fold the logic to call the callback if necessary into
>>> iommu_take_ownership().
>>
>>
>> I really want to keep VFIO stuff out of arch/powerpc/kernel/iommu.c as much
>> as possible as it is for platform DMA/IOMMU, not VFIO (which got SPAPR
>> driver for that). ops->set_ownership() is one of these things.
>
> What's VFIO specific about this fragment - it's just if you have the
> callback, call it, otherwise fall back to the default.
>
>> iommu_take_ownership()/iommu_release_ownership() are helpers for old-style
>> commercially-unsupported P5IOC2/IODA1, and this is kind of a hack while
>> ops->set_ownership() is an interface for VFIO to do dynamic windows thing.
>
> Can you put their logic into a set_ownership callback for IODA1 then?

And P5IOC2, and pseries. We know these callbacks will call the same
iommu_take_ownership() and iommu_release_ownership() and this is not going
to change. Too invasive for such a hack imho.

>> If it makes sense, I could fold the previous patch into this one and move
>> iommu_take_ownership()/iommu_release_ownership() to vfio_iommu_spapr_tce.c,
>> should I? Or leave things are they are now.
>
> That sounds like it might make sense.

This is what will go to v9, looks cleaner. Thanks.

>>
>>
>>>> + ret = 0;
>>>> + }
>>>> +
>>>> + if (ret)
>>>> + goto unlock_exit;
>>>> +
>>>> + container->grp = iommu_group;
>>>>
>>>> unlock_exit:
>>>> mutex_unlock(&container->lock);
>>>> @@ -572,7 +585,11 @@ static void tce_iommu_detach_group(void *iommu_data,
>>>> table_group = iommu_group_get_iommudata(iommu_group);
>>>> BUG_ON(!table_group);
>>>>
>>>> - iommu_release_ownership(table_group);
>>>> + /* Kernel owns the device now, we can restore bypass */
>>>> + if (!table_group->ops || !table_group->ops->set_ownership)
>>>> + iommu_release_ownership(table_group);
>>>> + else
>>>> + table_group->ops->set_ownership(table_group, false);
>>>
>>> Likewise fold this if into iommu_release_ownership().
>>>
>>>> unlock_exit:
>>>> mutex_unlock(&container->lock);
>>>
>>
>>
>

--
Alexey

2015-04-21 07:28:58

by David Gibson

[permalink] [raw]

Subject: Re: [PATCH kernel v8 15/31] powerpc/iommu: Fix IOMMU ownership control functions

On Mon, Apr 20, 2015 at 04:34:24PM +1000, Alexey Kardashevskiy wrote:
> On 04/20/2015 12:46 PM, David Gibson wrote:
> >On Fri, Apr 17, 2015 at 08:16:13PM +1000, Alexey Kardashevskiy wrote:
> >>On 04/16/2015 04:10 PM, David Gibson wrote:
> >>>On Fri, Apr 10, 2015 at 04:30:57PM +1000, Alexey Kardashevskiy wrote:
> >>>>This adds missing locks in iommu_take_ownership()/
> >>>>iommu_release_ownership().
> >>>>
> >>>>This marks all pages busy in iommu_table::it_map in order to catch
> >>>>errors if there is an attempt to use this table while ownership over it
> >>>>is taken.
> >>>>
> >>>>This only clears TCE content if there is no page marked busy in it_map.
> >>>>Clearing must be done outside of the table locks as iommu_clear_tce()
> >>>>called from iommu_clear_tces_and_put_pages() does this.
> >>>>
> >>>>Signed-off-by: Alexey Kardashevskiy <[email protected]>
> >>>>---
> >>>>Changes:
> >>>>v5:
> >>>>* do not store bit#0 value, it has to be set for zero-based table
> >>>>anyway
> >>>>* removed test_and_clear_bit
> >>>>---
> >>>> arch/powerpc/kernel/iommu.c | 26 ++++++++++++++++++++++----
> >>>> 1 file changed, 22 insertions(+), 4 deletions(-)
> >>>>
> >>>>diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >>>>index 7d6089b..068fe4ff 100644
> >>>>--- a/arch/powerpc/kernel/iommu.c
> >>>>+++ b/arch/powerpc/kernel/iommu.c
> >>>>@@ -1052,17 +1052,28 @@ EXPORT_SYMBOL_GPL(iommu_tce_build);
> >>>>
> >>>> static int iommu_table_take_ownership(struct iommu_table *tbl)
> >>>> {
> >>>>- unsigned long sz = (tbl->it_size + 7) >> 3;
> >>>>+ unsigned long flags, i, sz = (tbl->it_size + 7) >> 3;
> >>>>+ int ret = 0;
> >>>>+
> >>>>+ spin_lock_irqsave(&tbl->large_pool.lock, flags);
> >>>>+ for (i = 0; i < tbl->nr_pools; i++)
> >>>>+ spin_lock(&tbl->pools[i].lock);
> >>>>
> >>>> if (tbl->it_offset == 0)
> >>>> clear_bit(0, tbl->it_map);
> >>>>
> >>>> if (!bitmap_empty(tbl->it_map, tbl->it_size)) {
> >>>> pr_err("iommu_tce: it_map is not empty");
> >>>>- return -EBUSY;
> >>>>+ ret = -EBUSY;
> >>>>+ if (tbl->it_offset == 0)
> >>>>+ set_bit(0, tbl->it_map);
> >>>
> >>>This really needs a comment. Why on earth are you changing the it_map
> >>>on a failure case?
> >>
> >>
> >>Does this explain?
> >>
> >>/*
> >> * The platform code reserves zero address in iommu_init_table().
> >> * As we cleared busy bit for page @0 before using bitmap_empty(),
> >> * we are restoring it now.
> >> */
> >
> >Only partly. What's it reserved for, and why do you know it was
> >always set on entry?
>
>
> Because it is only handled in this file and I can see it in the code. Or I
> did not understand the question here...

Sure, you can see it in the code, but you've been looking at this code
for months and years. For anyone looking at this function who's not
familiar with the rest of the IOMMU code, a pointer to what's going on
here would be really helpful.

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

Attachments:

(No filename) (3.02 kB)
(No filename) (819.00 B)
Download all attachments

2015-04-21 09:43:20

On Tue, Apr 21, 2015 at 09:47:54PM +1000, Alexey Kardashevskiy wrote:
> On 04/21/2015 07:43 PM, David Gibson wrote:
> >On Mon, Apr 20, 2015 at 04:55:32PM +1000, Alexey Kardashevskiy wrote:
> >>On 04/20/2015 12:44 PM, David Gibson wrote:
> >>>On Fri, Apr 17, 2015 at 08:09:29PM +1000, Alexey Kardashevskiy wrote:
> >>>>On 04/16/2015 04:07 PM, David Gibson wrote:
> >>>>>On Fri, Apr 10, 2015 at 04:30:56PM +1000, Alexey Kardashevskiy wrote:
> >>>>>>At the moment the iommu_table struct has a set_bypass() which enables/
> >>>>>>disables DMA bypass on IODA2 PHB. This is exposed to POWERPC IOMMU code
> >>>>>>which calls this callback when external IOMMU users such as VFIO are
> >>>>>>about to get over a PHB.
> >>>>>>
> >>>>>>The set_bypass() callback is not really an iommu_table function but
> >>>>>>IOMMU/PE function. This introduces a iommu_table_group_ops struct and
> >>>>>>adds a set_ownership() callback to it which is called when an external
> >>>>>>user takes control over the IOMMU.
> >>>>>
> >>>>>Do you really need separate ops structures at both the single table
> >>>>>and table group level? The different tables in a group will all
> >>>>>belong to the same basic iommu won't they?
> >>>>
> >>>>
> >>>>IOMMU tables exist alone in VIO. Also, the platform code uses just a table
> >>>>(or it is in bypass mode) and does not care about table groups. It looked
> >>>>more clean for myself to keep them separated. Should I still merge
> >>>>those?
> >>>
> >>>Ok, that sounds like a reasonable argument for keeping them separate,
> >>>at least for now.
> >>>
> >>>>>>This renames set_bypass() to set_ownership() as it is not necessarily
> >>>>>>just enabling bypassing, it can be something else/more so let's give it
> >>>>>>more generic name. The bool parameter is inverted.
> >>>>>>
> >>>>>>The callback is implemented for IODA2 only. Other platforms (P5IOC2,
> >>>>>>IODA1) will use the old iommu_take_ownership/iommu_release_ownership API.
> >>>>>>
> >>>>>>Signed-off-by: Alexey Kardashevskiy <[email protected]>
> >>>>>>---
> >>>>>> arch/powerpc/include/asm/iommu.h | 14 +++++++++++++-
> >>>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 30 ++++++++++++++++++++++--------
> >>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 25 +++++++++++++++++++++----
> >>>>>> 3 files changed, 56 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>>diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> >>>>>>index b9e50d3..d1f8c6c 100644
> >>>>>>--- a/arch/powerpc/include/asm/iommu.h
> >>>>>>+++ b/arch/powerpc/include/asm/iommu.h
> >>>>>>@@ -92,7 +92,6 @@ struct iommu_table {
> >>>>>> unsigned long it_page_shift;/* table iommu page size */
> >>>>>> struct iommu_table_group *it_group;
> >>>>>> struct iommu_table_ops *it_ops;
> >>>>>>- void (*set_bypass)(struct iommu_table *tbl, bool enable);
> >>>>>> };
> >>>>>>
> >>>>>> /* Pure 2^n version of get_order */
> >>>>>>@@ -127,11 +126,24 @@ extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
> >>>>>>
> >>>>>> #define IOMMU_TABLE_GROUP_MAX_TABLES 1
> >>>>>>
> >>>>>>+struct iommu_table_group;
> >>>>>>+
> >>>>>>+struct iommu_table_group_ops {
> >>>>>>+ /*
> >>>>>>+ * Switches ownership from the kernel itself to an external
> >>>>>>+ * user. While onwership is enabled, the kernel cannot use IOMMU
> >>>>>>+ * for itself.
> >>>>>>+ */
> >>>>>>+ void (*set_ownership)(struct iommu_table_group *table_group,
> >>>>>>+ bool enable);
> >>>>>
> >>>>>The meaning of "enable" in a function called "set_ownership" is
> >>>>>entirely obscure.
> >>>>
> >>>>Suggest something better please :) I have nothing better...
> >>>
> >>>Well, given it's "set_ownershuip" you could have "owner" - that would
> >>>want to be an enum with OWNER_KERNEL and OWNER_VFIO or something
> >>>rather than a bool.
> >>
> >>
> >>It is iommu_take_ownership() in upstream and it is assumed that the owner is
> >>anything but the platform code (for now and probably for ever - VFIO). I am
> >>not changing this now, just using same naming approach when adding a
> >>callback with a similar name.
> >
> >So "enabled" is actually that non kernel ownership is enabled. That
> >is totally non-obvious.
> >
> >>>Or you could leave it a bool but call it "allow_bypass".
> >>
> >>Commented below.
> >>
> >>>>>>+};
> >>>>>>+
> >>>>>> struct iommu_table_group {
> >>>>>> #ifdef CONFIG_IOMMU_API
> >>>>>> struct iommu_group *group;
> >>>>>> #endif
> >>>>>> struct iommu_table tables[IOMMU_TABLE_GROUP_MAX_TABLES];
> >>>>>>+ struct iommu_table_group_ops *ops;
> >>>>>> };
> >>>>>>
> >>>>>> #ifdef CONFIG_IOMMU_API
> >>>>>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>index a964c50..9687731 100644
> >>>>>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>>>@@ -1255,10 +1255,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
> >>>>>> __free_pages(tce_mem, get_order(TCE32_TABLE_SIZE * segs));
> >>>>>> }
> >>>>>>
> >>>>>>-static void pnv_pci_ioda2_set_bypass(struct iommu_table *tbl, bool enable)
> >>>>>>+static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable)
> >>>>>> {
> >>>>>>- struct pnv_ioda_pe *pe = container_of(tbl->it_group, struct pnv_ioda_pe,
> >>>>>>- table_group);
> >>>>>> uint16_t window_id = (pe->pe_number << 1 ) + 1;
> >>>>>> int64_t rc;
> >>>>>>
> >>>>>>@@ -1286,7 +1284,8 @@ static void pnv_pci_ioda2_set_bypass(struct iommu_table *tbl, bool enable)
> >>>>>> * host side.
> >>>>>> */
> >>>>>> if (pe->pdev)
> >>>>>>- set_iommu_table_base(&pe->pdev->dev, tbl);
> >>>>>>+ set_iommu_table_base(&pe->pdev->dev,
> >>>>>>+ &pe->table_group.tables[0]);
> >>>>>> else
> >>>>>> pnv_ioda_setup_bus_dma(pe, pe->pbus, false);
> >>>>>> }
> >>>>>>@@ -1302,13 +1301,27 @@ static void pnv_pci_ioda2_setup_bypass_pe(struct pnv_phb *phb,
> >>>>>> /* TVE #1 is selected by PCI address bit 59 */
> >>>>>> pe->tce_bypass_base = 1ull << 59;
> >>>>>>
> >>>>>>- /* Install set_bypass callback for VFIO */
> >>>>>>- pe->table_group.tables[0].set_bypass = pnv_pci_ioda2_set_bypass;
> >>>>>>-
> >>>>>> /* Enable bypass by default */
> >>>>>>- pnv_pci_ioda2_set_bypass(&pe->table_group.tables[0], true);
> >>>>>>+ pnv_pci_ioda2_set_bypass(pe, true);
> >>>>>> }
> >>>>>>
> >>>>>>+static void pnv_ioda2_set_ownership(struct iommu_table_group *table_group,
> >>>>>>+ bool enable)
> >>>>>>+{
> >>>>>>+ struct pnv_ioda_pe *pe = container_of(table_group, struct pnv_ioda_pe,
> >>>>>>+ table_group);
> >>>>>>+ if (enable)
> >>>>>>+ iommu_take_ownership(table_group);
> >>>>>>+ else
> >>>>>>+ iommu_release_ownership(table_group);
> >>>>>>+
> >>>>>>+ pnv_pci_ioda2_set_bypass(pe, !enable);
> >>>>>>+}
> >>>>>>+
> >>>>>>+static struct iommu_table_group_ops pnv_pci_ioda2_ops = {
> >>>>>>+ .set_ownership = pnv_ioda2_set_ownership,
> >>>>>>+};
> >>>>>>+
> >>>>>> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> >>>>>> struct pnv_ioda_pe *pe)
> >>>>>> {
> >>>>>>@@ -1376,6 +1389,7 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> >>>>>> }
> >>>>>> tbl->it_ops = &pnv_iommu_ops;
> >>>>>> iommu_init_table(tbl, phb->hose->node);
> >>>>>>+ pe->table_group.ops = &pnv_pci_ioda2_ops;
> >>>>>> iommu_register_group(&pe->table_group, phb->hose->global_number,
> >>>>>> pe->pe_number);
> >>>>>>
> >>>>>>diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>>>index 9f38351..d5d8c50 100644
> >>>>>>--- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>>>+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>>>@@ -535,9 +535,22 @@ static int tce_iommu_attach_group(void *iommu_data,
> >>>>>> goto unlock_exit;
> >>>>>> }
> >>>>>>
> >>>>>>- ret = iommu_take_ownership(table_group);
> >>>>>>- if (!ret)
> >>>>>>- container->grp = iommu_group;
> >>>>>>+ if (!table_group->ops || !table_group->ops->set_ownership) {
> >>>>>>+ ret = iommu_take_ownership(table_group);
> >>>>>>+ } else {
> >>>>>>+ /*
> >>>>>>+ * Disable iommu bypass, otherwise the user can DMA to all of
> >>>>>>+ * our physical memory via the bypass window instead of just
> >>>>>>+ * the pages that has been explicitly mapped into the iommu
> >>>>>>+ */
> >>>>>>+ table_group->ops->set_ownership(table_group, true);
> >>>>>
> >>>>>And here to disable bypass you call it with enable=true, so it doesn't
> >>>>>even have the same meaning as it used to.
> >>>>
> >>>>
> >>>>I do not disable bypass per se (even if it what set_ownership(true) does) as
> >>>>it is IODA business and VFIO has no idea about it. I do take control over
> >>>>the group. I am not following you here - what used to have the same
> >>>>meaning?
> >>>
> >>>Well, in set_bypass, the enable parameter was whether bypass was
> >>>enabled. Here you're setting enable to true, when you want to
> >>>*disable* bypass (in the existing case). If the "enable" parameter
> >>>isn't about enabling bypass, it's meaning is even more confusing than
> >>>I thought.
> >>
> >>
> >>Its meaning is "take ownership over the group". In this patch
> >>set_ownership(true) means set_bypass(false).
> >
> >Ok. So "take_ownership" isn't quite as clear as I'd like, but it's
> >not too bad because it's implied that it's the caller that's taking
> >the ownership. *set* ownership makes no sense without saying who the
> >new owner is. "enable" has no clear meaning in that context.
> >
> >Calling it "kernel_owned" or "non_kernel_owned" would be ok if a bit
> >clunky.
>
>
> Strictly speaking VFIO and platform code are both kernel.

Well, true, but VFIO is generally holding the device on behalf of a
userspace process or guest.

> So which one to choose?
>
> +struct iommu_table_group_ops {
> + void (*take_ownership)(struct iommu_table_group *table_group);
> + void (*release_ownership)(struct iommu_table_group *table_group);
> +};
>
>
> OR
>
> +enum { IOMMU_TABLE_GROUP_OWNER_KERNEL, IOMMU_TABLE_GROUP_OWNER_VFIO };
> +struct iommu_table_group_ops {
> + void (*set_ownership)(struct iommu_table_group *table_group,
> + long owner);
> +};
>
>
> I have bad taste for names like this, need a hint here, please :)

I think I'd be ok with either.

I think I'd vote for the first option, for consistency with the
existing function names. If that requires a bunch of code duplication
in the implementations between take and release, I'd probably change
my mind though.

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

Attachments:

(No filename) (10.42 kB)
(No filename) (819.00 B)
Download all attachments