* Re: [PATCH RESEND] net: fec_mpc52xx: Read MAC address from device-tree
From: David Miller @ 2013-02-11 18:50 UTC (permalink / raw)
To: sr; +Cc: netdev, agust, linuxppc-dev
In-Reply-To: <1360403352-24237-1-git-send-email-sr@denx.de>
From: Stefan Roese <sr@denx.de>
Date: Sat, 9 Feb 2013 10:49:12 +0100
> Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
> (U-Boot) to write the MAC address into the ethernet controller
> registers. The Linux driver should not rely on such a thing. So
> lets read the MAC address from the DT as it should be done here.
>
> This fixes a problem with a MPC5200 board that uses the SPL U-Boot
> version without FEC initialization before Linux booting for
> boot speedup.
>
> Additionally a status line will now be printed upon successful
> driver probing, also displaying this MAC address.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
I don't think this is a conservative enough change.
You have to keep the MAC register reading code around, as a backup
code path in case the OF device node lacks a MAC address, also:
> + if (!is_zero_ether_addr(mpc52xx_fec_mac_addr)) {
I really wish I would have caught this terrible module parameter
when the driver was initially submitted.
I would just get rid of this, and have a priority list of cases:
1) First, try OF node MAC address, if not present or invalid, then:
2) Read from MAC address registers, if invalid, then:
3) Log a warning message, and choose a random MAC address.
That way no matter what happens, the user will at least have a
functioning networking device.
^ permalink raw reply
* Re: [PATCH v5 00/45] CPU hotplug: stop_machine()-free CPU hotplug
From: Paul E. McKenney @ 2013-02-11 19:08 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linux-doc, peterz, fweisbec, linux-kernel, walken, mingo,
linux-arch, Russell King - ARM Linux, xiaoguangrong, wangyun,
nikunj, linux-pm, Rusty Russell, rostedt, rjw, namhyung, tglx,
linux-arm-kernel, netdev, oleg, Vincent Guittot, sbw, tj, akpm,
linuxppc-dev
In-Reply-To: <5118E2CD.90401@linux.vnet.ibm.com>
On Mon, Feb 11, 2013 at 05:53:41PM +0530, Srivatsa S. Bhat wrote:
> On 02/11/2013 05:28 PM, Vincent Guittot wrote:
> > On 8 February 2013 19:09, Srivatsa S. Bhat
> > <srivatsa.bhat@linux.vnet.ibm.com> wrote:
[ . . . ]
> >> Adding Vincent to CC, who had previously evaluated the performance and
> >> latency implications of CPU hotplug on ARM platforms, IIRC.
> >>
> >
> > Hi Srivatsa,
> >
> > I can try to run some of our stress tests on your patches.
>
> Great!
>
> > Have you
> > got a git tree that i can pull ?
> >
>
> Unfortunately, no, none at the moment.. :-(
You do need to create an externally visible git tree. In the meantime,
I have added your series at rcu/bhat.2013.01.21a on -rcu:
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
This should appear soon on a kernel.org mirror near you. ;-)
Thanx, Paul
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Grant Likely @ 2013-02-11 20:52 UTC (permalink / raw)
To: Michael Ellerman, Baruch Siach; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20130211061949.GA5561@concordia>
On Mon, 11 Feb 2013 17:19:49 +1100, Michael Ellerman <michael@ellerman.id.au> wrote:
> On Mon, Feb 11, 2013 at 07:31:00AM +0200, Baruch Siach wrote:
> > Hi lkml,
>
> Hi Baruch,
>
> > The drivers/edac/mpc85xx_edac.c driver contains the following (abbreviated)
> > code snippet it its .probe:
>
> You dropped an important detail which is the preceeding line:
>
> pdata->irq = irq_of_parse_and_map(op->dev.of_node, 0);
>
> > res = devm_request_irq(&op->dev, pdata->irq,
> > mpc85xx_pci_isr, IRQF_DISABLED,
> > "[EDAC] PCI err", pci);
> > if (res < 0) {
> > irq_dispose_mapping(pdata->irq);
> > goto err2;
> > }
> >
> > Now, since the requested irq is already in use, and IRQF_SHARED is not set,
> > devm_request_irq errors() out, which is OK. Less OK is the
> > irq_dispose_mapping() call, which gives me this:
> >
> > EDAC PCI1: Giving out device to module 'MPC85xx_edac' controller 'mpc85xx_pci_err': DEV 'ffe0a000.pcie' (INTERRUPT)
> > genirq: Flags mismatch irq 16. 00000020 ([EDAC] PCI err) vs. 00000020 ([EDAC] PCI err)
>
> The hint here is to notice which other irq you're clashing with ^^
> ie. yourself. Which is odd, that is the root of the problem.
>
> The badness you're getting from irq_dispose_mapping() is caused because you're
> disposing of that mapping which is currently still in use, by the same interrupt.
>
> That is caused by a "feature" in the irq mapping code, where if you ask to map an
> already mapped hwirq, it will give you back the same virq. So in your case when
> you called irq_of_parse_and_map() it noticed that someone had already mapped
> that hwirq, and gave you back an existing (in use) virq.
Really the irq mappings should be using reference counting. The existing
code is naive on this count and just releases the irq on the first call
to irq_dispose_mapping(). I've not gotten around to fixing that. Anyone
want to take that task on?
g.
^ permalink raw reply
* Re: Re[2]: PS3 platform is broken on Linux 3.7.0
From: Geoff Levand @ 2013-02-11 20:56 UTC (permalink / raw)
To: Phileas Fogg; +Cc: linuxppc-dev, Aneesh Kumar K.V
In-Reply-To: <1360486777.923599245@f85.mail.ru>
On Sun, 2013-02-10 at 12:59 +0400, Phileas Fogg wrote:
> i found where the problem lies.
...
> And that's why lv1_insert_htab_entry fails with -17 which means LV1_ILLEGAL_PARAMETER_VALUE because
> the Hypervisor of PS3 checks 'AVPN' values for number of leading zeros and allows at least 15 bits which in case
> of 'v' value 25008684d8001 is too small of course.
Excellent work, thanks for debugging!
-Geoff
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: enabled on powernv platform
From: Alex Williamson @ 2013-02-11 22:16 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360583672-21924-2-git-send-email-aik@ozlabs.ru>
On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
> This patch initializes IOMMU groups based on the IOMMU
> configuration discovered during the PCI scan on POWERNV
> (POWER non virtualized) platform. The IOMMU groups are
> to be used later by VFIO driver (PCI pass through).
>
> It also implements an API for mapping/unmapping pages for
> guest PCI drivers and providing DMA window properties.
> This API is going to be used later by QEMU-VFIO to handle
> h_put_tce hypercalls from the KVM guest.
>
> The iommu_put_tce_user_mode() does only a single page mapping
> as an API for adding many mappings at once is going to be
> added later.
>
> Although this driver has been tested only on the POWERNV
> platform, it should work on any platform which supports
> TCE tables. As h_put_tce hypercall is received by the host
> kernel and processed by the QEMU (what involves calling
> the host kernel again), performance is not the best -
> circa 220MB/s on 10Gb ethernet network.
>
> To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config
> option and configure VFIO as required.
>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Yay, it's not dead! ;)
I'd love some kind of changelog to know what to look for in here,
especially given 2mo since the last version.
> ---
> arch/powerpc/include/asm/iommu.h | 15 ++
> arch/powerpc/kernel/iommu.c | 343 +++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 5 +-
> arch/powerpc/platforms/powernv/pci.c | 3 +
> drivers/iommu/Kconfig | 8 +
> 6 files changed, 374 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index cbfe678..900294b 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -76,6 +76,9 @@ struct iommu_table {
> struct iommu_pool large_pool;
> struct iommu_pool pools[IOMMU_NR_POOLS];
> unsigned long *it_map; /* A simple allocation bitmap for now */
> +#ifdef CONFIG_IOMMU_API
> + struct iommu_group *it_group;
> +#endif
> };
>
> struct scatterlist;
> @@ -98,6 +101,8 @@ extern void iommu_free_table(struct iommu_table *tbl, const char *node_name);
> */
> extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
> int nid);
> +extern void iommu_register_group(struct iommu_table * tbl,
> + int domain_number, unsigned long pe_num);
>
> extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
> struct scatterlist *sglist, int nelems,
> @@ -147,5 +152,15 @@ static inline void iommu_restore(void)
> }
> #endif
>
> +/* The API to support IOMMU operations for VFIO */
> +extern long iommu_clear_tce_user_mode(struct iommu_table *tbl,
> + unsigned long ioba, unsigned long tce_value,
> + unsigned long npages);
> +extern long iommu_put_tce_user_mode(struct iommu_table *tbl,
> + unsigned long ioba, unsigned long tce);
> +
> +extern void iommu_flush_tce(struct iommu_table *tbl);
> +extern long iommu_lock_table(struct iommu_table *tbl, bool lock);
> +
> #endif /* __KERNEL__ */
> #endif /* _ASM_IOMMU_H */
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index 7c309fe..b4fdabc 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -37,6 +37,7 @@
> #include <linux/fault-inject.h>
> #include <linux/pci.h>
> #include <linux/kvm_host.h>
> +#include <linux/iommu.h>
> #include <asm/io.h>
> #include <asm/prom.h>
> #include <asm/iommu.h>
> @@ -45,6 +46,7 @@
> #include <asm/kdump.h>
> #include <asm/fadump.h>
> #include <asm/vio.h>
> +#include <asm/tce.h>
>
> #define DBG(...)
>
> @@ -707,11 +709,39 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
> return tbl;
> }
>
> +static void group_release(void *iommu_data)
> +{
> + struct iommu_table *tbl = iommu_data;
> + tbl->it_group = NULL;
> +}
> +
> +void iommu_register_group(struct iommu_table * tbl,
> + int domain_number, unsigned long pe_num)
> +{
> + struct iommu_group *grp;
> +
> + grp = iommu_group_alloc();
> + if (IS_ERR(grp)) {
> + pr_info("powerpc iommu api: cannot create new group, err=%ld\n",
> + PTR_ERR(grp));
> + return;
> + }
> + tbl->it_group = grp;
> + iommu_group_set_iommudata(grp, tbl, group_release);
> + iommu_group_set_name(grp, kasprintf(GFP_KERNEL, "domain%d-pe%lx",
> + domain_number, pe_num));
> +}
> +
> void iommu_free_table(struct iommu_table *tbl, const char *node_name)
> {
> unsigned long bitmap_sz;
> unsigned int order;
>
> + if (tbl && tbl->it_group) {
> + iommu_group_put(tbl->it_group);
> + BUG_ON(tbl->it_group);
> + }
> +
> if (!tbl || !tbl->it_map) {
> printk(KERN_ERR "%s: expected TCE map for %s\n", __func__,
> node_name);
> @@ -876,4 +906,317 @@ void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
> {
> }
>
> +static enum dma_data_direction tce_direction(unsigned long tce)
> +{
> + if ((tce & TCE_PCI_READ) && (tce & TCE_PCI_WRITE))
> + return DMA_BIDIRECTIONAL;
> + else if (tce & TCE_PCI_READ)
> + return DMA_TO_DEVICE;
> + else if (tce & TCE_PCI_WRITE)
> + return DMA_FROM_DEVICE;
> + else
> + return DMA_NONE;
> +}
> +
> +void iommu_flush_tce(struct iommu_table *tbl)
> +{
> + /* Flush/invalidate TLB caches if necessary */
> + if (ppc_md.tce_flush)
> + ppc_md.tce_flush(tbl);
> +
> + /* Make sure updates are seen by hardware */
> + mb();
> +}
> +EXPORT_SYMBOL_GPL(iommu_flush_tce);
> +
> +static long tce_clear_param_check(struct iommu_table *tbl,
> + unsigned long ioba, unsigned long tce_value,
> + unsigned long npages)
> +{
> + unsigned long size = npages << IOMMU_PAGE_SHIFT;
> +
> + /* ppc_md.tce_free() does not support any value but 0 */
> + if (tce_value)
> + return -EINVAL;
> +
> + if (ioba & ~IOMMU_PAGE_MASK)
> + return -EINVAL;
> +
> + if ((ioba + size) > ((tbl->it_offset + tbl->it_size)
> + << IOMMU_PAGE_SHIFT))
> + return -EINVAL;
> +
> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
> + return -EINVAL;
> +
> + return 0;
Why do these all return long (vs int)? Is this a POWER-ism?
> +}
> +
> +static long tce_put_param_check(struct iommu_table *tbl,
> + unsigned long ioba, unsigned long tce)
> +{
> + if (!(tce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> + return -EINVAL;
> +
> + if (tce & ~(IOMMU_PAGE_MASK | TCE_PCI_WRITE | TCE_PCI_READ))
> + return -EINVAL;
> +
> + if (ioba & ~IOMMU_PAGE_MASK)
> + return -EINVAL;
> +
> + if ((ioba + IOMMU_PAGE_SIZE) > ((tbl->it_offset + tbl->it_size)
> + << IOMMU_PAGE_SHIFT))
> + return -EINVAL;
> +
> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static long clear_tce(struct iommu_table *tbl,
> + unsigned long entry, unsigned long pages)
> +{
> + unsigned long oldtce;
> + struct page *page;
> + struct iommu_pool *pool;
> +
> + for ( ; pages; --pages, ++entry) {
> + pool = get_pool(tbl, entry);
> + spin_lock(&(pool->lock));
> +
> + oldtce = ppc_md.tce_get(tbl, entry);
> + if (oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)) {
> + ppc_md.tce_free(tbl, entry, 1);
> +
> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
> + WARN_ON(!page);
> + if (page) {
> + if (oldtce & TCE_PCI_WRITE)
> + SetPageDirty(page);
> + put_page(page);
> + }
> + }
> + spin_unlock(&(pool->lock));
> + }
> +
> + return 0;
No non-zero return, make it void?
> +}
> +
> +long iommu_clear_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
> + unsigned long tce_value, unsigned long npages)
> +{
> + long ret;
> + unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
> +
> + ret = tce_clear_param_check(tbl, ioba, tce_value, npages);
> + if (!ret)
> + ret = clear_tce(tbl, entry, npages);
> +
> + if (ret < 0)
> + pr_err("iommu_tce: %s failed ioba=%lx, tce_value=%lx ret=%ld\n",
> + __func__, ioba, tce_value, ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_clear_tce_user_mode);
> +
> +/* hwaddr is a virtual address here, tce_build converts it to physical */
> +static long do_tce_build(struct iommu_table *tbl, unsigned long entry,
> + unsigned long hwaddr, enum dma_data_direction direction)
> +{
> + long ret = -EBUSY;
> + unsigned long oldtce;
> + struct iommu_pool *pool = get_pool(tbl, entry);
> +
> + spin_lock(&(pool->lock));
> +
> + oldtce = ppc_md.tce_get(tbl, entry);
> + /* Add new entry if it is not busy */
> + if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> + ret = ppc_md.tce_build(tbl, entry, 1, hwaddr, direction, NULL);
> +
> + spin_unlock(&(pool->lock));
> +
> + if (unlikely(ret))
> + pr_err("iommu_tce: %s failed on hwaddr=%lx ioba=%lx kva=%lx ret=%ld\n",
> + __func__, hwaddr, entry << IOMMU_PAGE_SHIFT,
> + hwaddr, ret);
> +
> + return ret;
> +}
> +
> +static int put_tce_user_mode(struct iommu_table *tbl, unsigned long entry,
> + unsigned long tce)
> +{
> + int ret;
Now we're back to returning ints.
> + struct page *page = NULL;
> + unsigned long hwaddr, offset = tce & IOMMU_PAGE_MASK & ~PAGE_MASK;
> + enum dma_data_direction direction = tce_direction(tce);
> +
> + ret = get_user_pages_fast(tce & PAGE_MASK, 1,
> + direction != DMA_TO_DEVICE, &page);
> + if (unlikely(ret != 1)) {
> + pr_err("iommu_tce: get_user_pages_fast failed tce=%lx ioba=%lx ret=%d\n",
> + tce, entry << IOMMU_PAGE_SHIFT, ret);
> + return -EFAULT;
> + }
> + hwaddr = (unsigned long) page_address(page) + offset;
> +
> + ret = do_tce_build(tbl, entry, hwaddr, direction);
> + if (ret)
> + put_page(page);
> +
> + return ret;
> +}
> +
> +long iommu_put_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
> + unsigned long tce)
> +{
> + long ret;
Oops, back to longs.
> + unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
> +
> + ret = tce_put_param_check(tbl, ioba, tce);
> + if (!ret)
> + ret = put_tce_user_mode(tbl, entry, tce);
> +
> + if (ret < 0)
> + pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",
> + __func__, ioba, tce, ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_put_tce_user_mode);
> +
> +/*
> + * Helpers to do locked pages accounting.
> + * Called from ioctl so down_write_trylock is not necessary.
> + */
> +static void lock_acct(long npage)
> +{
> + if (!current->mm)
> + return; /* process exited */
> +
> + down_write(¤t->mm->mmap_sem);
> + current->mm->locked_vm += npage;
> + up_write(¤t->mm->mmap_sem);
> +}
> +
> +/*
> + * iommu_lock_table - Start/stop using the table by VFIO
> + * @tbl: Pointer to the IOMMU table
> + * @lock: true when VFIO starts using the table
> + */
> +long iommu_lock_table(struct iommu_table *tbl, bool lock)
> +{
> + unsigned long sz = (tbl->it_size + 7) >> 3;
> + unsigned long locked, lock_limit;
> +
> + if (lock) {
> + /*
> + * Account for locked pages
> + * The worst case is when every IOMMU page
> + * is mapped to separate system page
> + */
> + locked = current->mm->locked_vm + tbl->it_size;
> + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
> + pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
> + rlimit(RLIMIT_MEMLOCK));
> + return -ENOMEM;
> + }
> +
> + 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;
> + }
> +
> + lock_acct(tbl->it_size);
> + memset(tbl->it_map, 0xff, sz);
> + }
> +
> + /* Clear TCE table */
> + clear_tce(tbl, tbl->it_offset, tbl->it_size);
> +
> + if (!lock) {
> + lock_acct(-tbl->it_size);
> + memset(tbl->it_map, 0, sz);
> +
> + /* Restore bit#0 set by iommu_init_table() */
> + if (tbl->it_offset == 0)
> + set_bit(0, tbl->it_map);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iommu_lock_table);
> +
> +int iommu_add_device(struct device *dev)
> +{
> + struct iommu_table *tbl;
> + int ret = 0;
> +
> + if (WARN_ON(dev->iommu_group)) {
> + pr_warn("iommu_tce: device %s is already in iommu group %d, skipping\n",
> + dev_name(dev),
> + iommu_group_id(dev->iommu_group));
> + return -EBUSY;
> + }
> +
> + tbl = get_iommu_table_base(dev);
> + if (!tbl) {
> + pr_debug("iommu_tce: skipping device %s with no tbl\n",
> + dev_name(dev));
> + return 0;
> + }
> +
> + pr_debug("iommu_tce: adding %s to iommu group %d\n",
> + dev_name(dev), iommu_group_id(tbl->it_group));
> +
> + ret = iommu_group_add_device(tbl->it_group, dev);
> + if (ret < 0)
> + pr_err("iommu_tce: %s has not been added, ret=%d\n",
> + dev_name(dev), ret);
> +
> + return ret;
> +}
> +
> +void iommu_del_device(struct device *dev)
> +{
> + iommu_group_remove_device(dev);
> +}
> +
> +static int iommu_bus_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + return iommu_add_device(dev);
> + case BUS_NOTIFY_DEL_DEVICE:
> + iommu_del_device(dev);
> + return 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static struct notifier_block tce_iommu_bus_nb = {
> + .notifier_call = iommu_bus_notifier,
> +};
> +
> +static int __init tce_iommu_init(void)
> +{
> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> +
> + bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> + return 0;
> +}
> +
> +arch_initcall(tce_iommu_init);
> +
> #endif /* CONFIG_IOMMU_API */
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 8e90e89..04dbc49 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -522,6 +522,7 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
> | TCE_PCI_SWINV_PAIR;
> }
> iommu_init_table(tbl, phb->hose->node);
> + iommu_register_group(tbl, pci_domain_nr(pe->pbus), pe->pe_number);
>
> if (pe->pdev)
> set_iommu_table_base(&pe->pdev->dev, tbl);
> diff --git a/arch/powerpc/platforms/powernv/pci-p5ioc2.c b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> index abe6780..7ce75b0 100644
> --- a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> +++ b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
> @@ -87,8 +87,11 @@ static void pnv_pci_init_p5ioc2_msis(struct pnv_phb *phb) { }
> static void pnv_pci_p5ioc2_dma_dev_setup(struct pnv_phb *phb,
> struct pci_dev *pdev)
> {
> - if (phb->p5ioc2.iommu_table.it_map == NULL)
> + if (phb->p5ioc2.iommu_table.it_map == NULL) {
> iommu_init_table(&phb->p5ioc2.iommu_table, phb->hose->node);
> + iommu_register_group(&phb->p5ioc2.iommu_table,
> + pci_domain_nr(phb->hose->bus), phb->opal_id);
> + }
>
> set_iommu_table_base(&pdev->dev, &phb->p5ioc2.iommu_table);
> }
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index f60a188..d112701 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -20,6 +20,7 @@
> #include <linux/irq.h>
> #include <linux/io.h>
> #include <linux/msi.h>
> +#include <linux/iommu.h>
>
> #include <asm/sections.h>
> #include <asm/io.h>
> @@ -503,6 +504,7 @@ static struct iommu_table *pnv_pci_setup_bml_iommu(struct pci_controller *hose)
> pnv_pci_setup_iommu_table(tbl, __va(be64_to_cpup(basep)),
> be32_to_cpup(sizep), 0);
> iommu_init_table(tbl, hose->node);
> + iommu_register_group(tbl, pci_domain_nr(hose->bus), 0);
>
> /* Deal with SW invalidated TCEs when needed (BML way) */
> swinvp = of_get_property(hose->dn, "linux,tce-sw-invalidate-info",
> @@ -631,3 +633,4 @@ void __init pnv_pci_init(void)
> ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
> #endif
> }
> +
stray newline
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index e39f9db..ce6b186 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -187,4 +187,12 @@ config EXYNOS_IOMMU_DEBUG
>
> Say N unless you need kernel log message for IOMMU debugging
>
> +config SPAPR_TCE_IOMMU
> + bool "sPAPR TCE IOMMU Support"
> + depends on PPC_POWERNV
> + select IOMMU_API
> + help
> + Enables bits of IOMMU API required by VFIO. The iommu_ops is
> + still not implemented.
> +
> endif # IOMMU_SUPPORT
Looks mostly ok to me other than the minor notes. As mentioned
previously, this one needs to go in through power maintainers before I
can include 2/2 . Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 2/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alex Williamson @ 2013-02-11 22:17 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360583672-21924-3-git-send-email-aik@ozlabs.ru>
On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
> VFIO implements platform independent stuff such as
> a PCI driver, BAR access (via read/write on a file descriptor
> or direct mapping when possible) and IRQ signaling.
>
> The platform dependent part includes IOMMU initialization
> and handling. This patch implements an IOMMU driver for VFIO
> which does mapping/unmapping pages for the guest IO and
> provides information about DMA window (required by a POWERPC
> guest).
>
> The counterpart in QEMU is required to support this functionality.
Revision info would be great here too.
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> drivers/vfio/Kconfig | 6 +
> drivers/vfio/Makefile | 1 +
> drivers/vfio/vfio_iommu_spapr_tce.c | 269 +++++++++++++++++++++++++++++++++++
> include/uapi/linux/vfio.h | 31 ++++
> 4 files changed, 307 insertions(+)
> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 7cd5dec..b464687 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
> depends on VFIO
> default n
>
> +config VFIO_IOMMU_SPAPR_TCE
> + tristate
> + depends on VFIO && SPAPR_TCE_IOMMU
> + default n
> +
> menuconfig VFIO
> tristate "VFIO Non-Privileged userspace driver framework"
> depends on IOMMU_API
> select VFIO_IOMMU_TYPE1 if X86
> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
> help
> VFIO provides a framework for secure userspace device drivers.
> See Documentation/vfio.txt for more details.
> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> index 2398d4a..72bfabc 100644
> --- a/drivers/vfio/Makefile
> +++ b/drivers/vfio/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_VFIO) += vfio.o
> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> obj-$(CONFIG_VFIO_PCI) += pci/
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> new file mode 100644
> index 0000000..9b3fa88
> --- /dev/null
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -0,0 +1,269 @@
> +/*
> + * VFIO: IOMMU DMA mapping support for TCE on POWER
> + *
> + * Copyright (C) 2012 IBM Corp. All rights reserved.
2013 now
> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Derived from original vfio_iommu_type1.c:
> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> + * Author: Alex Williamson <alex.williamson@redhat.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/err.h>
> +#include <linux/vfio.h>
> +#include <asm/iommu.h>
> +#include <asm/tce.h>
> +
> +#define DRIVER_VERSION "0.1"
> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> +
> +static void tce_iommu_detach_group(void *iommu_data,
> + struct iommu_group *iommu_group);
> +
> +/*
> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
> + *
> + * This code handles mapping and unmapping of user data buffers
> + * into DMA'ble space using the IOMMU
> + */
> +
> +/*
> + * The container descriptor supports only a single group per container.
> + * Required by the API as the container is not supplied with the IOMMU group
> + * at the moment of initialization.
> + */
> +struct tce_container {
> + struct mutex lock;
> + struct iommu_table *tbl;
> +};
> +
> +static void *tce_iommu_open(unsigned long arg)
> +{
> + struct tce_container *container;
> +
> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
> + pr_err("tce_vfio: Wrong IOMMU type\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + container = kzalloc(sizeof(*container), GFP_KERNEL);
> + if (!container)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_init(&container->lock);
> +
> + return container;
> +}
> +
> +static void tce_iommu_release(void *iommu_data)
> +{
> + struct tce_container *container = iommu_data;
> +
> + WARN_ON(container->tbl && !container->tbl->it_group);
> + if (container->tbl && container->tbl->it_group)
> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
> +
> + mutex_destroy(&container->lock);
> +
> + kfree(container);
> +}
> +
> +static long tce_iommu_ioctl(void *iommu_data,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct tce_container *container = iommu_data;
> + unsigned long minsz;
> + long ret;
> +
> + switch (cmd) {
> + case VFIO_CHECK_EXTENSION:
> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
> +
> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
> + struct vfio_iommu_spapr_tce_info info;
> + struct iommu_table *tbl = container->tbl;
> +
> + if (WARN_ON(!tbl))
> + return -ENXIO;
> +
> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
> + dma32_window_size);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
> + info.flags = 0;
> +
> + if (copy_to_user((void __user *)arg, &info, minsz))
> + return -EFAULT;
> +
> + return 0;
> + }
> + case VFIO_IOMMU_MAP_DMA: {
> + vfio_iommu_spapr_tce_dma_map param;
> + struct iommu_table *tbl = container->tbl;
> + unsigned long tce;
> +
> + if (WARN_ON(!tbl))
> + return -ENXIO;
> +
> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
> +
> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (param.argsz < minsz)
> + return -EINVAL;
> +
> + if (param.flags & ~(VFIO_DMA_MAP_FLAG_READ |
> + VFIO_DMA_MAP_FLAG_WRITE))
> + return -EINVAL;
> +
> + if ((param.size & ~IOMMU_PAGE_MASK) ||
> + (param.vaddr & ~IOMMU_PAGE_MASK))
> + return -EINVAL;
> +
> + /* TODO: support multiple TCEs */
> + if (param.size != IOMMU_PAGE_SIZE) {
Ouch, ioctl per page...
> + pr_err("VFIO map on POWER supports only %lu page size\n",
> + IOMMU_PAGE_SIZE);
> + return -EINVAL;
> + }
> +
> + /* iova is checked by the IOMMU API */
> + tce = param.vaddr;
> + if (param.flags & VFIO_DMA_MAP_FLAG_READ)
> + tce |= TCE_PCI_READ;
> + if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
> + tce |= TCE_PCI_WRITE;
> +
> + ret = iommu_put_tce_user_mode(tbl, param.iova, tce);
> + iommu_flush_tce(tbl);
> +
> + return ret;
> + }
> + case VFIO_IOMMU_UNMAP_DMA: {
> + vfio_iommu_spapr_tce_dma_unmap param;
> + struct iommu_table *tbl = container->tbl;
> +
> + if (WARN_ON(!tbl))
> + return -ENXIO;
> +
> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
> +
> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (param.argsz < minsz)
> + return -EINVAL;
> +
> + /* No flag is supported now */
> + if (param.flags)
> + return -EINVAL;
> +
> + if (param.size & ~IOMMU_PAGE_MASK)
> + return -EINVAL;
But you support multi-page unmaps?
> +
> + /* iova is checked by the IOMMU API */
> + ret = iommu_clear_tce_user_mode(tbl, param.iova, 0,
> + param.size >> IOMMU_PAGE_SHIFT);
> + iommu_flush_tce(tbl);
> +
> + return ret;
> + }
> + }
> +
> + return -ENOTTY;
> +}
> +
> +static int tce_iommu_attach_group(void *iommu_data,
> + struct iommu_group *iommu_group)
> +{
> + int ret;
> + struct tce_container *container = iommu_data;
> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> +
> + BUG_ON(!tbl);
> + mutex_lock(&container->lock);
> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
> + iommu_group_id(iommu_group), iommu_group);
> + if (container->tbl) {
> + pr_warn("tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
> + iommu_group_id(container->tbl->it_group),
> + iommu_group_id(iommu_group));
> + mutex_unlock(&container->lock);
> + return -EBUSY;
> + }
> +
> + container->tbl = tbl;
> + ret = iommu_lock_table(tbl, true);
Bug, container->tbl is set regardless of iommu_lock_table.
Ok, so now we're checking rlimits and handling page accounting on
VFIO_GROUP_SET_CONTAINER to avoid any overhead at map/unmap. How can
the user learn tbl->it_size to set their locked page limit prior to
this? It's available from GET_INFO, but there's a chicken and egg
problem that to get it there you have to get past this, which means
you're already ok. Maybe it's in sysfs somewhere already or it could be
exposed in the iommu group like the name attribute. Otherwise we might
consider doing locking on first mapping. Thanks,
Alex
> + mutex_unlock(&container->lock);
> +
> + return ret;
> +}
> +
> +static void tce_iommu_detach_group(void *iommu_data,
> + struct iommu_group *iommu_group)
> +{
> + struct tce_container *container = iommu_data;
> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> +
> + BUG_ON(!tbl);
> + mutex_lock(&container->lock);
> + if (tbl != container->tbl) {
> + pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n",
> + iommu_group_id(iommu_group),
> + iommu_group_id(tbl->it_group));
> + } else {
> +
> + pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
> + iommu_group_id(iommu_group), iommu_group);
> +
> + container->tbl = NULL;
> + iommu_lock_table(tbl, false);
> + }
> + mutex_unlock(&container->lock);
> +}
> +
> +const struct vfio_iommu_driver_ops tce_iommu_driver_ops = {
> + .name = "iommu-vfio-powerpc",
> + .owner = THIS_MODULE,
> + .open = tce_iommu_open,
> + .release = tce_iommu_release,
> + .ioctl = tce_iommu_ioctl,
> + .attach_group = tce_iommu_attach_group,
> + .detach_group = tce_iommu_detach_group,
> +};
> +
> +static int __init tce_iommu_init(void)
> +{
> + return vfio_register_iommu_driver(&tce_iommu_driver_ops);
> +}
> +
> +static void __exit tce_iommu_cleanup(void)
> +{
> + vfio_unregister_iommu_driver(&tce_iommu_driver_ops);
> +}
> +
> +module_init(tce_iommu_init);
> +module_exit(tce_iommu_cleanup);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 4758d1b..ea9a9a7 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -22,6 +22,7 @@
> /* Extensions */
>
> #define VFIO_TYPE1_IOMMU 1
> +#define VFIO_SPAPR_TCE_IOMMU 2
>
> /*
> * The IOCTL interface is designed for extensibility by embedding the
> @@ -365,4 +366,34 @@ struct vfio_iommu_type1_dma_unmap {
>
> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
>
> +/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
> +
> +/*
> + * The SPAPR TCE info struct provides the information about the PCI bus
> + * address ranges available for DMA, these values are programmed into
> + * the hardware so the guest has to know that information.
> + *
> + * The DMA 32 bit window start is an absolute PCI bus address.
> + * The IOVA address passed via map/unmap ioctls are absolute PCI bus
> + * addresses too so the window works as a filter rather than an offset
> + * for IOVA addresses.
> + *
> + * A flag will need to be added if other page sizes are supported,
> + * so as defined here, it is always 4k.
> + */
> +struct vfio_iommu_spapr_tce_info {
> + __u32 argsz;
> + __u32 flags; /* reserved for future use */
> + __u32 dma32_window_start; /* 32 bit window start (bytes) */
> + __u32 dma32_window_size; /* 32 bit window size (bytes) */
> +};
> +
> +#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> +
> +/* Reuse type1 map/unmap structs as they are the same at the moment */
> +typedef struct vfio_iommu_type1_dma_map vfio_iommu_spapr_tce_dma_map;
> +typedef struct vfio_iommu_type1_dma_unmap vfio_iommu_spapr_tce_dma_unmap;
> +
> +/* ***************************************************************** */
> +
> #endif /* _UAPIVFIO_H */
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: enabled on powernv platform
From: Alexey Kardashevskiy @ 2013-02-11 23:19 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360621003.12392.117.camel@bling.home>
On 12/02/13 09:16, Alex Williamson wrote:
> On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
>> This patch initializes IOMMU groups based on the IOMMU
>> configuration discovered during the PCI scan on POWERNV
>> (POWER non virtualized) platform. The IOMMU groups are
>> to be used later by VFIO driver (PCI pass through).
>>
>> It also implements an API for mapping/unmapping pages for
>> guest PCI drivers and providing DMA window properties.
>> This API is going to be used later by QEMU-VFIO to handle
>> h_put_tce hypercalls from the KVM guest.
>>
>> The iommu_put_tce_user_mode() does only a single page mapping
>> as an API for adding many mappings at once is going to be
>> added later.
>>
>> Although this driver has been tested only on the POWERNV
>> platform, it should work on any platform which supports
>> TCE tables. As h_put_tce hypercall is received by the host
>> kernel and processed by the QEMU (what involves calling
>> the host kernel again), performance is not the best -
>> circa 220MB/s on 10Gb ethernet network.
>>
>> To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config
>> option and configure VFIO as required.
>>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Yay, it's not dead! ;)
No, still alive :) I have been implementing real mode and multiple pages
support (for real mode too) so I changed the interface a bit as I did not
realize some bits from the powerpc arch.
> I'd love some kind of changelog to know what to look for in here,
> especially given 2mo since the last version.
There is no serious change actually.
>> ---
>> arch/powerpc/include/asm/iommu.h | 15 ++
>> arch/powerpc/kernel/iommu.c | 343 +++++++++++++++++++++++++++
>> arch/powerpc/platforms/powernv/pci-ioda.c | 1 +
>> arch/powerpc/platforms/powernv/pci-p5ioc2.c | 5 +-
>> arch/powerpc/platforms/powernv/pci.c | 3 +
>> drivers/iommu/Kconfig | 8 +
>> 6 files changed, 374 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
>> index cbfe678..900294b 100644
>> --- a/arch/powerpc/include/asm/iommu.h
>> +++ b/arch/powerpc/include/asm/iommu.h
>> @@ -76,6 +76,9 @@ struct iommu_table {
>> struct iommu_pool large_pool;
>> struct iommu_pool pools[IOMMU_NR_POOLS];
>> unsigned long *it_map; /* A simple allocation bitmap for now */
>> +#ifdef CONFIG_IOMMU_API
>> + struct iommu_group *it_group;
>> +#endif
>> };
>>
>> struct scatterlist;
>> @@ -98,6 +101,8 @@ extern void iommu_free_table(struct iommu_table *tbl, const char *node_name);
>> */
>> extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
>> int nid);
>> +extern void iommu_register_group(struct iommu_table * tbl,
>> + int domain_number, unsigned long pe_num);
>>
>> extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
>> struct scatterlist *sglist, int nelems,
>> @@ -147,5 +152,15 @@ static inline void iommu_restore(void)
>> }
>> #endif
>>
>> +/* The API to support IOMMU operations for VFIO */
>> +extern long iommu_clear_tce_user_mode(struct iommu_table *tbl,
>> + unsigned long ioba, unsigned long tce_value,
>> + unsigned long npages);
>> +extern long iommu_put_tce_user_mode(struct iommu_table *tbl,
>> + unsigned long ioba, unsigned long tce);
>> +
>> +extern void iommu_flush_tce(struct iommu_table *tbl);
>> +extern long iommu_lock_table(struct iommu_table *tbl, bool lock);
>> +
>> #endif /* __KERNEL__ */
>> #endif /* _ASM_IOMMU_H */
>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> index 7c309fe..b4fdabc 100644
>> --- a/arch/powerpc/kernel/iommu.c
>> +++ b/arch/powerpc/kernel/iommu.c
>> @@ -37,6 +37,7 @@
>> #include <linux/fault-inject.h>
>> #include <linux/pci.h>
>> #include <linux/kvm_host.h>
>> +#include <linux/iommu.h>
>> #include <asm/io.h>
>> #include <asm/prom.h>
>> #include <asm/iommu.h>
>> @@ -45,6 +46,7 @@
>> #include <asm/kdump.h>
>> #include <asm/fadump.h>
>> #include <asm/vio.h>
>> +#include <asm/tce.h>
>>
>> #define DBG(...)
>>
>> @@ -707,11 +709,39 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
>> return tbl;
>> }
>>
>> +static void group_release(void *iommu_data)
>> +{
>> + struct iommu_table *tbl = iommu_data;
>> + tbl->it_group = NULL;
>> +}
>> +
>> +void iommu_register_group(struct iommu_table * tbl,
>> + int domain_number, unsigned long pe_num)
>> +{
>> + struct iommu_group *grp;
>> +
>> + grp = iommu_group_alloc();
>> + if (IS_ERR(grp)) {
>> + pr_info("powerpc iommu api: cannot create new group, err=%ld\n",
>> + PTR_ERR(grp));
>> + return;
>> + }
>> + tbl->it_group = grp;
>> + iommu_group_set_iommudata(grp, tbl, group_release);
>> + iommu_group_set_name(grp, kasprintf(GFP_KERNEL, "domain%d-pe%lx",
>> + domain_number, pe_num));
>> +}
>> +
>> void iommu_free_table(struct iommu_table *tbl, const char *node_name)
>> {
>> unsigned long bitmap_sz;
>> unsigned int order;
>>
>> + if (tbl && tbl->it_group) {
>> + iommu_group_put(tbl->it_group);
>> + BUG_ON(tbl->it_group);
>> + }
>> +
>> if (!tbl || !tbl->it_map) {
>> printk(KERN_ERR "%s: expected TCE map for %s\n", __func__,
>> node_name);
>> @@ -876,4 +906,317 @@ void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
>> {
>> }
>>
>> +static enum dma_data_direction tce_direction(unsigned long tce)
>> +{
>> + if ((tce & TCE_PCI_READ) && (tce & TCE_PCI_WRITE))
>> + return DMA_BIDIRECTIONAL;
>> + else if (tce & TCE_PCI_READ)
>> + return DMA_TO_DEVICE;
>> + else if (tce & TCE_PCI_WRITE)
>> + return DMA_FROM_DEVICE;
>> + else
>> + return DMA_NONE;
>> +}
>> +
>> +void iommu_flush_tce(struct iommu_table *tbl)
>> +{
>> + /* Flush/invalidate TLB caches if necessary */
>> + if (ppc_md.tce_flush)
>> + ppc_md.tce_flush(tbl);
>> +
>> + /* Make sure updates are seen by hardware */
>> + mb();
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_flush_tce);
>> +
>> +static long tce_clear_param_check(struct iommu_table *tbl,
>> + unsigned long ioba, unsigned long tce_value,
>> + unsigned long npages)
>> +{
>> + unsigned long size = npages << IOMMU_PAGE_SHIFT;
>> +
>> + /* ppc_md.tce_free() does not support any value but 0 */
>> + if (tce_value)
>> + return -EINVAL;
>> +
>> + if (ioba & ~IOMMU_PAGE_MASK)
>> + return -EINVAL;
>> +
>> + if ((ioba + size) > ((tbl->it_offset + tbl->it_size)
>> + << IOMMU_PAGE_SHIFT))
>> + return -EINVAL;
>> +
>> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
>> + return -EINVAL;
>> +
>> + return 0;
>
> Why do these all return long (vs int)? Is this a POWER-ism?
No, not really but yeah, I picked it in powerpc code :) I tried to keep
them "long" but I noticed "int" below so what is the rule? Change all to int?
>> +}
>> +
>> +static long tce_put_param_check(struct iommu_table *tbl,
>> + unsigned long ioba, unsigned long tce)
>> +{
>> + if (!(tce & (TCE_PCI_WRITE | TCE_PCI_READ)))
>> + return -EINVAL;
>> +
>> + if (tce & ~(IOMMU_PAGE_MASK | TCE_PCI_WRITE | TCE_PCI_READ))
>> + return -EINVAL;
>> +
>> + if (ioba & ~IOMMU_PAGE_MASK)
>> + return -EINVAL;
>> +
>> + if ((ioba + IOMMU_PAGE_SIZE) > ((tbl->it_offset + tbl->it_size)
>> + << IOMMU_PAGE_SHIFT))
>> + return -EINVAL;
>> +
>> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static long clear_tce(struct iommu_table *tbl,
>> + unsigned long entry, unsigned long pages)
>> +{
>> + unsigned long oldtce;
>> + struct page *page;
>> + struct iommu_pool *pool;
>> +
>> + for ( ; pages; --pages, ++entry) {
>> + pool = get_pool(tbl, entry);
>> + spin_lock(&(pool->lock));
>> +
>> + oldtce = ppc_md.tce_get(tbl, entry);
>> + if (oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)) {
>> + ppc_md.tce_free(tbl, entry, 1);
>> +
>> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
>> + WARN_ON(!page);
>> + if (page) {
>> + if (oldtce & TCE_PCI_WRITE)
>> + SetPageDirty(page);
>> + put_page(page);
>> + }
>> + }
>> + spin_unlock(&(pool->lock));
>> + }
>> +
>> + return 0;
>
> No non-zero return, make it void?
ah, ok. The prototype will change for real mode either way, it will get a
"realmode" flag and become able to fail (which will switch the virtual mode).
>> +}
>> +
>> +long iommu_clear_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
>> + unsigned long tce_value, unsigned long npages)
>> +{
>> + long ret;
>> + unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
>> +
>> + ret = tce_clear_param_check(tbl, ioba, tce_value, npages);
>> + if (!ret)
>> + ret = clear_tce(tbl, entry, npages);
>> +
>> + if (ret < 0)
>> + pr_err("iommu_tce: %s failed ioba=%lx, tce_value=%lx ret=%ld\n",
>> + __func__, ioba, tce_value, ret);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_clear_tce_user_mode);
>> +
>> +/* hwaddr is a virtual address here, tce_build converts it to physical */
>> +static long do_tce_build(struct iommu_table *tbl, unsigned long entry,
>> + unsigned long hwaddr, enum dma_data_direction direction)
>> +{
>> + long ret = -EBUSY;
>> + unsigned long oldtce;
>> + struct iommu_pool *pool = get_pool(tbl, entry);
>> +
>> + spin_lock(&(pool->lock));
>> +
>> + oldtce = ppc_md.tce_get(tbl, entry);
>> + /* Add new entry if it is not busy */
>> + if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
>> + ret = ppc_md.tce_build(tbl, entry, 1, hwaddr, direction, NULL);
>> +
>> + spin_unlock(&(pool->lock));
>> +
>> + if (unlikely(ret))
>> + pr_err("iommu_tce: %s failed on hwaddr=%lx ioba=%lx kva=%lx ret=%ld\n",
>> + __func__, hwaddr, entry << IOMMU_PAGE_SHIFT,
>> + hwaddr, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int put_tce_user_mode(struct iommu_table *tbl, unsigned long entry,
>> + unsigned long tce)
>> +{
>> + int ret;
>
> Now we're back to returning ints.
>
>> + struct page *page = NULL;
>> + unsigned long hwaddr, offset = tce & IOMMU_PAGE_MASK & ~PAGE_MASK;
>> + enum dma_data_direction direction = tce_direction(tce);
>> +
>> + ret = get_user_pages_fast(tce & PAGE_MASK, 1,
>> + direction != DMA_TO_DEVICE, &page);
>> + if (unlikely(ret != 1)) {
>> + pr_err("iommu_tce: get_user_pages_fast failed tce=%lx ioba=%lx ret=%d\n",
>> + tce, entry << IOMMU_PAGE_SHIFT, ret);
>> + return -EFAULT;
>> + }
>> + hwaddr = (unsigned long) page_address(page) + offset;
>> +
>> + ret = do_tce_build(tbl, entry, hwaddr, direction);
>> + if (ret)
>> + put_page(page);
>> +
>> + return ret;
>> +}
>> +
>> +long iommu_put_tce_user_mode(struct iommu_table *tbl, unsigned long ioba,
>> + unsigned long tce)
>> +{
>> + long ret;
>
> Oops, back to longs.
>
>> + unsigned long entry = ioba >> IOMMU_PAGE_SHIFT;
>> +
>> + ret = tce_put_param_check(tbl, ioba, tce);
>> + if (!ret)
>> + ret = put_tce_user_mode(tbl, entry, tce);
>> +
>> + if (ret < 0)
>> + pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",
>> + __func__, ioba, tce, ret);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_put_tce_user_mode);
>> +
>> +/*
>> + * Helpers to do locked pages accounting.
>> + * Called from ioctl so down_write_trylock is not necessary.
>> + */
>> +static void lock_acct(long npage)
>> +{
>> + if (!current->mm)
>> + return; /* process exited */
>> +
>> + down_write(¤t->mm->mmap_sem);
>> + current->mm->locked_vm += npage;
>> + up_write(¤t->mm->mmap_sem);
>> +}
>> +
>> +/*
>> + * iommu_lock_table - Start/stop using the table by VFIO
>> + * @tbl: Pointer to the IOMMU table
>> + * @lock: true when VFIO starts using the table
>> + */
>> +long iommu_lock_table(struct iommu_table *tbl, bool lock)
>> +{
>> + unsigned long sz = (tbl->it_size + 7) >> 3;
>> + unsigned long locked, lock_limit;
>> +
>> + if (lock) {
>> + /*
>> + * Account for locked pages
>> + * The worst case is when every IOMMU page
>> + * is mapped to separate system page
>> + */
>> + locked = current->mm->locked_vm + tbl->it_size;
>> + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
>> + pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
>> + rlimit(RLIMIT_MEMLOCK));
>> + return -ENOMEM;
>> + }
>> +
>> + 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;
>> + }
>> +
>> + lock_acct(tbl->it_size);
>> + memset(tbl->it_map, 0xff, sz);
>> + }
>> +
>> + /* Clear TCE table */
>> + clear_tce(tbl, tbl->it_offset, tbl->it_size);
>> +
>> + if (!lock) {
>> + lock_acct(-tbl->it_size);
>> + memset(tbl->it_map, 0, sz);
>> +
>> + /* Restore bit#0 set by iommu_init_table() */
>> + if (tbl->it_offset == 0)
>> + set_bit(0, tbl->it_map);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_lock_table);
>> +
>> +int iommu_add_device(struct device *dev)
>> +{
>> + struct iommu_table *tbl;
>> + int ret = 0;
>> +
>> + if (WARN_ON(dev->iommu_group)) {
>> + pr_warn("iommu_tce: device %s is already in iommu group %d, skipping\n",
>> + dev_name(dev),
>> + iommu_group_id(dev->iommu_group));
>> + return -EBUSY;
>> + }
>> +
>> + tbl = get_iommu_table_base(dev);
>> + if (!tbl) {
>> + pr_debug("iommu_tce: skipping device %s with no tbl\n",
>> + dev_name(dev));
>> + return 0;
>> + }
>> +
>> + pr_debug("iommu_tce: adding %s to iommu group %d\n",
>> + dev_name(dev), iommu_group_id(tbl->it_group));
>> +
>> + ret = iommu_group_add_device(tbl->it_group, dev);
>> + if (ret < 0)
>> + pr_err("iommu_tce: %s has not been added, ret=%d\n",
>> + dev_name(dev), ret);
>> +
>> + return ret;
>> +}
>> +
>> +void iommu_del_device(struct device *dev)
>> +{
>> + iommu_group_remove_device(dev);
>> +}
>> +
>> +static int iommu_bus_notifier(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct device *dev = data;
>> +
>> + switch (action) {
>> + case BUS_NOTIFY_ADD_DEVICE:
>> + return iommu_add_device(dev);
>> + case BUS_NOTIFY_DEL_DEVICE:
>> + iommu_del_device(dev);
>> + return 0;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static struct notifier_block tce_iommu_bus_nb = {
>> + .notifier_call = iommu_bus_notifier,
>> +};
>> +
>> +static int __init tce_iommu_init(void)
>> +{
>> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
>> +
>> + bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
>> + return 0;
>> +}
>> +
>> +arch_initcall(tce_iommu_init);
>> +
>> #endif /* CONFIG_IOMMU_API */
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index 8e90e89..04dbc49 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -522,6 +522,7 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
>> | TCE_PCI_SWINV_PAIR;
>> }
>> iommu_init_table(tbl, phb->hose->node);
>> + iommu_register_group(tbl, pci_domain_nr(pe->pbus), pe->pe_number);
>>
>> if (pe->pdev)
>> set_iommu_table_base(&pe->pdev->dev, tbl);
>> diff --git a/arch/powerpc/platforms/powernv/pci-p5ioc2.c b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
>> index abe6780..7ce75b0 100644
>> --- a/arch/powerpc/platforms/powernv/pci-p5ioc2.c
>> +++ b/arch/powerpc/platforms/powernv/pci-p5ioc2.c
>> @@ -87,8 +87,11 @@ static void pnv_pci_init_p5ioc2_msis(struct pnv_phb *phb) { }
>> static void pnv_pci_p5ioc2_dma_dev_setup(struct pnv_phb *phb,
>> struct pci_dev *pdev)
>> {
>> - if (phb->p5ioc2.iommu_table.it_map == NULL)
>> + if (phb->p5ioc2.iommu_table.it_map == NULL) {
>> iommu_init_table(&phb->p5ioc2.iommu_table, phb->hose->node);
>> + iommu_register_group(&phb->p5ioc2.iommu_table,
>> + pci_domain_nr(phb->hose->bus), phb->opal_id);
>> + }
>>
>> set_iommu_table_base(&pdev->dev, &phb->p5ioc2.iommu_table);
>> }
>> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
>> index f60a188..d112701 100644
>> --- a/arch/powerpc/platforms/powernv/pci.c
>> +++ b/arch/powerpc/platforms/powernv/pci.c
>> @@ -20,6 +20,7 @@
>> #include <linux/irq.h>
>> #include <linux/io.h>
>> #include <linux/msi.h>
>> +#include <linux/iommu.h>
>>
>> #include <asm/sections.h>
>> #include <asm/io.h>
>> @@ -503,6 +504,7 @@ static struct iommu_table *pnv_pci_setup_bml_iommu(struct pci_controller *hose)
>> pnv_pci_setup_iommu_table(tbl, __va(be64_to_cpup(basep)),
>> be32_to_cpup(sizep), 0);
>> iommu_init_table(tbl, hose->node);
>> + iommu_register_group(tbl, pci_domain_nr(hose->bus), 0);
>>
>> /* Deal with SW invalidated TCEs when needed (BML way) */
>> swinvp = of_get_property(hose->dn, "linux,tce-sw-invalidate-info",
>> @@ -631,3 +633,4 @@ void __init pnv_pci_init(void)
>> ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
>> #endif
>> }
>> +
>
> stray newline
>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index e39f9db..ce6b186 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -187,4 +187,12 @@ config EXYNOS_IOMMU_DEBUG
>>
>> Say N unless you need kernel log message for IOMMU debugging
>>
>> +config SPAPR_TCE_IOMMU
>> + bool "sPAPR TCE IOMMU Support"
>> + depends on PPC_POWERNV
>> + select IOMMU_API
>> + help
>> + Enables bits of IOMMU API required by VFIO. The iommu_ops is
>> + still not implemented.
>> +
>> endif # IOMMU_SUPPORT
>
> Looks mostly ok to me other than the minor notes. As mentioned
> previously, this one needs to go in through power maintainers before I
> can include 2/2 . Thanks,
>
> Alex
>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 2/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alexey Kardashevskiy @ 2013-02-11 23:45 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1360621078.12392.119.camel@bling.home>
On 12/02/13 09:17, Alex Williamson wrote:
> On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
>> VFIO implements platform independent stuff such as
>> a PCI driver, BAR access (via read/write on a file descriptor
>> or direct mapping when possible) and IRQ signaling.
>>
>> The platform dependent part includes IOMMU initialization
>> and handling. This patch implements an IOMMU driver for VFIO
>> which does mapping/unmapping pages for the guest IO and
>> provides information about DMA window (required by a POWERPC
>> guest).
>>
>> The counterpart in QEMU is required to support this functionality.
>
> Revision info would be great here too.
>
>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> drivers/vfio/Kconfig | 6 +
>> drivers/vfio/Makefile | 1 +
>> drivers/vfio/vfio_iommu_spapr_tce.c | 269 +++++++++++++++++++++++++++++++++++
>> include/uapi/linux/vfio.h | 31 ++++
>> 4 files changed, 307 insertions(+)
>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
>>
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index 7cd5dec..b464687 100644
>> --- a/drivers/vfio/Kconfig
>> +++ b/drivers/vfio/Kconfig
>> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
>> depends on VFIO
>> default n
>>
>> +config VFIO_IOMMU_SPAPR_TCE
>> + tristate
>> + depends on VFIO && SPAPR_TCE_IOMMU
>> + default n
>> +
>> menuconfig VFIO
>> tristate "VFIO Non-Privileged userspace driver framework"
>> depends on IOMMU_API
>> select VFIO_IOMMU_TYPE1 if X86
>> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
>> help
>> VFIO provides a framework for secure userspace device drivers.
>> See Documentation/vfio.txt for more details.
>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>> index 2398d4a..72bfabc 100644
>> --- a/drivers/vfio/Makefile
>> +++ b/drivers/vfio/Makefile
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_VFIO) += vfio.o
>> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
>> obj-$(CONFIG_VFIO_PCI) += pci/
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> new file mode 100644
>> index 0000000..9b3fa88
>> --- /dev/null
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -0,0 +1,269 @@
>> +/*
>> + * VFIO: IOMMU DMA mapping support for TCE on POWER
>> + *
>> + * Copyright (C) 2012 IBM Corp. All rights reserved.
>
> 2013 now
>
>> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Derived from original vfio_iommu_type1.c:
>> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
>> + * Author: Alex Williamson <alex.williamson@redhat.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/err.h>
>> +#include <linux/vfio.h>
>> +#include <asm/iommu.h>
>> +#include <asm/tce.h>
>> +
>> +#define DRIVER_VERSION "0.1"
>> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
>> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>> +
>> +static void tce_iommu_detach_group(void *iommu_data,
>> + struct iommu_group *iommu_group);
>> +
>> +/*
>> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
>> + *
>> + * This code handles mapping and unmapping of user data buffers
>> + * into DMA'ble space using the IOMMU
>> + */
>> +
>> +/*
>> + * The container descriptor supports only a single group per container.
>> + * Required by the API as the container is not supplied with the IOMMU group
>> + * at the moment of initialization.
>> + */
>> +struct tce_container {
>> + struct mutex lock;
>> + struct iommu_table *tbl;
>> +};
>> +
>> +static void *tce_iommu_open(unsigned long arg)
>> +{
>> + struct tce_container *container;
>> +
>> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
>> + pr_err("tce_vfio: Wrong IOMMU type\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + container = kzalloc(sizeof(*container), GFP_KERNEL);
>> + if (!container)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mutex_init(&container->lock);
>> +
>> + return container;
>> +}
>> +
>> +static void tce_iommu_release(void *iommu_data)
>> +{
>> + struct tce_container *container = iommu_data;
>> +
>> + WARN_ON(container->tbl && !container->tbl->it_group);
>> + if (container->tbl && container->tbl->it_group)
>> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
>> +
>> + mutex_destroy(&container->lock);
>> +
>> + kfree(container);
>> +}
>> +
>> +static long tce_iommu_ioctl(void *iommu_data,
>> + unsigned int cmd, unsigned long arg)
>> +{
>> + struct tce_container *container = iommu_data;
>> + unsigned long minsz;
>> + long ret;
>> +
>> + switch (cmd) {
>> + case VFIO_CHECK_EXTENSION:
>> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
>> +
>> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
>> + struct vfio_iommu_spapr_tce_info info;
>> + struct iommu_table *tbl = container->tbl;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
>> + dma32_window_size);
>> +
>> + if (copy_from_user(&info, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (info.argsz < minsz)
>> + return -EINVAL;
>> +
>> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
>> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
>> + info.flags = 0;
>> +
>> + if (copy_to_user((void __user *)arg, &info, minsz))
>> + return -EFAULT;
>> +
>> + return 0;
>> + }
>> + case VFIO_IOMMU_MAP_DMA: {
>> + vfio_iommu_spapr_tce_dma_map param;
>> + struct iommu_table *tbl = container->tbl;
>> + unsigned long tce;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
>> +
>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (param.argsz < minsz)
>> + return -EINVAL;
>> +
>> + if (param.flags & ~(VFIO_DMA_MAP_FLAG_READ |
>> + VFIO_DMA_MAP_FLAG_WRITE))
>> + return -EINVAL;
>> +
>> + if ((param.size & ~IOMMU_PAGE_MASK) ||
>> + (param.vaddr & ~IOMMU_PAGE_MASK))
>> + return -EINVAL;
>> +
>> + /* TODO: support multiple TCEs */
>> + if (param.size != IOMMU_PAGE_SIZE) {
>
> Ouch, ioctl per page...
Well, there is something to discuss.
On POWER, there is an interface to add multiple pages at once but the guest
does not supply the range of guest phys addresses, it puts some addresses
to a small page (so it is up to 512 pages at once) and passes this address
to the host as a parameter.
I posted another series yesterday but forgot to cc: you :) You can find
them here - http://patchwork.ozlabs.org/patch/219592/ (emulated devices)
and http://patchwork.ozlabs.org/patch/219594/ (vfio). There I convert guest
phys address (real and virtual mode are handled different ways) and call
iommu_put_tce_user_mode() (or analog) in a loop.
Either way, I did some tests with 10Gb card and without real mode stuff it
does 220MB/s, and even if I do multi-tce it won't be faster than ~400MB/s
which is still not enough as the real mode code makes it 1020MB/s. Slower
devices work on the same speed no matter what.
>> + pr_err("VFIO map on POWER supports only %lu page size\n",
>> + IOMMU_PAGE_SIZE);
>> + return -EINVAL;
>> + }
>> +
>> + /* iova is checked by the IOMMU API */
>> + tce = param.vaddr;
>> + if (param.flags & VFIO_DMA_MAP_FLAG_READ)
>> + tce |= TCE_PCI_READ;
>> + if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
>> + tce |= TCE_PCI_WRITE;
>> +
>> + ret = iommu_put_tce_user_mode(tbl, param.iova, tce);
>> + iommu_flush_tce(tbl);
>> +
>> + return ret;
>> + }
>> + case VFIO_IOMMU_UNMAP_DMA: {
>> + vfio_iommu_spapr_tce_dma_unmap param;
>> + struct iommu_table *tbl = container->tbl;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
>> +
>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (param.argsz < minsz)
>> + return -EINVAL;
>> +
>> + /* No flag is supported now */
>> + if (param.flags)
>> + return -EINVAL;
>> +
>> + if (param.size & ~IOMMU_PAGE_MASK)
>> + return -EINVAL;
>
> But you support multi-page unmaps?
Yes, this is a lot easier :)
>> + /* iova is checked by the IOMMU API */
>> + ret = iommu_clear_tce_user_mode(tbl, param.iova, 0,
>> + param.size >> IOMMU_PAGE_SHIFT);
>> + iommu_flush_tce(tbl);
>> +
>> + return ret;
>> + }
>> + }
>> +
>> + return -ENOTTY;
>> +}
>> +
>> +static int tce_iommu_attach_group(void *iommu_data,
>> + struct iommu_group *iommu_group)
>> +{
>> + int ret;
>> + struct tce_container *container = iommu_data;
>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
>> +
>> + BUG_ON(!tbl);
>> + mutex_lock(&container->lock);
>> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
>> + iommu_group_id(iommu_group), iommu_group);
>> + if (container->tbl) {
>> + pr_warn("tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
>> + iommu_group_id(container->tbl->it_group),
>> + iommu_group_id(iommu_group));
>> + mutex_unlock(&container->lock);
>> + return -EBUSY;
>> + }
>> +
>> + container->tbl = tbl;
>> + ret = iommu_lock_table(tbl, true);
>
> Bug, container->tbl is set regardless of iommu_lock_table.
Oops, bug.
> Ok, so now we're checking rlimits and handling page accounting on
> VFIO_GROUP_SET_CONTAINER to avoid any overhead at map/unmap. How can
> the user learn tbl->it_size to set their locked page limit prior to
> this? It's available from GET_INFO, but there's a chicken and egg
> problem that to get it there you have to get past this, which means
> you're already ok. Maybe it's in sysfs somewhere already or it could be
> exposed in the iommu group like the name attribute. Otherwise we might
> consider doing locking on first mapping. Thanks,
GET_INFO is called in the beginning so QEMU will exit right there. No real
work will have been done till that moment so what is the problem?
> Alex
>
>> + mutex_unlock(&container->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static void tce_iommu_detach_group(void *iommu_data,
>> + struct iommu_group *iommu_group)
>> +{
>> + struct tce_container *container = iommu_data;
>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
>> +
>> + BUG_ON(!tbl);
>> + mutex_lock(&container->lock);
>> + if (tbl != container->tbl) {
>> + pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n",
>> + iommu_group_id(iommu_group),
>> + iommu_group_id(tbl->it_group));
>> + } else {
>> +
>> + pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
>> + iommu_group_id(iommu_group), iommu_group);
>> +
>> + container->tbl = NULL;
>> + iommu_lock_table(tbl, false);
>> + }
>> + mutex_unlock(&container->lock);
>> +}
>> +
>> +const struct vfio_iommu_driver_ops tce_iommu_driver_ops = {
>> + .name = "iommu-vfio-powerpc",
>> + .owner = THIS_MODULE,
>> + .open = tce_iommu_open,
>> + .release = tce_iommu_release,
>> + .ioctl = tce_iommu_ioctl,
>> + .attach_group = tce_iommu_attach_group,
>> + .detach_group = tce_iommu_detach_group,
>> +};
>> +
>> +static int __init tce_iommu_init(void)
>> +{
>> + return vfio_register_iommu_driver(&tce_iommu_driver_ops);
>> +}
>> +
>> +static void __exit tce_iommu_cleanup(void)
>> +{
>> + vfio_unregister_iommu_driver(&tce_iommu_driver_ops);
>> +}
>> +
>> +module_init(tce_iommu_init);
>> +module_exit(tce_iommu_cleanup);
>> +
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 4758d1b..ea9a9a7 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -22,6 +22,7 @@
>> /* Extensions */
>>
>> #define VFIO_TYPE1_IOMMU 1
>> +#define VFIO_SPAPR_TCE_IOMMU 2
>>
>> /*
>> * The IOCTL interface is designed for extensibility by embedding the
>> @@ -365,4 +366,34 @@ struct vfio_iommu_type1_dma_unmap {
>>
>> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
>>
>> +/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>> +
>> +/*
>> + * The SPAPR TCE info struct provides the information about the PCI bus
>> + * address ranges available for DMA, these values are programmed into
>> + * the hardware so the guest has to know that information.
>> + *
>> + * The DMA 32 bit window start is an absolute PCI bus address.
>> + * The IOVA address passed via map/unmap ioctls are absolute PCI bus
>> + * addresses too so the window works as a filter rather than an offset
>> + * for IOVA addresses.
>> + *
>> + * A flag will need to be added if other page sizes are supported,
>> + * so as defined here, it is always 4k.
>> + */
>> +struct vfio_iommu_spapr_tce_info {
>> + __u32 argsz;
>> + __u32 flags; /* reserved for future use */
>> + __u32 dma32_window_start; /* 32 bit window start (bytes) */
>> + __u32 dma32_window_size; /* 32 bit window size (bytes) */
>> +};
>> +
>> +#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>> +
>> +/* Reuse type1 map/unmap structs as they are the same at the moment */
>> +typedef struct vfio_iommu_type1_dma_map vfio_iommu_spapr_tce_dma_map;
>> +typedef struct vfio_iommu_type1_dma_unmap vfio_iommu_spapr_tce_dma_unmap;
>> +
>> +/* ***************************************************************** */
>> +
>> #endif /* _UAPIVFIO_H */
>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: enabled on powernv platform
From: Alex Williamson @ 2013-02-12 0:01 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <51197C6A.5060403@ozlabs.ru>
On Tue, 2013-02-12 at 10:19 +1100, Alexey Kardashevskiy wrote:
> On 12/02/13 09:16, Alex Williamson wrote:
> > On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
> >> @@ -707,11 +709,39 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid)
> >> return tbl;
> >> }
> >>
> >> +static void group_release(void *iommu_data)
> >> +{
> >> + struct iommu_table *tbl = iommu_data;
> >> + tbl->it_group = NULL;
> >> +}
> >> +
> >> +void iommu_register_group(struct iommu_table * tbl,
> >> + int domain_number, unsigned long pe_num)
> >> +{
> >> + struct iommu_group *grp;
> >> +
> >> + grp = iommu_group_alloc();
> >> + if (IS_ERR(grp)) {
> >> + pr_info("powerpc iommu api: cannot create new group, err=%ld\n",
> >> + PTR_ERR(grp));
> >> + return;
> >> + }
> >> + tbl->it_group = grp;
> >> + iommu_group_set_iommudata(grp, tbl, group_release);
> >> + iommu_group_set_name(grp, kasprintf(GFP_KERNEL, "domain%d-pe%lx",
> >> + domain_number, pe_num));
> >> +}
> >> +
> >> void iommu_free_table(struct iommu_table *tbl, const char *node_name)
> >> {
> >> unsigned long bitmap_sz;
> >> unsigned int order;
> >>
> >> + if (tbl && tbl->it_group) {
> >> + iommu_group_put(tbl->it_group);
> >> + BUG_ON(tbl->it_group);
> >> + }
> >> +
> >> if (!tbl || !tbl->it_map) {
> >> printk(KERN_ERR "%s: expected TCE map for %s\n", __func__,
> >> node_name);
> >> @@ -876,4 +906,317 @@ void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
> >> {
> >> }
> >>
> >> +static enum dma_data_direction tce_direction(unsigned long tce)
> >> +{
> >> + if ((tce & TCE_PCI_READ) && (tce & TCE_PCI_WRITE))
> >> + return DMA_BIDIRECTIONAL;
> >> + else if (tce & TCE_PCI_READ)
> >> + return DMA_TO_DEVICE;
> >> + else if (tce & TCE_PCI_WRITE)
> >> + return DMA_FROM_DEVICE;
> >> + else
> >> + return DMA_NONE;
> >> +}
> >> +
> >> +void iommu_flush_tce(struct iommu_table *tbl)
> >> +{
> >> + /* Flush/invalidate TLB caches if necessary */
> >> + if (ppc_md.tce_flush)
> >> + ppc_md.tce_flush(tbl);
> >> +
> >> + /* Make sure updates are seen by hardware */
> >> + mb();
> >> +}
> >> +EXPORT_SYMBOL_GPL(iommu_flush_tce);
> >> +
> >> +static long tce_clear_param_check(struct iommu_table *tbl,
> >> + unsigned long ioba, unsigned long tce_value,
> >> + unsigned long npages)
> >> +{
> >> + unsigned long size = npages << IOMMU_PAGE_SHIFT;
> >> +
> >> + /* ppc_md.tce_free() does not support any value but 0 */
> >> + if (tce_value)
> >> + return -EINVAL;
> >> +
> >> + if (ioba & ~IOMMU_PAGE_MASK)
> >> + return -EINVAL;
> >> +
> >> + if ((ioba + size) > ((tbl->it_offset + tbl->it_size)
> >> + << IOMMU_PAGE_SHIFT))
> >> + return -EINVAL;
> >> +
> >> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >
> > Why do these all return long (vs int)? Is this a POWER-ism?
>
> No, not really but yeah, I picked it in powerpc code :) I tried to keep
> them "long" but I noticed "int" below so what is the rule? Change all to int?
I'd say anything that's returning 0/-errno should probably be an int.
> >> +}
> >> +
> >> +static long tce_put_param_check(struct iommu_table *tbl,
> >> + unsigned long ioba, unsigned long tce)
> >> +{
> >> + if (!(tce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> >> + return -EINVAL;
> >> +
> >> + if (tce & ~(IOMMU_PAGE_MASK | TCE_PCI_WRITE | TCE_PCI_READ))
> >> + return -EINVAL;
> >> +
> >> + if (ioba & ~IOMMU_PAGE_MASK)
> >> + return -EINVAL;
> >> +
> >> + if ((ioba + IOMMU_PAGE_SIZE) > ((tbl->it_offset + tbl->it_size)
> >> + << IOMMU_PAGE_SHIFT))
> >> + return -EINVAL;
> >> +
> >> + if (ioba < (tbl->it_offset << IOMMU_PAGE_SHIFT))
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static long clear_tce(struct iommu_table *tbl,
> >> + unsigned long entry, unsigned long pages)
> >> +{
> >> + unsigned long oldtce;
> >> + struct page *page;
> >> + struct iommu_pool *pool;
> >> +
> >> + for ( ; pages; --pages, ++entry) {
> >> + pool = get_pool(tbl, entry);
> >> + spin_lock(&(pool->lock));
> >> +
> >> + oldtce = ppc_md.tce_get(tbl, entry);
> >> + if (oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)) {
> >> + ppc_md.tce_free(tbl, entry, 1);
> >> +
> >> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
> >> + WARN_ON(!page);
> >> + if (page) {
> >> + if (oldtce & TCE_PCI_WRITE)
> >> + SetPageDirty(page);
> >> + put_page(page);
> >> + }
> >> + }
> >> + spin_unlock(&(pool->lock));
> >> + }
> >> +
> >> + return 0;
> >
> > No non-zero return, make it void?
>
> ah, ok. The prototype will change for real mode either way, it will get a
> "realmode" flag and become able to fail (which will switch the virtual mode).
If you'll use it later on, no need to change it for me. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 2/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alex Williamson @ 2013-02-12 0:25 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <5119828C.3020106@ozlabs.ru>
On Tue, 2013-02-12 at 10:45 +1100, Alexey Kardashevskiy wrote:
> On 12/02/13 09:17, Alex Williamson wrote:
> > On Mon, 2013-02-11 at 22:54 +1100, Alexey Kardashevskiy wrote:
> >> VFIO implements platform independent stuff such as
> >> a PCI driver, BAR access (via read/write on a file descriptor
> >> or direct mapping when possible) and IRQ signaling.
> >>
> >> The platform dependent part includes IOMMU initialization
> >> and handling. This patch implements an IOMMU driver for VFIO
> >> which does mapping/unmapping pages for the guest IO and
> >> provides information about DMA window (required by a POWERPC
> >> guest).
> >>
> >> The counterpart in QEMU is required to support this functionality.
> >
> > Revision info would be great here too.
> >
> >
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> drivers/vfio/Kconfig | 6 +
> >> drivers/vfio/Makefile | 1 +
> >> drivers/vfio/vfio_iommu_spapr_tce.c | 269 +++++++++++++++++++++++++++++++++++
> >> include/uapi/linux/vfio.h | 31 ++++
> >> 4 files changed, 307 insertions(+)
> >> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
> >>
> >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >> index 7cd5dec..b464687 100644
> >> --- a/drivers/vfio/Kconfig
> >> +++ b/drivers/vfio/Kconfig
> >> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
> >> depends on VFIO
> >> default n
> >>
> >> +config VFIO_IOMMU_SPAPR_TCE
> >> + tristate
> >> + depends on VFIO && SPAPR_TCE_IOMMU
> >> + default n
> >> +
> >> menuconfig VFIO
> >> tristate "VFIO Non-Privileged userspace driver framework"
> >> depends on IOMMU_API
> >> select VFIO_IOMMU_TYPE1 if X86
> >> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
> >> help
> >> VFIO provides a framework for secure userspace device drivers.
> >> See Documentation/vfio.txt for more details.
> >> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> >> index 2398d4a..72bfabc 100644
> >> --- a/drivers/vfio/Makefile
> >> +++ b/drivers/vfio/Makefile
> >> @@ -1,3 +1,4 @@
> >> obj-$(CONFIG_VFIO) += vfio.o
> >> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> >> obj-$(CONFIG_VFIO_PCI) += pci/
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> new file mode 100644
> >> index 0000000..9b3fa88
> >> --- /dev/null
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -0,0 +1,269 @@
> >> +/*
> >> + * VFIO: IOMMU DMA mapping support for TCE on POWER
> >> + *
> >> + * Copyright (C) 2012 IBM Corp. All rights reserved.
> >
> > 2013 now
> >
> >> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Derived from original vfio_iommu_type1.c:
> >> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> >> + * Author: Alex Williamson <alex.williamson@redhat.com>
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/uaccess.h>
> >> +#include <linux/err.h>
> >> +#include <linux/vfio.h>
> >> +#include <asm/iommu.h>
> >> +#include <asm/tce.h>
> >> +
> >> +#define DRIVER_VERSION "0.1"
> >> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
> >> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >> +
> >> +static void tce_iommu_detach_group(void *iommu_data,
> >> + struct iommu_group *iommu_group);
> >> +
> >> +/*
> >> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
> >> + *
> >> + * This code handles mapping and unmapping of user data buffers
> >> + * into DMA'ble space using the IOMMU
> >> + */
> >> +
> >> +/*
> >> + * The container descriptor supports only a single group per container.
> >> + * Required by the API as the container is not supplied with the IOMMU group
> >> + * at the moment of initialization.
> >> + */
> >> +struct tce_container {
> >> + struct mutex lock;
> >> + struct iommu_table *tbl;
> >> +};
> >> +
> >> +static void *tce_iommu_open(unsigned long arg)
> >> +{
> >> + struct tce_container *container;
> >> +
> >> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
> >> + pr_err("tce_vfio: Wrong IOMMU type\n");
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + container = kzalloc(sizeof(*container), GFP_KERNEL);
> >> + if (!container)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + mutex_init(&container->lock);
> >> +
> >> + return container;
> >> +}
> >> +
> >> +static void tce_iommu_release(void *iommu_data)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> +
> >> + WARN_ON(container->tbl && !container->tbl->it_group);
> >> + if (container->tbl && container->tbl->it_group)
> >> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
> >> +
> >> + mutex_destroy(&container->lock);
> >> +
> >> + kfree(container);
> >> +}
> >> +
> >> +static long tce_iommu_ioctl(void *iommu_data,
> >> + unsigned int cmd, unsigned long arg)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> + unsigned long minsz;
> >> + long ret;
> >> +
> >> + switch (cmd) {
> >> + case VFIO_CHECK_EXTENSION:
> >> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
> >> +
> >> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
> >> + struct vfio_iommu_spapr_tce_info info;
> >> + struct iommu_table *tbl = container->tbl;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
> >> + dma32_window_size);
> >> +
> >> + if (copy_from_user(&info, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (info.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
> >> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
> >> + info.flags = 0;
> >> +
> >> + if (copy_to_user((void __user *)arg, &info, minsz))
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> + }
> >> + case VFIO_IOMMU_MAP_DMA: {
> >> + vfio_iommu_spapr_tce_dma_map param;
> >> + struct iommu_table *tbl = container->tbl;
> >> + unsigned long tce;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
> >> +
> >> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (param.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + if (param.flags & ~(VFIO_DMA_MAP_FLAG_READ |
> >> + VFIO_DMA_MAP_FLAG_WRITE))
> >> + return -EINVAL;
> >> +
> >> + if ((param.size & ~IOMMU_PAGE_MASK) ||
> >> + (param.vaddr & ~IOMMU_PAGE_MASK))
> >> + return -EINVAL;
> >> +
> >> + /* TODO: support multiple TCEs */
> >> + if (param.size != IOMMU_PAGE_SIZE) {
> >
> > Ouch, ioctl per page...
>
> Well, there is something to discuss.
>
> On POWER, there is an interface to add multiple pages at once but the guest
> does not supply the range of guest phys addresses, it puts some addresses
> to a small page (so it is up to 512 pages at once) and passes this address
> to the host as a parameter.
>
> I posted another series yesterday but forgot to cc: you :) You can find
> them here - http://patchwork.ozlabs.org/patch/219592/ (emulated devices)
> and http://patchwork.ozlabs.org/patch/219594/ (vfio). There I convert guest
> phys address (real and virtual mode are handled different ways) and call
> iommu_put_tce_user_mode() (or analog) in a loop.
>
> Either way, I did some tests with 10Gb card and without real mode stuff it
> does 220MB/s, and even if I do multi-tce it won't be faster than ~400MB/s
> which is still not enough as the real mode code makes it 1020MB/s. Slower
> devices work on the same speed no matter what.
Ok, I'll take a look.
> >> + pr_err("VFIO map on POWER supports only %lu page size\n",
> >> + IOMMU_PAGE_SIZE);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* iova is checked by the IOMMU API */
> >> + tce = param.vaddr;
> >> + if (param.flags & VFIO_DMA_MAP_FLAG_READ)
> >> + tce |= TCE_PCI_READ;
> >> + if (param.flags & VFIO_DMA_MAP_FLAG_WRITE)
> >> + tce |= TCE_PCI_WRITE;
> >> +
> >> + ret = iommu_put_tce_user_mode(tbl, param.iova, tce);
> >> + iommu_flush_tce(tbl);
> >> +
> >> + return ret;
> >> + }
> >> + case VFIO_IOMMU_UNMAP_DMA: {
> >> + vfio_iommu_spapr_tce_dma_unmap param;
> >> + struct iommu_table *tbl = container->tbl;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
> >> +
> >> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (param.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + /* No flag is supported now */
> >> + if (param.flags)
> >> + return -EINVAL;
> >> +
> >> + if (param.size & ~IOMMU_PAGE_MASK)
> >> + return -EINVAL;
> >
> > But you support multi-page unmaps?
>
> Yes, this is a lot easier :)
>
>
> >> + /* iova is checked by the IOMMU API */
> >> + ret = iommu_clear_tce_user_mode(tbl, param.iova, 0,
> >> + param.size >> IOMMU_PAGE_SHIFT);
> >> + iommu_flush_tce(tbl);
> >> +
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + return -ENOTTY;
> >> +}
> >> +
> >> +static int tce_iommu_attach_group(void *iommu_data,
> >> + struct iommu_group *iommu_group)
> >> +{
> >> + int ret;
> >> + struct tce_container *container = iommu_data;
> >> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> >> +
> >> + BUG_ON(!tbl);
> >> + mutex_lock(&container->lock);
> >> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
> >> + iommu_group_id(iommu_group), iommu_group);
> >> + if (container->tbl) {
> >> + pr_warn("tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
> >> + iommu_group_id(container->tbl->it_group),
> >> + iommu_group_id(iommu_group));
> >> + mutex_unlock(&container->lock);
> >> + return -EBUSY;
> >> + }
> >> +
> >> + container->tbl = tbl;
> >> + ret = iommu_lock_table(tbl, true);
> >
> > Bug, container->tbl is set regardless of iommu_lock_table.
>
> Oops, bug.
>
> > Ok, so now we're checking rlimits and handling page accounting on
> > VFIO_GROUP_SET_CONTAINER to avoid any overhead at map/unmap. How can
> > the user learn tbl->it_size to set their locked page limit prior to
> > this? It's available from GET_INFO, but there's a chicken and egg
> > problem that to get it there you have to get past this, which means
> > you're already ok. Maybe it's in sysfs somewhere already or it could be
> > exposed in the iommu group like the name attribute. Otherwise we might
> > consider doing locking on first mapping. Thanks,
>
> GET_INFO is called in the beginning so QEMU will exit right there. No real
> work will have been done till that moment so what is the problem?
GET_INFO uses container->tbl, which is set here, so how could it be
called first? Thanks,
Alex
> >> + mutex_unlock(&container->lock);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void tce_iommu_detach_group(void *iommu_data,
> >> + struct iommu_group *iommu_group)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> >> +
> >> + BUG_ON(!tbl);
> >> + mutex_lock(&container->lock);
> >> + if (tbl != container->tbl) {
> >> + pr_warn("tce_vfio: detaching group #%u, expected group is #%u\n",
> >> + iommu_group_id(iommu_group),
> >> + iommu_group_id(tbl->it_group));
> >> + } else {
> >> +
> >> + pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
> >> + iommu_group_id(iommu_group), iommu_group);
> >> +
> >> + container->tbl = NULL;
> >> + iommu_lock_table(tbl, false);
> >> + }
> >> + mutex_unlock(&container->lock);
> >> +}
> >> +
> >> +const struct vfio_iommu_driver_ops tce_iommu_driver_ops = {
> >> + .name = "iommu-vfio-powerpc",
> >> + .owner = THIS_MODULE,
> >> + .open = tce_iommu_open,
> >> + .release = tce_iommu_release,
> >> + .ioctl = tce_iommu_ioctl,
> >> + .attach_group = tce_iommu_attach_group,
> >> + .detach_group = tce_iommu_detach_group,
> >> +};
> >> +
> >> +static int __init tce_iommu_init(void)
> >> +{
> >> + return vfio_register_iommu_driver(&tce_iommu_driver_ops);
> >> +}
> >> +
> >> +static void __exit tce_iommu_cleanup(void)
> >> +{
> >> + vfio_unregister_iommu_driver(&tce_iommu_driver_ops);
> >> +}
> >> +
> >> +module_init(tce_iommu_init);
> >> +module_exit(tce_iommu_cleanup);
> >> +
> >> +MODULE_VERSION(DRIVER_VERSION);
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_AUTHOR(DRIVER_AUTHOR);
> >> +MODULE_DESCRIPTION(DRIVER_DESC);
> >> +
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index 4758d1b..ea9a9a7 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -22,6 +22,7 @@
> >> /* Extensions */
> >>
> >> #define VFIO_TYPE1_IOMMU 1
> >> +#define VFIO_SPAPR_TCE_IOMMU 2
> >>
> >> /*
> >> * The IOCTL interface is designed for extensibility by embedding the
> >> @@ -365,4 +366,34 @@ struct vfio_iommu_type1_dma_unmap {
> >>
> >> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
> >>
> >> +/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
> >> +
> >> +/*
> >> + * The SPAPR TCE info struct provides the information about the PCI bus
> >> + * address ranges available for DMA, these values are programmed into
> >> + * the hardware so the guest has to know that information.
> >> + *
> >> + * The DMA 32 bit window start is an absolute PCI bus address.
> >> + * The IOVA address passed via map/unmap ioctls are absolute PCI bus
> >> + * addresses too so the window works as a filter rather than an offset
> >> + * for IOVA addresses.
> >> + *
> >> + * A flag will need to be added if other page sizes are supported,
> >> + * so as defined here, it is always 4k.
> >> + */
> >> +struct vfio_iommu_spapr_tce_info {
> >> + __u32 argsz;
> >> + __u32 flags; /* reserved for future use */
> >> + __u32 dma32_window_start; /* 32 bit window start (bytes) */
> >> + __u32 dma32_window_size; /* 32 bit window size (bytes) */
> >> +};
> >> +
> >> +#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> >> +
> >> +/* Reuse type1 map/unmap structs as they are the same at the moment */
> >> +typedef struct vfio_iommu_type1_dma_map vfio_iommu_spapr_tce_dma_map;
> >> +typedef struct vfio_iommu_type1_dma_unmap vfio_iommu_spapr_tce_dma_unmap;
> >> +
> >> +/* ***************************************************************** */
> >> +
> >> #endif /* _UAPIVFIO_H */
> >
> >
> >
>
>
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Benjamin Herrenschmidt @ 2013-02-12 0:51 UTC (permalink / raw)
To: Grant Likely; +Cc: Baruch Siach, linuxppc-dev, linux-kernel
In-Reply-To: <20130211205255.72B513E3530@localhost>
On Mon, 2013-02-11 at 20:52 +0000, Grant Likely wrote:
> Really the irq mappings should be using reference counting. The existing
> code is naive on this count and just releases the irq on the first call
> to irq_dispose_mapping(). I've not gotten around to fixing that. Anyone
> want to take that task on?
Is this the best approach ?
The original idea was that there was no point disposing of mappings in most
cases and keeping the mapping around would provide a bit of stability of
interrupt numbers which might come in handy for debugging etc...
The few cases where disposing of a mapping might be useful is if the underlying
physical interrupts completely disappear, as in a cascaded controller gets
removed or that sort of thing, which is a very rare case... And even then...
Could you just make irq_dispose_mapping() check if the irq desc is
active and fail/WARN/BUG if it is ? I don't see the point of adding a refcount,
that feels overkill.
Cheers,
Ben.
^ permalink raw reply
* Re: Re[3]: PS3 platform is broken on Linux 3.7.0
From: Geoff Levand @ 2013-02-12 1:11 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Phileas Fogg, linuxppc-dev
In-Reply-To: <87zjzb4051.fsf@linux.vnet.ibm.com>
Hi Aneesh,
On Mon, 2013-02-11 at 15:56 +0530, Aneesh Kumar K.V wrote:
> Can you try this patch ?
>
> diff --git a/arch/powerpc/include/asm/mmu-hash64.h b/arch/powerpc/include/asm/mmu-hash64.h
I tried your patch on PS3 with a ps3_defconfig build on both Linux-3.7
and Linux-3.8-rc7 and it seems to be working OK.
Please try to prepare a final fix for v3.8 and also send to linux-stable
for v3.7.
Thanks!
-Geoff
^ permalink raw reply
* BOOKE KVM calling load_up_fpu from C?
From: Michael Neuling @ 2013-02-12 3:29 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
Scott,
I was looking at changing how load_up_fpu works and I found this in
arch/powerpc/kvm/booke.h:
static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu)
{
#ifdef CONFIG_PPC_FPU
if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
load_up_fpu();
current->thread.regs->msr |= MSR_FP;
}
#endif
}
I'm wondering how this is suppose to work since load_up_fpu is suppose
to have MSR in R12?
Mikey
^ permalink raw reply
* RE: BOOKE KVM calling load_up_fpu from C?
From: Bhushan Bharat-R65777 @ 2013-02-12 3:37 UTC (permalink / raw)
To: Michael Neuling, Wood Scott-B07421; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <14306.1360639760@ale.ozlabs.ibm.com>
> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Mic=
hael
> Neuling
> Sent: Tuesday, February 12, 2013 8:59 AM
> To: Wood Scott-B07421
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: BOOKE KVM calling load_up_fpu from C?
>=20
> Scott,
>=20
> I was looking at changing how load_up_fpu works and I found this in
> arch/powerpc/kvm/booke.h:
>=20
> static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) { #ifdef
> CONFIG_PPC_FPU
> if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> load_up_fpu();
> current->thread.regs->msr |=3D MSR_FP;
> }
> #endif
> }
>=20
> I'm wondering how this is suppose to work since load_up_fpu is suppose to=
have
> MSR in R12?
Is not the load_up_fpu() does mfmsr:
_GLOBAL(load_up_fpu)
mfmsr r5
ori r5,r5,MSR_FP
#ifdef CONFIG_VSX
BEGIN_FTR_SECTION
oris r5,r5,MSR_VSX@h
END_FTR_SECTION_IFSET(CPU_FTR_VSX)
#endif
SYNC
MTMSRD(r5) /* enable use of fpu now */
isync
<snip>
-Bharat
>=20
> Mikey
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: BOOKE KVM calling load_up_fpu from C?
From: Michael Neuling @ 2013-02-12 3:46 UTC (permalink / raw)
To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D0659EC4A@039-SN2MPN1-023.039d.mgd.msft.net>
Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>
>
> > -----Original Message-----
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of Michael
> > Neuling
> > Sent: Tuesday, February 12, 2013 8:59 AM
> > To: Wood Scott-B07421
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Subject: BOOKE KVM calling load_up_fpu from C?
> >
> > Scott,
> >
> > I was looking at changing how load_up_fpu works and I found this in
> > arch/powerpc/kvm/booke.h:
> >
> > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) { #ifdef
> > CONFIG_PPC_FPU
> > if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > load_up_fpu();
> > current->thread.regs->msr |= MSR_FP;
> > }
> > #endif
> > }
> >
> > I'm wondering how this is suppose to work since load_up_fpu is suppose to have
> > MSR in R12?
>
> Is not the load_up_fpu() does mfmsr:
>
> _GLOBAL(load_up_fpu)
> mfmsr r5
> ori r5,r5,MSR_FP
> #ifdef CONFIG_VSX
> BEGIN_FTR_SECTION
> oris r5,r5,MSR_VSX@h
> END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> #endif
> SYNC
> MTMSRD(r5) /* enable use of fpu now */
> isync
> <snip>
Look further down...
#ifdef CONFIG_PPC32
mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) */
lwz r4,THREAD_FPEXC_MODE(r5)
ori r9,r9,MSR_FP /* enable FP for current */
or r9,r9,r4
#else
ld r4,PACACURRENT(r13)
addi r5,r4,THREAD /* Get THREAD */
lwz r4,THREAD_FPEXC_MODE(r5)
ori r12,r12,MSR_FP
or r12,r12,r4
std r12,_MSR(r1)
#endif
R12 is loaded with SRR1 in the exception prolog before load_up_fpu is
called. It's the MSR of the user process, not the current MSR.
Mikey
^ permalink raw reply
* RE: BOOKE KVM calling load_up_fpu from C?
From: Bhushan Bharat-R65777 @ 2013-02-12 3:58 UTC (permalink / raw)
To: Michael Neuling; +Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <16381.1360640767@ale.ozlabs.ibm.com>
> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org]
> Sent: Tuesday, February 12, 2013 9:16 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>=20
> >
> >
> > > -----Original Message-----
> > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of
> > > bounces+Michael
> > > Neuling
> > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > To: Wood Scott-B07421
> > > Cc: linuxppc-dev@lists.ozlabs.org
> > > Subject: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Scott,
> > >
> > > I was looking at changing how load_up_fpu works and I found this in
> > > arch/powerpc/kvm/booke.h:
> > >
> > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > #ifdef CONFIG_PPC_FPU
> > > if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > load_up_fpu();
> > > current->thread.regs->msr |=3D MSR_FP;
> > > }
> > > #endif
> > > }
> > >
> > > I'm wondering how this is suppose to work since load_up_fpu is
> > > suppose to have MSR in R12?
> >
> > Is not the load_up_fpu() does mfmsr:
> >
> > _GLOBAL(load_up_fpu)
> > mfmsr r5
> > ori r5,r5,MSR_FP
> > #ifdef CONFIG_VSX
> > BEGIN_FTR_SECTION
> > oris r5,r5,MSR_VSX@h
> > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > #endif
> > SYNC
> > MTMSRD(r5) /* enable use of fpu now */
> > isync
> > <snip>
>=20
> Look further down...
>=20
> #ifdef CONFIG_PPC32
> mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) */
> lwz r4,THREAD_FPEXC_MODE(r5)
> ori r9,r9,MSR_FP /* enable FP for current */
> or r9,r9,r4
> #else
> ld r4,PACACURRENT(r13)
> addi r5,r4,THREAD /* Get THREAD */
> lwz r4,THREAD_FPEXC_MODE(r5)
> ori r12,r12,MSR_FP
> or r12,r12,r4
> std r12,_MSR(r1)
> #endif
>=20
> R12 is loaded with SRR1 in the exception prolog before load_up_fpu is cal=
led.
Yes it is SRR1 not MSR.
Also on 32bit it looks like that R9 is assumed to have SRR1.
-Bharat
> It's the MSR of the user process, not the current MSR.
>=20
> Mikey
^ permalink raw reply
* Re: [PATCH v5 00/45] CPU hotplug: stop_machine()-free CPU hotplug
From: Srivatsa S. Bhat @ 2013-02-12 3:58 UTC (permalink / raw)
To: paulmck
Cc: linux-doc, peterz, fweisbec, linux-kernel, walken, mingo,
linux-arch, Russell King - ARM Linux, xiaoguangrong, wangyun,
nikunj, linux-pm, Rusty Russell, rostedt, rjw, namhyung, tglx,
linux-arm-kernel, netdev, oleg, Vincent Guittot, sbw, tj, akpm,
linuxppc-dev
In-Reply-To: <20130211190852.GA5695@linux.vnet.ibm.com>
On 02/12/2013 12:38 AM, Paul E. McKenney wrote:
> On Mon, Feb 11, 2013 at 05:53:41PM +0530, Srivatsa S. Bhat wrote:
>> On 02/11/2013 05:28 PM, Vincent Guittot wrote:
>>> On 8 February 2013 19:09, Srivatsa S. Bhat
>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>
> [ . . . ]
>
>>>> Adding Vincent to CC, who had previously evaluated the performance and
>>>> latency implications of CPU hotplug on ARM platforms, IIRC.
>>>>
>>>
>>> Hi Srivatsa,
>>>
>>> I can try to run some of our stress tests on your patches.
>>
>> Great!
>>
>>> Have you
>>> got a git tree that i can pull ?
>>>
>>
>> Unfortunately, no, none at the moment.. :-(
>
> You do need to create an externally visible git tree.
Ok, I'll do that soon.
> In the meantime,
> I have added your series at rcu/bhat.2013.01.21a on -rcu:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>
> This should appear soon on a kernel.org mirror near you. ;-)
>
Thank you very much, Paul! :-)
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* Re: BOOKE KVM calling load_up_fpu from C?
From: Michael Neuling @ 2013-02-12 4:16 UTC (permalink / raw)
To: Bhushan Bharat-R65777; +Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D0659EC82@039-SN2MPN1-023.039d.mgd.msft.net>
Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>
>
> > -----Original Message-----
> > From: Michael Neuling [mailto:mikey@neuling.org]
> > Sent: Tuesday, February 12, 2013 9:16 AM
> > To: Bhushan Bharat-R65777
> > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> >
> > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
> > > > bounces+Michael
> > > > Neuling
> > > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > > To: Wood Scott-B07421
> > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > Subject: BOOKE KVM calling load_up_fpu from C?
> > > >
> > > > Scott,
> > > >
> > > > I was looking at changing how load_up_fpu works and I found this in
> > > > arch/powerpc/kvm/booke.h:
> > > >
> > > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > > #ifdef CONFIG_PPC_FPU
> > > > if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > > load_up_fpu();
> > > > current->thread.regs->msr |= MSR_FP;
> > > > }
> > > > #endif
> > > > }
> > > >
> > > > I'm wondering how this is suppose to work since load_up_fpu is
> > > > suppose to have MSR in R12?
> > >
> > > Is not the load_up_fpu() does mfmsr:
> > >
> > > _GLOBAL(load_up_fpu)
> > > mfmsr r5
> > > ori r5,r5,MSR_FP
> > > #ifdef CONFIG_VSX
> > > BEGIN_FTR_SECTION
> > > oris r5,r5,MSR_VSX@h
> > > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > > #endif
> > > SYNC
> > > MTMSRD(r5) /* enable use of fpu now */
> > > isync
> > > <snip>
> >
> > Look further down...
> >
> > #ifdef CONFIG_PPC32
> > mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) */
> > lwz r4,THREAD_FPEXC_MODE(r5)
> > ori r9,r9,MSR_FP /* enable FP for current */
> > or r9,r9,r4
> > #else
> > ld r4,PACACURRENT(r13)
> > addi r5,r4,THREAD /* Get THREAD */
> > lwz r4,THREAD_FPEXC_MODE(r5)
> > ori r12,r12,MSR_FP
> > or r12,r12,r4
> > std r12,_MSR(r1)
> > #endif
> >
> > R12 is loaded with SRR1 in the exception prolog before load_up_fpu is called.
>
> Yes it is SRR1 not MSR.
Yes, SRR1 == the MSR of the user process, not the current MSR.
> Also on 32bit it looks like that R9 is assumed to have SRR1.
Yep that too.
So any idea how it's suppose to work or is it broken?
Mikey
^ permalink raw reply
* Re: [PATCH] Centralise CONFIG_ARCH_NO_VIRT_TO_BUS
From: Stephen Rothwell @ 2013-02-12 4:40 UTC (permalink / raw)
To: James Hogan
Cc: linux-arch, linux-sh, Vineet Gupta, linux-kernel, David S. Miller,
sparclinux, Paul Mundt, Paul Mackerras, Bjorn Helgaas,
Andrew Morton, linuxppc-dev, H Hartley Sweeten
In-Reply-To: <5118DCA8.1030905@imgtec.com>
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
Hi James,
On Mon, 11 Feb 2013 11:57:28 +0000 James Hogan <james.hogan@imgtec.com> wrote:
>
> On 12/11/12 21:26, Stephen Rothwell wrote:
> > Make if easier for more architectures to select it and thus disable
> > drivers that use virt_to_bus().
> >
> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
>
> I was just wondering what the status of this patch is? It was in -next
> for a while but seems to have disappeared.
Yeah, I think Andrew dropped it in favour of a more complete solution
which I haven't written yet. Let me dig out the emails and have a shot.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Michael Ellerman @ 2013-02-12 6:18 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Baruch Siach, linuxppc-dev, linux-kernel
In-Reply-To: <1360630273.2035.2.camel@pasglop>
On Tue, Feb 12, 2013 at 11:51:13AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2013-02-11 at 20:52 +0000, Grant Likely wrote:
> > Really the irq mappings should be using reference counting. The existing
> > code is naive on this count and just releases the irq on the first call
> > to irq_dispose_mapping(). I've not gotten around to fixing that. Anyone
> > want to take that task on?
>
> Is this the best approach ?
>
> The original idea was that there was no point disposing of mappings in most
> cases and keeping the mapping around would provide a bit of stability of
> interrupt numbers which might come in handy for debugging etc...
>
> The few cases where disposing of a mapping might be useful is if the underlying
> physical interrupts completely disappear, as in a cascaded controller gets
> removed or that sort of thing, which is a very rare case... And even then...
That may have been the intent, but we forgot to tell driver writers, ourselves
included.
> Could you just make irq_dispose_mapping() check if the irq desc is
> active and fail/WARN/BUG if it is ? I don't see the point of adding a refcount,
> that feels overkill.
I don't think you can, "active" is not well defined. Other code may have
done nothing other than create the mapping and remembered the virq,
which will break if you destroy the mapping. Or?
I agree refcounting is not fun. It'll end up with the same mess as
of_node_get/put() where practically every 2nd piece of code leaks
references.
I guess we can't go the other way, and say that mapping the same hwirq
twice is an error.
cheers
^ permalink raw reply
* Re: [BUG] irq_dispose_mapping after irq request failure
From: Benjamin Herrenschmidt @ 2013-02-12 8:53 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Baruch Siach, linuxppc-dev, linux-kernel
In-Reply-To: <20130212061822.GA8977@concordia>
On Tue, 2013-02-12 at 17:18 +1100, Michael Ellerman wrote:
>
> I don't think you can, "active" is not well defined. Other code may have
> done nothing other than create the mapping and remembered the virq,
> which will break if you destroy the mapping. Or?
Active as in "requested". Yes there's a potential problems with multiple
requests for mappings & shared interrupts. This is not a problem for PCI
on powerpc because we don't free those mappings afaik.
> I agree refcounting is not fun. It'll end up with the same mess as
> of_node_get/put() where practically every 2nd piece of code leaks
> references.
>
> I guess we can't go the other way, and say that mapping the same hwirq
> twice is an error.
Might be worth it, and force the sharing case to be handled at some kind
of upper level (bus or platform).
Ben.
^ permalink raw reply
* RE: BOOKE KVM calling load_up_fpu from C?
From: Bhushan Bharat-R65777 @ 2013-02-12 9:01 UTC (permalink / raw)
To: Michael Neuling; +Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <30014.1360642578@ale.ozlabs.ibm.com>
> -----Original Message-----
> From: Michael Neuling [mailto:mikey@neuling.org]
> Sent: Tuesday, February 12, 2013 9:46 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> Subject: Re: BOOKE KVM calling load_up_fpu from C?
>=20
> Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
>=20
> >
> >
> > > -----Original Message-----
> > > From: Michael Neuling [mailto:mikey@neuling.org]
> > > Sent: Tuesday, February 12, 2013 9:16 AM
> > > To: Bhushan Bharat-R65777
> > > Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org
> > > Subject: Re: BOOKE KVM calling load_up_fpu from C?
> > >
> > > Bhushan Bharat-R65777 <R65777@freescale.com> wrote:
> > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > > bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behal=
f
> > > > > bounces+Of Michael
> > > > > Neuling
> > > > > Sent: Tuesday, February 12, 2013 8:59 AM
> > > > > To: Wood Scott-B07421
> > > > > Cc: linuxppc-dev@lists.ozlabs.org
> > > > > Subject: BOOKE KVM calling load_up_fpu from C?
> > > > >
> > > > > Scott,
> > > > >
> > > > > I was looking at changing how load_up_fpu works and I found this
> > > > > in
> > > > > arch/powerpc/kvm/booke.h:
> > > > >
> > > > > static inline void kvmppc_load_guest_fp(struct kvm_vcpu *vcpu) {
> > > > > #ifdef CONFIG_PPC_FPU
> > > > > if (vcpu->fpu_active && !(current->thread.regs->msr & MSR_FP)) {
> > > > > load_up_fpu();
> > > > > current->thread.regs->msr |=3D MSR_FP;
> > > > > }
> > > > > #endif
> > > > > }
> > > > >
> > > > > I'm wondering how this is suppose to work since load_up_fpu is
> > > > > suppose to have MSR in R12?
> > > >
> > > > Is not the load_up_fpu() does mfmsr:
> > > >
> > > > _GLOBAL(load_up_fpu)
> > > > mfmsr r5
> > > > ori r5,r5,MSR_FP
> > > > #ifdef CONFIG_VSX
> > > > BEGIN_FTR_SECTION
> > > > oris r5,r5,MSR_VSX@h
> > > > END_FTR_SECTION_IFSET(CPU_FTR_VSX)
> > > > #endif
> > > > SYNC
> > > > MTMSRD(r5) /* enable use of fpu now */
> > > > isync
> > > > <snip>
> > >
> > > Look further down...
> > >
> > > #ifdef CONFIG_PPC32
> > > mfspr r5,SPRN_SPRG_THREAD /* current task's THREAD (phys) */
> > > lwz r4,THREAD_FPEXC_MODE(r5)
> > > ori r9,r9,MSR_FP /* enable FP for current */
> > > or r9,r9,r4
> > > #else
> > > ld r4,PACACURRENT(r13)
> > > addi r5,r4,THREAD /* Get THREAD */
> > > lwz r4,THREAD_FPEXC_MODE(r5)
> > > ori r12,r12,MSR_FP
> > > or r12,r12,r4
> > > std r12,_MSR(r1)
> > > #endif
> > >
> > > R12 is loaded with SRR1 in the exception prolog before load_up_fpu is
> called.
> >
> > Yes it is SRR1 not MSR.
>=20
> Yes, SRR1 =3D=3D the MSR of the user process, not the current MSR.
>=20
> > Also on 32bit it looks like that R9 is assumed to have SRR1.
>=20
> Yep that too.
>=20
> So any idea how it's suppose to work or is it broken?
To me this looks wrong. And this seems to works because the thread->reg->ms=
r is not actually used to write SRR1 (and eventually the thread MSR) when d=
oing rfi to enter guest. Infact Guest(shadow_msr) MSR is used as SRR1 and w=
hich will have proper MSR (including FP set).
But Yes, Scott is right person to comment, So let us wait for him comment.
Thanks
-Bharat
^ permalink raw reply
* [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
From: Stefan Roese @ 2013-02-12 9:08 UTC (permalink / raw)
To: netdev; +Cc: linuxppc-dev, Anatolij Gustschin
Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
(U-Boot) to write the MAC address into the ethernet controller
registers. The Linux driver should not rely on such a thing. So
lets read the MAC address from the DT as it should be done here.
The following priority is now used to read the MAC address:
1) First, try OF node MAC address, if not present or invalid, then:
2) Read from MAC address registers, if invalid, then:
3) Log a warning message, and choose a random MAC address.
This fixes a problem with a MPC5200 board that uses the SPL U-Boot
version without FEC initialization before Linux booting for
boot speedup.
Additionally a status line is now be printed upon successful
driver probing, also displaying this MAC address.
Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Anatolij Gustschin <agust@denx.de>
---
v2:
- Remove module parameter mpc52xx_fec_mac_addr
- Priority for MAC address probing now is DT, controller regs
If the resulting MAC address is invalid, a random address will
be generated and used with a warning message
- Use "np" variable to simplify the code
drivers/net/ethernet/freescale/fec_mpc52xx.c | 61 +++++++++++++++++-----------
1 file changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c b/drivers/net/ethernet/freescale/fec_mpc52xx.c
index 817d081..8b725f4 100644
--- a/drivers/net/ethernet/freescale/fec_mpc52xx.c
+++ b/drivers/net/ethernet/freescale/fec_mpc52xx.c
@@ -76,10 +76,6 @@ static void mpc52xx_fec_stop(struct net_device *dev);
static void mpc52xx_fec_start(struct net_device *dev);
static void mpc52xx_fec_reset(struct net_device *dev);
-static u8 mpc52xx_fec_mac_addr[6];
-module_param_array_named(mac, mpc52xx_fec_mac_addr, byte, NULL, 0);
-MODULE_PARM_DESC(mac, "six hex digits, ie. 0x1,0x2,0xc0,0x01,0xba,0xbe");
-
#define MPC52xx_MESSAGES_DEFAULT ( NETIF_MSG_DRV | NETIF_MSG_PROBE | \
NETIF_MSG_LINK | NETIF_MSG_IFDOWN | NETIF_MSG_IFUP)
static int debug = -1; /* the above default */
@@ -110,15 +106,6 @@ static void mpc52xx_fec_set_paddr(struct net_device *dev, u8 *mac)
out_be32(&fec->paddr2, (*(u16 *)(&mac[4]) << 16) | FEC_PADDR2_TYPE);
}
-static void mpc52xx_fec_get_paddr(struct net_device *dev, u8 *mac)
-{
- struct mpc52xx_fec_priv *priv = netdev_priv(dev);
- struct mpc52xx_fec __iomem *fec = priv->fec;
-
- *(u32 *)(&mac[0]) = in_be32(&fec->paddr1);
- *(u16 *)(&mac[4]) = in_be32(&fec->paddr2) >> 16;
-}
-
static int mpc52xx_fec_set_mac_address(struct net_device *dev, void *addr)
{
struct sockaddr *sock = addr;
@@ -853,6 +840,8 @@ static int mpc52xx_fec_probe(struct platform_device *op)
struct resource mem;
const u32 *prop;
int prop_size;
+ struct device_node *np = op->dev.of_node;
+ const void *p;
phys_addr_t rx_fifo;
phys_addr_t tx_fifo;
@@ -866,7 +855,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
priv->ndev = ndev;
/* Reserve FEC control zone */
- rv = of_address_to_resource(op->dev.of_node, 0, &mem);
+ rv = of_address_to_resource(np, 0, &mem);
if (rv) {
printk(KERN_ERR DRIVER_NAME ": "
"Error while parsing device node resource\n" );
@@ -919,7 +908,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
/* Get the IRQ we need one by one */
/* Control */
- ndev->irq = irq_of_parse_and_map(op->dev.of_node, 0);
+ ndev->irq = irq_of_parse_and_map(np, 0);
/* RX */
priv->r_irq = bcom_get_task_irq(priv->rx_dmatsk);
@@ -927,11 +916,33 @@ static int mpc52xx_fec_probe(struct platform_device *op)
/* TX */
priv->t_irq = bcom_get_task_irq(priv->tx_dmatsk);
- /* MAC address init */
- if (!is_zero_ether_addr(mpc52xx_fec_mac_addr))
- memcpy(ndev->dev_addr, mpc52xx_fec_mac_addr, 6);
- else
- mpc52xx_fec_get_paddr(ndev, ndev->dev_addr);
+ /*
+ * MAC address init:
+ *
+ * First try to read MAC address from DT
+ */
+ p = of_get_property(np, "local-mac-address", NULL);
+ if (p != NULL) {
+ memcpy(ndev->dev_addr, p, 6);
+ } else {
+ struct mpc52xx_fec __iomem *fec = priv->fec;
+
+ /*
+ * If the MAC addresse is not provided via DT then read
+ * it back from the controller regs
+ */
+ *(u32 *)(&ndev->dev_addr[0]) = in_be32(&fec->paddr1);
+ *(u16 *)(&ndev->dev_addr[4]) = in_be32(&fec->paddr2) >> 16;
+ }
+
+ /*
+ * Check if the MAC address is valid, if not get a random one
+ */
+ if (!is_valid_ether_addr(ndev->dev_addr)) {
+ eth_hw_addr_random(ndev);
+ dev_warn(&ndev->dev, "using random MAC address %pM\n",
+ ndev->dev_addr);
+ }
priv->msg_enable = netif_msg_init(debug, MPC52xx_MESSAGES_DEFAULT);
@@ -942,20 +953,20 @@ static int mpc52xx_fec_probe(struct platform_device *op)
/* Start with safe defaults for link connection */
priv->speed = 100;
priv->duplex = DUPLEX_HALF;
- priv->mdio_speed = ((mpc5xxx_get_bus_frequency(op->dev.of_node) >> 20) / 5) << 1;
+ priv->mdio_speed = ((mpc5xxx_get_bus_frequency(np) >> 20) / 5) << 1;
/* The current speed preconfigures the speed of the MII link */
- prop = of_get_property(op->dev.of_node, "current-speed", &prop_size);
+ prop = of_get_property(np, "current-speed", &prop_size);
if (prop && (prop_size >= sizeof(u32) * 2)) {
priv->speed = prop[0];
priv->duplex = prop[1] ? DUPLEX_FULL : DUPLEX_HALF;
}
/* If there is a phy handle, then get the PHY node */
- priv->phy_node = of_parse_phandle(op->dev.of_node, "phy-handle", 0);
+ priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
/* the 7-wire property means don't use MII mode */
- if (of_find_property(op->dev.of_node, "fsl,7-wire-mode", NULL)) {
+ if (of_find_property(np, "fsl,7-wire-mode", NULL)) {
priv->seven_wire_mode = 1;
dev_info(&ndev->dev, "using 7-wire PHY mode\n");
}
@@ -970,6 +981,8 @@ static int mpc52xx_fec_probe(struct platform_device *op)
/* We're done ! */
dev_set_drvdata(&op->dev, ndev);
+ printk(KERN_INFO "%s: %s MAC %pM\n",
+ ndev->name, op->dev.of_node->full_name, ndev->dev_addr);
return 0;
--
1.8.1.3
^ permalink raw reply related
* RE: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
From: Bhushan Bharat-R65777 @ 2013-02-12 10:56 UTC (permalink / raw)
To: Stefan Roese, netdev@vger.kernel.org
Cc: linuxppc-dev@ozlabs.org, Anatolij Gustschin
In-Reply-To: <1360660088-27464-1-git-send-email-sr@denx.de>
> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+bharat.bhushan=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Ste=
fan Roese
> Sent: Tuesday, February 12, 2013 2:38 PM
> To: netdev@vger.kernel.org
> Cc: linuxppc-dev@ozlabs.org; Anatolij Gustschin
> Subject: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
>=20
> Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
> (U-Boot) to write the MAC address into the ethernet controller registers.=
The
> Linux driver should not rely on such a thing. So lets read the MAC addres=
s from
> the DT as it should be done here.
>=20
> The following priority is now used to read the MAC address:
>=20
> 1) First, try OF node MAC address, if not present or invalid, then:
>=20
> 2) Read from MAC address registers, if invalid, then:
Why we read from MAC registers if Linux should not rely on bootloader?
-Bharat
>=20
> 3) Log a warning message, and choose a random MAC address.
>=20
> This fixes a problem with a MPC5200 board that uses the SPL U-Boot versio=
n
> without FEC initialization before Linux booting for boot speedup.
>=20
> Additionally a status line is now be printed upon successful driver probi=
ng,
> also displaying this MAC address.
>=20
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
> v2:
> - Remove module parameter mpc52xx_fec_mac_addr
> - Priority for MAC address probing now is DT, controller regs
> If the resulting MAC address is invalid, a random address will
> be generated and used with a warning message
> - Use "np" variable to simplify the code
>=20
> drivers/net/ethernet/freescale/fec_mpc52xx.c | 61 +++++++++++++++++-----=
------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/freescale/fec_mpc52xx.c
> b/drivers/net/ethernet/freescale/fec_mpc52xx.c
> index 817d081..8b725f4 100644
> --- a/drivers/net/ethernet/freescale/fec_mpc52xx.c
> +++ b/drivers/net/ethernet/freescale/fec_mpc52xx.c
> @@ -76,10 +76,6 @@ static void mpc52xx_fec_stop(struct net_device *dev); =
static
> void mpc52xx_fec_start(struct net_device *dev); static void
> mpc52xx_fec_reset(struct net_device *dev);
>=20
> -static u8 mpc52xx_fec_mac_addr[6];
> -module_param_array_named(mac, mpc52xx_fec_mac_addr, byte, NULL, 0); -
> MODULE_PARM_DESC(mac, "six hex digits, ie. 0x1,0x2,0xc0,0x01,0xba,0xbe");
> -
> #define MPC52xx_MESSAGES_DEFAULT ( NETIF_MSG_DRV | NETIF_MSG_PROBE | \
> NETIF_MSG_LINK | NETIF_MSG_IFDOWN | NETIF_MSG_IFUP)
> static int debug =3D -1; /* the above default */
> @@ -110,15 +106,6 @@ static void mpc52xx_fec_set_paddr(struct net_device =
*dev,
> u8 *mac)
> out_be32(&fec->paddr2, (*(u16 *)(&mac[4]) << 16) | FEC_PADDR2_TYPE); }
>=20
> -static void mpc52xx_fec_get_paddr(struct net_device *dev, u8 *mac) -{
> - struct mpc52xx_fec_priv *priv =3D netdev_priv(dev);
> - struct mpc52xx_fec __iomem *fec =3D priv->fec;
> -
> - *(u32 *)(&mac[0]) =3D in_be32(&fec->paddr1);
> - *(u16 *)(&mac[4]) =3D in_be32(&fec->paddr2) >> 16;
> -}
> -
> static int mpc52xx_fec_set_mac_address(struct net_device *dev, void *add=
r) {
> struct sockaddr *sock =3D addr;
> @@ -853,6 +840,8 @@ static int mpc52xx_fec_probe(struct platform_device *=
op)
> struct resource mem;
> const u32 *prop;
> int prop_size;
> + struct device_node *np =3D op->dev.of_node;
> + const void *p;
>=20
> phys_addr_t rx_fifo;
> phys_addr_t tx_fifo;
> @@ -866,7 +855,7 @@ static int mpc52xx_fec_probe(struct platform_device *=
op)
> priv->ndev =3D ndev;
>=20
> /* Reserve FEC control zone */
> - rv =3D of_address_to_resource(op->dev.of_node, 0, &mem);
> + rv =3D of_address_to_resource(np, 0, &mem);
> if (rv) {
> printk(KERN_ERR DRIVER_NAME ": "
> "Error while parsing device node resource\n" ); @@ -
> 919,7 +908,7 @@ static int mpc52xx_fec_probe(struct platform_device *op)
>=20
> /* Get the IRQ we need one by one */
> /* Control */
> - ndev->irq =3D irq_of_parse_and_map(op->dev.of_node, 0);
> + ndev->irq =3D irq_of_parse_and_map(np, 0);
>=20
> /* RX */
> priv->r_irq =3D bcom_get_task_irq(priv->rx_dmatsk);
> @@ -927,11 +916,33 @@ static int mpc52xx_fec_probe(struct platform_device=
*op)
> /* TX */
> priv->t_irq =3D bcom_get_task_irq(priv->tx_dmatsk);
>=20
> - /* MAC address init */
> - if (!is_zero_ether_addr(mpc52xx_fec_mac_addr))
> - memcpy(ndev->dev_addr, mpc52xx_fec_mac_addr, 6);
> - else
> - mpc52xx_fec_get_paddr(ndev, ndev->dev_addr);
> + /*
> + * MAC address init:
> + *
> + * First try to read MAC address from DT
> + */
> + p =3D of_get_property(np, "local-mac-address", NULL);
> + if (p !=3D NULL) {
> + memcpy(ndev->dev_addr, p, 6);
> + } else {
> + struct mpc52xx_fec __iomem *fec =3D priv->fec;
> +
> + /*
> + * If the MAC addresse is not provided via DT then read
> + * it back from the controller regs
> + */
> + *(u32 *)(&ndev->dev_addr[0]) =3D in_be32(&fec->paddr1);
> + *(u16 *)(&ndev->dev_addr[4]) =3D in_be32(&fec->paddr2) >> 16;
> + }
> +
> + /*
> + * Check if the MAC address is valid, if not get a random one
> + */
> + if (!is_valid_ether_addr(ndev->dev_addr)) {
> + eth_hw_addr_random(ndev);
> + dev_warn(&ndev->dev, "using random MAC address %pM\n",
> + ndev->dev_addr);
> + }
>=20
> priv->msg_enable =3D netif_msg_init(debug, MPC52xx_MESSAGES_DEFAULT);
>=20
> @@ -942,20 +953,20 @@ static int mpc52xx_fec_probe(struct platform_device=
*op)
> /* Start with safe defaults for link connection */
> priv->speed =3D 100;
> priv->duplex =3D DUPLEX_HALF;
> - priv->mdio_speed =3D ((mpc5xxx_get_bus_frequency(op->dev.of_node) >> 20=
) /
> 5) << 1;
> + priv->mdio_speed =3D ((mpc5xxx_get_bus_frequency(np) >> 20) / 5) << 1;
>=20
> /* The current speed preconfigures the speed of the MII link */
> - prop =3D of_get_property(op->dev.of_node, "current-speed", &prop_size);
> + prop =3D of_get_property(np, "current-speed", &prop_size);
> if (prop && (prop_size >=3D sizeof(u32) * 2)) {
> priv->speed =3D prop[0];
> priv->duplex =3D prop[1] ? DUPLEX_FULL : DUPLEX_HALF;
> }
>=20
> /* If there is a phy handle, then get the PHY node */
> - priv->phy_node =3D of_parse_phandle(op->dev.of_node, "phy-handle", 0);
> + priv->phy_node =3D of_parse_phandle(np, "phy-handle", 0);
>=20
> /* the 7-wire property means don't use MII mode */
> - if (of_find_property(op->dev.of_node, "fsl,7-wire-mode", NULL)) {
> + if (of_find_property(np, "fsl,7-wire-mode", NULL)) {
> priv->seven_wire_mode =3D 1;
> dev_info(&ndev->dev, "using 7-wire PHY mode\n");
> }
> @@ -970,6 +981,8 @@ static int mpc52xx_fec_probe(struct platform_device *=
op)
>=20
> /* We're done ! */
> dev_set_drvdata(&op->dev, ndev);
> + printk(KERN_INFO "%s: %s MAC %pM\n",
> + ndev->name, op->dev.of_node->full_name, ndev->dev_addr);
>=20
> return 0;
>=20
> --
> 1.8.1.3
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
From: Stefan Roese @ 2013-02-12 11:03 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: netdev@vger.kernel.org, Anatolij Gustschin, David S. Miller,
linuxppc-dev@ozlabs.org
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D065A0271@039-SN2MPN1-023.039d.mgd.msft.net>
On 12.02.2013 11:56, Bhushan Bharat-R65777 wrote:
>> Until now, the MPC5200 FEC ethernet driver relied upon the bootloader
>> (U-Boot) to write the MAC address into the ethernet controller registers. The
>> Linux driver should not rely on such a thing. So lets read the MAC address from
>> the DT as it should be done here.
>>
>> The following priority is now used to read the MAC address:
>>
>> 1) First, try OF node MAC address, if not present or invalid, then:
>>
>> 2) Read from MAC address registers, if invalid, then:
>
> Why we read from MAC registers if Linux should not rely on bootloader?
It was suggested by David. Backwards compatibility. Here Davids comment
to my original patch which removed this register reading completely:
"
I don't think this is a conservative enough change.
You have to keep the MAC register reading code around, as a backup
code path in case the OF device node lacks a MAC address
"
Thanks,
Stefan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox