* Re: [PATCH 2/8] powerpc/eeh: Info to trace passed devices
From: Benjamin Herrenschmidt @ 2014-05-19 22:37 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <5379FD19.2020100@suse.de>
On Mon, 2014-05-19 at 14:46 +0200, Alexander Graf wrote:
> I don't see the point of VFIO knowing about guest addresses. They are
> not unique across a system and the whole idea that a VFIO device has to
> be owned by a guest is also pretty dubious.
>
> I suppose what you really care about here is just a token for a specific
> device? But why do you need one where we don't have tokens yet?
I think this is going to be needed when doing in-kernel acceleration
of some of the RTAS calls, but yes, Gavin, why do we need that now ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 6/8] powerpc: Extend syscall ppc_rtas()
From: Benjamin Herrenschmidt @ 2014-05-19 22:38 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <5379FF4C.7080202@suse.de>
On Mon, 2014-05-19 at 14:55 +0200, Alexander Graf wrote:
> On 14.05.14 06:12, Gavin Shan wrote:
> > Originally, syscall ppc_rtas() can be used to invoke RTAS call from
> > user space. Utility "errinjct" is using it to inject various errors
> > to the system for testing purpose. The patch intends to extend the
> > syscall to support both pSeries and PowerNV platform. With that,
> > RTAS and OPAL call can be invoked from user space. In turn, utility
> > "errinjct" can be supported on pSeries and PowerNV platform at same
> > time.
> >
> > The original syscall handler ppc_rtas() is renamed to ppc_firmware(),
> > which calls ppc_call_rtas() or ppc_call_opal() depending on the
> > running platform. The data transported between userland and kerenl is
>
> Please fix your spelling of kernel.
>
> > by "struct rtas_args". It's platform specific on how to use the data.
> >
> > Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
> I think the basic idea to maintain the same interface between PAPR and
> OPAL to user space is sound, but this is really Ben's call.
Yeah that worries me a bit, RTAS and OPAL are completely different
beasts.
We can keep that error injection separate from the rest of the EEH
enablement for now. I'll look at it when I get a chance.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 8/8] powerpc/powernv: Error injection infrastructure
From: Benjamin Herrenschmidt @ 2014-05-19 22:40 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537A015D.8030202@suse.de>
On Mon, 2014-05-19 at 15:04 +0200, Alexander Graf wrote:
> On 14.05.14 06:12, Gavin Shan wrote:
> > The patch intends to implement the error injection infrastructure
> > for PowerNV platform. The predetermined handlers will be called
> > according to the type of injected error (e.g. OpalErrinjctTypeIoaBusError).
> > For now, we just support PCI error injection. We need support
> > injecting other types of errors in future.
>
> Your token to a VFIO device is the VFIO fd. If you want to inject an
> error into that device, you should do it via that token. That gets you
> all permission problems solved for free.
>
> But I still didn't quite grasp why you need to do this. Why do we need
> to inject an error into a device via OPAL when we want to do EEH inside
> of a guest? Are you trying to emulate guest side error injection?
Yes, that's what he's trying to do but let's keep that separate from
the core EEH. I'd like the latter to be reviewed /fixed and upstream
first, then we can look at guest side injection.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Benjamin Herrenschmidt @ 2014-05-19 22:51 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1400538790.3289.305.camel@ul30vt.home>
On Mon, 2014-05-19 at 16:33 -0600, Alex Williamson wrote:
> On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
> > The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
> > to support EEH functionality for PCI devices, which have been
> > passed from host to guest via VFIO.
>
> Some comments throughout, but overall this seems to forgo every bit of
> the device ownership and protection model used by VFIO and lets the user
> pick arbitrary host devices and do various operations, mostly unchecked.
> That's not acceptable.
Right, I don't understand why we need that mapping for base EEH. The
VFIO fd owns the PE, it can do operations on that PE, it doesn't need to
know the guest token/address.
Only when we start doing in-kernel acceleration of RTAS do we need that
sort of mapping established, which we'll be able to do via something
akin to what we did with TCEs to ensure we validate that we have
ownership of the physical device.
But let's proceed step by step. First, have something that is strictly
EEH from qemu RTAS to VFIO via ioctl, and that requires no mapping of
any sort.
Cheers,
Ben.
> > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/Makefile | 1 +
> > arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
> > drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
> > include/uapi/linux/vfio.h | 57 +++
> > 4 files changed, 663 insertions(+)
> > create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
> >
> > diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> > index 63cebb9..2b15a03 100644
> > --- a/arch/powerpc/platforms/powernv/Makefile
> > +++ b/arch/powerpc/platforms/powernv/Makefile
> > @@ -6,5 +6,6 @@ obj-y += opal-msglog.o
> > obj-$(CONFIG_SMP) += smp.o
> > obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
> > obj-$(CONFIG_EEH) += eeh-ioda.o eeh-powernv.o
> > +obj-$(CONFIG_VFIO_EEH) += eeh-vfio.o
> > obj-$(CONFIG_PPC_SCOM) += opal-xscom.o
> > obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
> > diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
> > new file mode 100644
> > index 0000000..69d5f2d
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
> > @@ -0,0 +1,593 @@
> > +/*
> > + * The file intends to support EEH funtionality for those PCI devices,
> > + * which have been passed through from host to guest via VFIO. So this
> > + * file is naturally part of VFIO implementation on PowerNV platform.
> > + *
> > + * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/kvm_host.h>
> > +#include <linux/msi.h>
> > +#include <linux/pci.h>
> > +#include <linux/string.h>
> > +#include <linux/vfio.h>
> > +
> > +#include <asm/eeh.h>
> > +#include <asm/eeh_event.h>
> > +#include <asm/io.h>
> > +#include <asm/iommu.h>
> > +#include <asm/opal.h>
> > +#include <asm/msi_bitmap.h>
> > +#include <asm/pci-bridge.h>
> > +#include <asm/ppc-pci.h>
> > +#include <asm/tce.h>
> > +#include <asm/uaccess.h>
> > +
> > +#include "powernv.h"
> > +#include "pci.h"
> > +
> > +static int powernv_eeh_vfio_map(struct vfio_eeh_info *info)
> > +{
> > + struct pci_bus *bus, *pe_bus;
> > + struct pci_dev *pdev;
> > + struct eeh_dev *edev;
> > + struct eeh_pe *pe;
> > + int domain, bus_no, devfn;
> > +
> > + /* Host address */
> > + domain = info->map.host_domain;
> > + bus_no = (info->map.host_cfg_addr >> 8) & 0xff;
> > + devfn = info->map.host_cfg_addr & 0xff;
>
> Where are we validating that the user has any legitimate claim to be
> touching this device?
>
> > + /* Find PCI bus */
> > + bus = pci_find_bus(domain, bus_no);
> > + if (!bus) {
> > + pr_warn("%s: PCI bus %04x:%02x not found\n",
> > + __func__, domain, bus_no);
> > + return -ENODEV;
> > + }
> > +
> > + /* Find PCI device */
> > + pdev = pci_get_slot(bus, devfn);
> > + if (!pdev) {
> > + pr_warn("%s: PCI device %04x:%02x:%02x.%01x not found\n",
> > + __func__, domain, bus_no,
> > + PCI_SLOT(devfn), PCI_FUNC(devfn));
> > + return -ENODEV;
> > + }
> > +
> > + /* No EEH device - almost impossible */
> > + edev = pci_dev_to_eeh_dev(pdev);
> > + if (unlikely(!edev)) {
> > + pci_dev_put(pdev);
> > + pr_warn("%s: No EEH dev for PCI device %s\n",
> > + __func__, pci_name(pdev));
> > + return -ENODEV;
> > + }
> > +
> > + /* Doesn't support PE migration between different PHBs */
> > + pe = edev->pe;
> > + if (!eeh_pe_passed(pe)) {
> > + pe_bus = eeh_pe_bus_get(pe);
> > + BUG_ON(!pe_bus);
>
> Can a user trigger this maliciously?
>
> > +
> > + /* PE# has format 00BBSS00 */
> > + pe->guest_addr.buid = info->map.guest_buid;
> > + pe->guest_addr.pe_addr = pe_bus->number << 16;
> > + eeh_pe_set_passed(pe, true);
> > + } else if (pe->guest_addr.buid != info->map.guest_buid) {
> > + pci_dev_put(pdev);
> > + pr_warn("%s: Mismatched PHB BUID (0x%llx, 0x%llx)\n",
> > + __func__, pe->guest_addr.buid, info->map.guest_buid);
> > + return -EINVAL;
> > + }
> > +
> > + edev->guest_addr.buid = info->map.guest_buid;
> > + edev->guest_addr.config_addr = info->map.guest_cfg_addr;
> > + eeh_dev_set_passed(edev, true);
> > +
> > + pr_debug("EEH: Host PCI dev %s to %llx-%02x:%02x.%01x\n",
> > + pci_name(pdev), info->map.guest_buid,
> > + (info->map.guest_cfg_addr >> 8) & 0xFF,
> > + PCI_SLOT(info->map.guest_cfg_addr & 0xFF),
> > + PCI_FUNC(info->map.guest_cfg_addr & 0xFF));
> > +
> > + pci_dev_put(pdev);
> > + return 0;
> > +}
>
> So the effect of this function is that a user gets to setup an arbitrary
> guest mapping for an arbitrary host device and associated pe. Is that
> right? It seems bad.
>
> > +
> > +static int powernv_eeh_vfio_unmap(struct vfio_eeh_info *info)
> > +{
> > + struct eeh_vfio_pci_addr addr;
> > + struct pci_dev *pdev;
> > + struct eeh_dev *edev, *tmp;
> > + struct eeh_pe *pe;
> > + bool passed;
> > +
> > + /* Get EEH device */
> > + addr.buid = info->unmap.buid;
> > + addr.config_addr = info->unmap.cfg_addr;
> > + edev = eeh_vfio_dev_get(&addr);
>
> eeh_vfio_dev_get() just looks for a "passed" dev and a match for a well
> known address space. Seems very exploitable.
>
> > + if (!edev) {
> > + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> > + __func__, info->unmap.buid,
> > + (info->unmap.cfg_addr >> 8) & 0xFF,
> > + PCI_SLOT(info->unmap.cfg_addr & 0xFF),
> > + PCI_FUNC(info->unmap.cfg_addr & 0xFF));
> > + return -ENODEV;
> > + }
> > +
> > + /* Return EEH device */
> > + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
> > + eeh_dev_set_passed(edev, false);
> > + pdev = eeh_dev_to_pci_dev(edev);
> > + pr_debug("EEH: Host PCI dev %s returned\n",
> > + pdev ? pci_name(pdev) : "NULL");
> > +
> > + /* Return PE if no EEH device is owned by guest */
> > + pe = edev->pe;
> > + passed = false;
> > + eeh_pe_for_each_dev(pe, edev, tmp) {
> > + pdev = eeh_dev_to_pci_dev(edev);
> > + if (pdev && pdev->subordinate)
> > + continue;
> > +
> > + if (eeh_dev_passed(edev)) {
> > + passed = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!passed) {
> > + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
> > + eeh_pe_set_passed(pe, false);
> > + pr_debug("EEH: PHB#%x-PE#%x returned to host\n",
> > + pe->phb->global_number, pe->addr);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int powernv_eeh_vfio_set_option(struct vfio_eeh_info *info)
> > +{
> > + struct pnv_phb *phb;
> > + struct eeh_dev *edev;
> > + struct eeh_pe *pe;
> > + struct eeh_vfio_pci_addr addr;
> > + int opcode = info->option.option;
> > + int ret = 0;
> > +
> > + /* Check opcode */
> > + if (opcode < EEH_OPT_DISABLE || opcode > EEH_OPT_THAW_DMA) {
> > + pr_warn("%s: opcode %d out of range (%d, %d)\n",
> > + __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
> > + ret = 3;
>
> Please don't make up arbitrary return values.
>
> > + goto out;
> > + }
> > +
> > + /* Option "enable" uses PCI config address */
> > + if (opcode == EEH_OPT_ENABLE) {
> > + addr.buid = info->option.buid;
> > + addr.config_addr = (info->option.addr >> 8) & 0xFFFF;
> > + edev = eeh_vfio_dev_get(&addr);
> > + if (!edev) {
> > + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> > + __func__, addr.buid,
> > + (addr.config_addr >> 8) & 0xFF,
> > + PCI_SLOT(addr.config_addr & 0xFF),
> > + PCI_FUNC(addr.config_addr & 0xFF));
> > + ret = 7;
> > + goto out;
> > + }
> > + phb = edev->phb->private_data;
> > + } else {
> > + addr.buid = info->option.buid;
> > + addr.pe_addr = info->option.addr;
> > + pe = eeh_vfio_pe_get(&addr);
> > + if (!pe) {
> > + pr_warn("%s: Cannot find PE %llx:%x\n",
> > + __func__, addr.buid, addr.pe_addr);
> > + ret = 7;
> > + goto out;
> > + }
> > + phb = pe->phb->private_data;
> > + }
> > +
> > + /* Insure that the EEH stuff has been initialized */
> > + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> > + pr_warn("%s: EEH disabled on PHB#%d\n",
> > + __func__, phb->hose->global_number);
> > + ret = 7;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The EEH functionality has been enabled on all PEs
> > + * by default. So just return success. The same situation
> > + * would be applied while we disable EEH functionality.
> > + * However, the guest isn't expected to disable that
> > + * at all.
> > + */
> > + if (opcode == EEH_OPT_DISABLE ||
> > + opcode == EEH_OPT_ENABLE) {
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Call into the IODA dependent backend in order
> > + * to enable DMA or MMIO for the indicated PE.
> > + */
> > + if (phb->eeh_ops && phb->eeh_ops->set_option) {
> > + if (phb->eeh_ops->set_option(pe, opcode)) {
> > + pr_warn("%s: Failure from backend\n",
> > + __func__);
> > + ret = 1;
> > + }
> > + } else {
> > + pr_warn("%s: Unsupported request\n",
> > + __func__);
> > + ret = 7;
> > + }
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int powernv_eeh_vfio_get_addr(struct vfio_eeh_info *info)
> > +{
> > + struct pnv_phb *phb;
> > + struct eeh_dev *edev;
> > + struct eeh_vfio_pci_addr addr;
> > + int opcode = info->addr.option;
> > + int ret = 0;
> > +
> > + /* Check opcode */
> > + if (opcode != 0 && opcode != 1) {
> > + pr_warn("%s: opcode %d out of range (0, 1)\n",
> > + __func__, opcode);
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /* Find EEH device */
> > + addr.buid = info->addr.buid;
> > + addr.config_addr = (info->addr.cfg_addr >> 8 ) & 0xFFFF;
> > + edev = eeh_vfio_dev_get(&addr);
> > + if (!edev) {
> > + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> > + __func__, addr.buid,
> > + (addr.config_addr >> 8) & 0xFF,
> > + PCI_SLOT(addr.config_addr & 0xFF),
> > + PCI_FUNC(addr.config_addr & 0xFF));
> > + ret = 7;
> > + goto out;
> > + }
> > + phb = edev->phb->private_data;
> > +
> > + /* EEH enabled ? */
> > + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> > + pr_warn("%s: EEH disabled on PHB#%d\n",
> > + __func__, phb->hose->global_number);
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /* EEH device passed ? */
> > + if (!eeh_dev_passed(edev)) {
> > + pr_warn("%s: EEH dev %llx:%02x:%02x.%01x owned by host\n",
> > + __func__, addr.buid,
> > + (addr.config_addr >> 8) & 0xFF,
> > + PCI_SLOT(addr.config_addr & 0xFF),
> > + PCI_FUNC(addr.config_addr & 0xFF));
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Fill result according to opcode. We don't differentiate
> > + * PCI bus and device sensitive PE here.
> > + */
> > + if (opcode == 0)
> > + info->addr.ret = edev->pe->guest_addr.pe_addr;
> > + else
> > + info->addr.ret = 1;
> > +out:
> > + return ret;
> > +}
> > +
> > +static int powernv_eeh_vfio_get_state(struct vfio_eeh_info *info)
> > +{
> > + struct pnv_phb *phb;
> > + struct eeh_pe *pe;
> > + struct eeh_vfio_pci_addr addr;
> > + int result, ret = 0;
> > +
> > + /* Locate the PE */
> > + addr.buid = info->state.buid;
> > + addr.pe_addr = info->state.pe_addr;
> > + pe = eeh_vfio_pe_get(&addr);
> > + if (!pe) {
> > + pr_warn("%s: Cannot locate %llx:%x\n",
> > + __func__, addr.buid, addr.pe_addr);
> > + ret = 3;
> > + goto out;
> > + }
> > + phb = pe->phb->private_data;
> > +
> > + /* EEH enabled ? */
> > + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> > + pr_warn("%s: EEH disabled on PHB#%d\n",
> > + __func__, phb->hose->global_number);
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /* Call to the IOC dependent function */
> > + if (phb->eeh_ops && phb->eeh_ops->get_state) {
> > + result = phb->eeh_ops->get_state(pe);
> > +
> > + if (!(result & EEH_STATE_RESET_ACTIVE) &&
> > + (result & EEH_STATE_DMA_ENABLED) &&
> > + (result & EEH_STATE_MMIO_ENABLED))
> > + info->state.state = 0;
> > + else if (result & EEH_STATE_RESET_ACTIVE)
> > + info->state.state = 1;
> > + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> > + !(result & EEH_STATE_DMA_ENABLED) &&
> > + !(result & EEH_STATE_MMIO_ENABLED))
> > + info->state.state = 2;
> > + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> > + (result & EEH_STATE_DMA_ENABLED) &&
> > + !(result & EEH_STATE_MMIO_ENABLED))
> > + info->state.state = 4;
> > + else
> > + info->state.state = 5;
> > +
> > + ret = 0;
> > + } else {
> > + pr_warn("%s: Unsupported request\n", __func__);
> > + ret = 3;
> > + }
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int powernv_eeh_vfio_pe_reset(struct vfio_eeh_info *info)
> > +{
> > + struct pnv_phb *phb;
> > + struct eeh_pe *pe;
> > + struct eeh_vfio_pci_addr addr;
> > + int opcode = info->reset.option;
> > + int ret = 0;
> > +
> > + /* Check opcode */
> > + if (opcode != EEH_RESET_DEACTIVATE &&
> > + opcode != EEH_RESET_HOT &&
> > + opcode != EEH_RESET_FUNDAMENTAL) {
> > + pr_warn("%s: Unsupported opcode %d\n",
> > + __func__, opcode);
>
> Console warnings are exploitable DoS attacks.
>
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /* Locate the PE */
> > + addr.buid = info->reset.buid;
> > + addr.pe_addr = info->reset.pe_addr;
> > + pe = eeh_vfio_pe_get(&addr);
> > + if (!pe) {
> > + pr_warn("%s: Cannot locate %llx:%x\n",
> > + __func__, addr.buid, addr.pe_addr);
> > + ret = 3;
> > + goto out;
> > + }
> > + phb = pe->phb->private_data;
> > +
> > + /* EEH enabled ? */
> > + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> > + pr_warn("%s: EEH disabled on PHB#%d\n",
> > + __func__, phb->hose->global_number);
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /* Call into the IODA dependent backend to do the reset */
> > + if (!phb->eeh_ops ||
> > + !phb->eeh_ops->set_option ||
> > + !phb->eeh_ops->reset) {
> > + pr_warn("%s: Unsupported request\n",
> > + __func__);
> > + ret = 7;
> > + } else {
> > + /*
> > + * The frozen PE might be caused by the mechanism called
> > + * PAPR error injection, which is supposed to be one-shot
> > + * without "sticky" bit as being stated by the spec. But
> > + * the reality isn't that, at least on P7IOC. So we have
> > + * to clear that to avoid recrusive error, which fails the
> > + * recovery eventually.
> > + */
> > + if (opcode == EEH_RESET_DEACTIVATE)
> > + opal_pci_reset(phb->opal_id,
> > + OPAL_PHB_ERROR,
> > + OPAL_ASSERT_RESET);
> > +
> > + if (phb->eeh_ops->reset(pe, opcode)) {
> > + pr_warn("%s: Failure from backend\n", __func__);
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The PE is still in frozen state and we need clear that.
> > + * It's good to clear frozen state after deassert to avoid
> > + * messy IO access during reset, which might cause recrusive
> > + * frozen PE.
> > + */
> > + if (opcode == EEH_RESET_DEACTIVATE) {
> > + if (phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO) ||
> > + phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA)) {
> > + pr_warn("%s: Cannot clear frozen state\n",
> > + __func__);
> > + ret = 1;
> > + }
> > +
> > + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
> > + }
> > + }
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int powernv_eeh_vfio_pe_config(struct vfio_eeh_info *info)
> > +{
> > + struct pnv_phb *phb;
> > + struct eeh_pe *pe;
> > + struct eeh_vfio_pci_addr addr;
> > + int ret = 0;
> > +
> > + /* Locate the PE */
> > + addr.buid = info->config.buid;
> > + addr.pe_addr = info->config.pe_addr;
> > + pe = eeh_vfio_pe_get(&addr);
> > + if (!pe) {
> > + pr_warn("%s: Cannot locate %llx:%x\n",
> > + __func__, addr.buid, addr.pe_addr);
> > + ret = 3;
> > + goto out;
> > + }
> > + phb = pe->phb->private_data;
> > +
> > + /* EEH enabled ? */
> > + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> > + pr_warn("%s: EEH disabled on PHB#%d\n",
> > + __func__, phb->hose->global_number);
> > + ret = 3;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The access to PCI config space on VFIO device has some
> > + * limitations. Part of PCI config space, including BAR
> > + * registers are not readable and writable. So the guest
> > + * should have stale values for those registers and we have
> > + * to restore them in host side.
> > + */
> > + eeh_pe_restore_bars(pe);
> > +out:
> > + return ret;
> > +}
> > +
> > +void eeh_vfio_release(struct iommu_table *tbl)
> > +{
> > + struct pnv_ioda_pe *pnv_pe = container_of(tbl, struct pnv_ioda_pe,
> > + tce32_table);
> > + struct pnv_phb *phb = pnv_pe->phb;
> > + struct eeh_pe *phb_pe, *pe;
> > + struct eeh_dev dev, *edev, *tmp;
> > +
> > + /* Find PHB PE */
> > + phb_pe = eeh_phb_pe_get(phb->hose);
> > + if (unlikely(!phb_pe)) {
> > + pr_warn("%s: Cannot find PHB#%d PE\n",
> > + __func__, phb->hose->global_number);
> > + return;
> > + }
> > +
> > + /* Find PE */
> > + memset(&dev, 0, sizeof(struct eeh_dev));
> > + dev.phb = phb->hose;
> > + dev.pe_config_addr = pnv_pe->pe_number;
> > + pe = eeh_pe_get(&dev);
> > + if (unlikely(!pe)) {
> > + pr_warn("%s: Cannot find PE instance for PHB#%d-PE#%d\n",
> > + __func__, phb->hose->global_number,
> > + pnv_pe->pe_number);
> > + return;
> > + }
> > +
> > + /* Release it to host */
> > + if (!eeh_pe_passed(pe))
> > + return;
> > +
> > + eeh_pe_for_each_dev(pe, edev, tmp) {
> > + if (!eeh_dev_passed(edev))
> > + continue;
> > +
> > + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>
> Is guest_addr = { 0 } not valid? As agraf already mentioned, there are
> a number of issues with using a guest_address for a token.
>
> > + eeh_dev_set_passed(edev, false);
> > + }
> > +
> > + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
> > + eeh_pe_set_passed(pe, false);
> > +}
> > +EXPORT_SYMBOL(eeh_vfio_release);
> > +
> > +int eeh_vfio_ioctl(unsigned long arg)
> > +{
> > + struct vfio_eeh_info info;
> > + int ret = -EINVAL;
> > +
> > + /* Copy over user argument */
> > + if (copy_from_user(&info, (void __user *)arg, sizeof(info))) {
> > + pr_warn("%s: Cannot copy user argument 0x%lx\n",
> > + __func__, arg);
> > + return -EFAULT;
> > + }
> > +
> > + /* Sanity check */
> > + if (info.argsz != sizeof(info)) {
>
> This breaks compatibility if you need to later add a new ops with a
> larger footprint.
>
> > + pr_warn("%s: Invalid argument size (%d, %ld)\n",
> > + __func__, info.argsz, sizeof(info));
> > + return -EINVAL;
> > + }
> > +
> > + /* Route according to operation */
> > + switch (info.op) {
> > + case VFIO_EEH_OP_MAP:
> > + ret = powernv_eeh_vfio_map(&info);
> > + break;
> > + case VFIO_EEH_OP_UNMAP:
> > + ret = powernv_eeh_vfio_unmap(&info);
> > + break;
> > + case VFIO_EEH_OP_SET_OPTION:
> > + ret = powernv_eeh_vfio_set_option(&info);
> > + break;
> > + case VFIO_EEH_OP_GET_ADDR:
> > + ret = powernv_eeh_vfio_get_addr(&info);
> > + break;
> > + case VFIO_EEH_OP_GET_STATE:
> > + ret = powernv_eeh_vfio_get_state(&info);
> > + break;
> > + case VFIO_EEH_OP_PE_RESET:
> > + ret = powernv_eeh_vfio_pe_reset(&info);
> > + break;
> > + case VFIO_EEH_OP_PE_CONFIG:
> > + ret = powernv_eeh_vfio_pe_config(&info);
> > + break;
> > + default:
> > + pr_info("%s: Cannot handle op#%d\n",
> > + __func__, info.op);
> > + }
> > +
> > + /* Copy data back */
> > + if (!ret && copy_to_user((void __user *)arg, &info, sizeof(info))) {
> > + pr_warn("%s: Cannot copy to user 0x%lx\n",
> > + __func__, arg);
> > + return -EFAULT;
> > + }
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(eeh_vfio_ioctl);
> > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> > index a84788b..c45dece 100644
> > --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> > @@ -26,6 +26,11 @@
> > #define DRIVER_AUTHOR "aik@ozlabs.ru"
> > #define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >
> > +#ifdef CONFIG_VFIO_EEH
> > +extern void eeh_vfio_release(struct iommu_table *tbl);
> > +extern int eeh_vfio_ioctl(unsigned long arg);
> > +#endif
> > +
> > static void tce_iommu_detach_group(void *iommu_data,
> > struct iommu_group *iommu_group);
> >
> > @@ -283,6 +288,10 @@ static long tce_iommu_ioctl(void *iommu_data,
> > tce_iommu_disable(container);
> > mutex_unlock(&container->lock);
> > return 0;
> > +#ifdef CONFIG_VFIO_EEH
>
> I'm not a fan of all these #ifdefs, hide it in eeh_vfio_ioctl() and
> eeh_vfio_release() if needed.
>
> > + case VFIO_EEH_INFO:
> > + return eeh_vfio_ioctl(arg);
> > +#endif
> > }
> >
> > return -ENOTTY;
> > @@ -342,6 +351,9 @@ static void tce_iommu_detach_group(void *iommu_data,
> > /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
> > iommu_group_id(iommu_group), iommu_group); */
> > container->tbl = NULL;
> > +#ifdef CONFIG_VFIO_EEH
> > + eeh_vfio_release(tbl);
> > +#endif
> > iommu_release_ownership(tbl);
> > }
> > mutex_unlock(&container->lock);
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index cb9023d..1fd1bfb 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
> >
> > #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> >
> > +/*
> > + * The VFIO EEH info struct provides way to support EEH functionality
> > + * for PCI device that is passed from host to guest via VFIO.
> > + */
> > +#define VFIO_EEH_OP_MAP 0
> > +#define VFIO_EEH_OP_UNMAP 1
> > +#define VFIO_EEH_OP_SET_OPTION 2
> > +#define VFIO_EEH_OP_GET_ADDR 3
> > +#define VFIO_EEH_OP_GET_STATE 4
> > +#define VFIO_EEH_OP_PE_RESET 5
> > +#define VFIO_EEH_OP_PE_CONFIG 6
>
> Is this really an "info" ioctl?
>
> > +
> > +struct vfio_eeh_info {
> > + __u32 argsz;
> > + __u32 op;
> > +
> > + union {
> > + struct vfio_eeh_map {
> > + __u32 host_domain;
> > + __u16 host_cfg_addr;
> > + __u64 guest_buid;
> > + __u16 guest_cfg_addr;
> > + } map;
> > + struct vfio_eeh_unmap {
> > + __u64 buid;
> > + __u16 cfg_addr;
> > + } unmap;
> > + struct vfio_eeh_set_option {
> > + __u64 buid;
> > + __u32 addr;
> > + __u32 option;
> > + } option;
> > + struct vfio_eeh_pe_addr {
> > + __u64 buid;
> > + __u32 cfg_addr;
> > + __u32 option;
> > + __u32 ret;
> > + } addr;
> > + struct vfio_eeh_state {
> > + __u64 buid;
> > + __u32 pe_addr;
> > + __u32 state;
> > + } state;
> > + struct vfio_eeh_reset {
> > + __u64 buid;
> > + __u32 pe_addr;
> > + __u32 option;
> > + } reset;
> > + struct vfio_eeh_config {
> > + __u64 buid;
> > + __u32 pe_addr;
> > + } config;
> > + };
> > +};
> > +
> > +#define VFIO_EEH_INFO _IO(VFIO_TYPE, VFIO_BASE + 21)
> > +
> > /* ***************************************************************** */
> >
> > #endif /* _UAPIVFIO_H */
>
>
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Hugh Dickins @ 2014-05-19 23:23 UTC (permalink / raw)
To: Madhavan Srinivasan
Cc: linux-arch, riel, x86, dave.hansen, peterz, Rusty Russell,
linux-kernel, linux-mm, ak, paulus, mgorman, akpm, linuxppc-dev,
mingo, Kirill A. Shutemov
In-Reply-To: <53797511.1050409@linux.vnet.ibm.com>
On Mon, 19 May 2014, Madhavan Srinivasan wrote:
> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
> > Hugh Dickins <hughd@google.com> writes:
> >> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
> >>>
> >>> Hi Ingo,
> >>>
> >>> Do you have any comments for the latest version of the patchset. If
> >>> not, kindly can you pick it up as is.
> >>>
> >>>
> >>> With regards
> >>> Maddy
> >>>
> >>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
> >>>> vm_ops->map_pages() for mapping easy accessible pages around
> >>>> fault address in hope to reduce number of minor page faults.
> >>>>
> >>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
> >>>> value using mm/Kconfig. This will enable architecture maintainers
> >>>> to decide on suitable FAULT_AROUND_ORDER value based on
> >>>> performance data for that architecture. First patch also defaults
> >>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
> >>>> out the performance numbers for powerpc (platform pseries) and
> >>>> initialize the fault around order variable for pseries platform of
> >>>> powerpc.
> >>
> >> Sorry for not commenting earlier - just reminded by this ping to Ingo.
> >>
> >> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
> >>
> >> arch/powerpc/Kconfig suggests that Power supports base page size of
> >> 4k, 16k, 64k or 256k.
> >>
> >> I would expect your optimal fault_around_order to depend very much on
> >> the base page size.
> >
> > It was 64k, which is what PPC64 uses on all the major distributions.
> > You really only get a choice of 4k and 64k with 64 bit power.
> >
> This is true. PPC64 support multiple pagesize and yes the default page
> size of 64k, is taken as base pagesize for the tests.
>
> >> Perhaps fault_around_size would provide a more useful default?
> >
> > That seems to fit. With 4k pages and order 4, you're asking for 64k.
> > Maddy's result shows 64k is also reasonable for 64k pages.
> >
> > Perhaps we try to generalize from two data points (a slight improvement
> > over doing it from 1!), eg:
> >
> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
> > unsigned int fault_around_order __read_mostly =
> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Rusty's bimodal answer doesn't seem the right starting point to me.
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
- when that doesn't wrap, of course!
That would at least have a better chance of being appropriate for
architectures with 8k and 16k pages (Itanium springs to mind).
Not necessarily right for them, since each architecture may have
different faulting overheads; but a better chance of being right
than blindly assuming 4k or 64k pages for everyone.
I'd be glad to see that change go into v3.15: what do you think,
Kirill, are we too late to make such a change now?
Or do you see some objection to it?
> This may be right. But these are the concerns, will not this make other
> arch to pick default without any tuning
Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
Did other architectures, with other page sizes, back that default?
Clearly not powerpc.
> and also this will remove the
> compile time option to disable the feature?
Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
for v3.16?
I'm not sure whether Rusty was arguing against that or not. I think
we are all three concerned to have a more sensible default than what's
there at present. I don't feel very strongly about your Kconfig
option: I've no objection, if it were to default to byte order 16.
Hugh
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Andrew Morton @ 2014-05-19 23:43 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-arch, x86, riel, Madhavan Srinivasan, dave.hansen, peterz,
Rusty Russell, linux-kernel, linux-mm, ak, paulus, mgorman,
linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
^ permalink raw reply
* Re: [PATCH 2/8] powerpc/eeh: Info to trace passed devices
From: Gavin Shan @ 2014-05-19 23:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: aik, Alexander Graf, kvm-ppc, Gavin Shan, alex.williamson,
qiudayu, linuxppc-dev
In-Reply-To: <1400539049.3986.71.camel@pasglop>
On Tue, May 20, 2014 at 08:37:29AM +1000, Benjamin Herrenschmidt wrote:
>On Mon, 2014-05-19 at 14:46 +0200, Alexander Graf wrote:
>> I don't see the point of VFIO knowing about guest addresses. They are
>> not unique across a system and the whole idea that a VFIO device has to
>> be owned by a guest is also pretty dubious.
>>
>> I suppose what you really care about here is just a token for a specific
>> device? But why do you need one where we don't have tokens yet?
>
>I think this is going to be needed when doing in-kernel acceleration
>of some of the RTAS calls, but yes, Gavin, why do we need that now ?
>
Thanks for your comments :-)
For now, the newly introduced IOCTL command (VFIO_EEH_INFO) is handled by
VFIO-Container fd, which can't identify IOMMU group (PE) or VFIO-PCI-dev
naturally because one VFIO-container can have multiple IOMMU groups docked
there. I'm going to let VFIO-PCI-dev fd handle the IOCTL command because
VFIO-PCI-dev fd can identify the device and the corresponding IOMMU group
(PE). With that, the address mapping stuff isn't necessary.
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-19 23:57 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <5379FE4F.6070706@suse.de>
On Mon, May 19, 2014 at 02:51:27PM +0200, Alexander Graf wrote:
>
>On 14.05.14 06:11, Gavin Shan wrote:
>>The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>>to support EEH functionality for PCI devices, which have been
>>passed from host to guest via VFIO.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>---
>> arch/powerpc/platforms/powernv/Makefile | 1 +
>> arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
>> drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
>> include/uapi/linux/vfio.h | 57 +++
>
>Documentation/...?
>
Will amend it in next revision. Thanks for reminder.
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Gavin Shan @ 2014-05-20 0:22 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1400538790.3289.305.camel@ul30vt.home>
On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
>On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
>> The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
>> to support EEH functionality for PCI devices, which have been
>> passed from host to guest via VFIO.
Thanks for your comments, Alex.W :-)
>
>Some comments throughout, but overall this seems to forgo every bit of
>the device ownership and protection model used by VFIO and lets the user
>pick arbitrary host devices and do various operations, mostly unchecked.
>That's not acceptable.
>
As what I replied to patch[2], I'm going to let VFIO-PCI-dev fd handle
the newly introduced IOCTL command. That way, we should follow the VFIO
design principles (ownership and protection) because VFIO-PCI-dev fd
is owned by QEMU process usually.
Also, the address mapping maintained in EEH will be removed.
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/powernv/Makefile | 1 +
>> arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
>> drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
>> include/uapi/linux/vfio.h | 57 +++
>> 4 files changed, 663 insertions(+)
>> create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
>>
>> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
>> index 63cebb9..2b15a03 100644
>> --- a/arch/powerpc/platforms/powernv/Makefile
>> +++ b/arch/powerpc/platforms/powernv/Makefile
>> @@ -6,5 +6,6 @@ obj-y += opal-msglog.o
>> obj-$(CONFIG_SMP) += smp.o
>> obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
>> obj-$(CONFIG_EEH) += eeh-ioda.o eeh-powernv.o
>> +obj-$(CONFIG_VFIO_EEH) += eeh-vfio.o
>> obj-$(CONFIG_PPC_SCOM) += opal-xscom.o
>> obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
>> diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
>> new file mode 100644
>> index 0000000..69d5f2d
>> --- /dev/null
>> +++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
>> @@ -0,0 +1,593 @@
>> +/*
>> + * The file intends to support EEH funtionality for those PCI devices,
>> + * which have been passed through from host to guest via VFIO. So this
>> + * file is naturally part of VFIO implementation on PowerNV platform.
>> + *
>> + * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/kvm_host.h>
>> +#include <linux/msi.h>
>> +#include <linux/pci.h>
>> +#include <linux/string.h>
>> +#include <linux/vfio.h>
>> +
>> +#include <asm/eeh.h>
>> +#include <asm/eeh_event.h>
>> +#include <asm/io.h>
>> +#include <asm/iommu.h>
>> +#include <asm/opal.h>
>> +#include <asm/msi_bitmap.h>
>> +#include <asm/pci-bridge.h>
>> +#include <asm/ppc-pci.h>
>> +#include <asm/tce.h>
>> +#include <asm/uaccess.h>
>> +
>> +#include "powernv.h"
>> +#include "pci.h"
>> +
>> +static int powernv_eeh_vfio_map(struct vfio_eeh_info *info)
>> +{
>> + struct pci_bus *bus, *pe_bus;
>> + struct pci_dev *pdev;
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + int domain, bus_no, devfn;
>> +
>> + /* Host address */
>> + domain = info->map.host_domain;
>> + bus_no = (info->map.host_cfg_addr >> 8) & 0xff;
>> + devfn = info->map.host_cfg_addr & 0xff;
>
>Where are we validating that the user has any legitimate claim to be
>touching this device?
>
I'll let VFIO-PCI-dev fd handle the IOCTL command. With that, we shouldn't
have the problem.
>> + /* Find PCI bus */
>> + bus = pci_find_bus(domain, bus_no);
>> + if (!bus) {
>> + pr_warn("%s: PCI bus %04x:%02x not found\n",
>> + __func__, domain, bus_no);
>> + return -ENODEV;
>> + }
>> +
>> + /* Find PCI device */
>> + pdev = pci_get_slot(bus, devfn);
>> + if (!pdev) {
>> + pr_warn("%s: PCI device %04x:%02x:%02x.%01x not found\n",
>> + __func__, domain, bus_no,
>> + PCI_SLOT(devfn), PCI_FUNC(devfn));
>> + return -ENODEV;
>> + }
>> +
>> + /* No EEH device - almost impossible */
>> + edev = pci_dev_to_eeh_dev(pdev);
>> + if (unlikely(!edev)) {
>> + pci_dev_put(pdev);
>> + pr_warn("%s: No EEH dev for PCI device %s\n",
>> + __func__, pci_name(pdev));
>> + return -ENODEV;
>> + }
>> +
>> + /* Doesn't support PE migration between different PHBs */
>> + pe = edev->pe;
>> + if (!eeh_pe_passed(pe)) {
>> + pe_bus = eeh_pe_bus_get(pe);
>> + BUG_ON(!pe_bus);
>
>Can a user trigger this maliciously?
>
>> +
>> + /* PE# has format 00BBSS00 */
>> + pe->guest_addr.buid = info->map.guest_buid;
>> + pe->guest_addr.pe_addr = pe_bus->number << 16;
>> + eeh_pe_set_passed(pe, true);
>> + } else if (pe->guest_addr.buid != info->map.guest_buid) {
>> + pci_dev_put(pdev);
>> + pr_warn("%s: Mismatched PHB BUID (0x%llx, 0x%llx)\n",
>> + __func__, pe->guest_addr.buid, info->map.guest_buid);
>> + return -EINVAL;
>> + }
>> +
>> + edev->guest_addr.buid = info->map.guest_buid;
>> + edev->guest_addr.config_addr = info->map.guest_cfg_addr;
>> + eeh_dev_set_passed(edev, true);
>> +
>> + pr_debug("EEH: Host PCI dev %s to %llx-%02x:%02x.%01x\n",
>> + pci_name(pdev), info->map.guest_buid,
>> + (info->map.guest_cfg_addr >> 8) & 0xFF,
>> + PCI_SLOT(info->map.guest_cfg_addr & 0xFF),
>> + PCI_FUNC(info->map.guest_cfg_addr & 0xFF));
>> +
>> + pci_dev_put(pdev);
>> + return 0;
>> +}
>
>So the effect of this function is that a user gets to setup an arbitrary
>guest mapping for an arbitrary host device and associated pe. Is that
>right? It seems bad.
>
I'm going to remove this mapping in next revision.
>> +
>> +static int powernv_eeh_vfio_unmap(struct vfio_eeh_info *info)
>> +{
>> + struct eeh_vfio_pci_addr addr;
>> + struct pci_dev *pdev;
>> + struct eeh_dev *edev, *tmp;
>> + struct eeh_pe *pe;
>> + bool passed;
>> +
>> + /* Get EEH device */
>> + addr.buid = info->unmap.buid;
>> + addr.config_addr = info->unmap.cfg_addr;
>> + edev = eeh_vfio_dev_get(&addr);
>
>eeh_vfio_dev_get() just looks for a "passed" dev and a match for a well
>known address space. Seems very exploitable.
>
>> + if (!edev) {
>> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> + __func__, info->unmap.buid,
>> + (info->unmap.cfg_addr >> 8) & 0xFF,
>> + PCI_SLOT(info->unmap.cfg_addr & 0xFF),
>> + PCI_FUNC(info->unmap.cfg_addr & 0xFF));
>> + return -ENODEV;
>> + }
>> +
>> + /* Return EEH device */
>> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>> + eeh_dev_set_passed(edev, false);
>> + pdev = eeh_dev_to_pci_dev(edev);
>> + pr_debug("EEH: Host PCI dev %s returned\n",
>> + pdev ? pci_name(pdev) : "NULL");
>> +
>> + /* Return PE if no EEH device is owned by guest */
>> + pe = edev->pe;
>> + passed = false;
>> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> + pdev = eeh_dev_to_pci_dev(edev);
>> + if (pdev && pdev->subordinate)
>> + continue;
>> +
>> + if (eeh_dev_passed(edev)) {
>> + passed = true;
>> + break;
>> + }
>> + }
>> +
>> + if (!passed) {
>> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> + eeh_pe_set_passed(pe, false);
>> + pr_debug("EEH: PHB#%x-PE#%x returned to host\n",
>> + pe->phb->global_number, pe->addr);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int powernv_eeh_vfio_set_option(struct vfio_eeh_info *info)
>> +{
>> + struct pnv_phb *phb;
>> + struct eeh_dev *edev;
>> + struct eeh_pe *pe;
>> + struct eeh_vfio_pci_addr addr;
>> + int opcode = info->option.option;
>> + int ret = 0;
>> +
>> + /* Check opcode */
>> + if (opcode < EEH_OPT_DISABLE || opcode > EEH_OPT_THAW_DMA) {
>> + pr_warn("%s: opcode %d out of range (%d, %d)\n",
>> + __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>> + ret = 3;
>
>Please don't make up arbitrary return values.
>
Nope, it will be turned to "-3" eventually by QEMU. That means "Invalid Parameter"
defined in PAPR spec.
The IOCTL command handler return 3 values:
< 0: Linux kernel error. For example, error from copy_from_user().
> 0: Error code to the EEH RTAS request, which will be returned to guest.
= 0: Success
>> + goto out;
>> + }
>> +
>> + /* Option "enable" uses PCI config address */
>> + if (opcode == EEH_OPT_ENABLE) {
>> + addr.buid = info->option.buid;
>> + addr.config_addr = (info->option.addr >> 8) & 0xFFFF;
>> + edev = eeh_vfio_dev_get(&addr);
>> + if (!edev) {
>> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> + __func__, addr.buid,
>> + (addr.config_addr >> 8) & 0xFF,
>> + PCI_SLOT(addr.config_addr & 0xFF),
>> + PCI_FUNC(addr.config_addr & 0xFF));
>> + ret = 7;
>> + goto out;
>> + }
>> + phb = edev->phb->private_data;
>> + } else {
>> + addr.buid = info->option.buid;
>> + addr.pe_addr = info->option.addr;
>> + pe = eeh_vfio_pe_get(&addr);
>> + if (!pe) {
>> + pr_warn("%s: Cannot find PE %llx:%x\n",
>> + __func__, addr.buid, addr.pe_addr);
>> + ret = 7;
>> + goto out;
>> + }
>> + phb = pe->phb->private_data;
>> + }
>> +
>> + /* Insure that the EEH stuff has been initialized */
>> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> + __func__, phb->hose->global_number);
>> + ret = 7;
>> + goto out;
>> + }
>> +
>> + /*
>> + * The EEH functionality has been enabled on all PEs
>> + * by default. So just return success. The same situation
>> + * would be applied while we disable EEH functionality.
>> + * However, the guest isn't expected to disable that
>> + * at all.
>> + */
>> + if (opcode == EEH_OPT_DISABLE ||
>> + opcode == EEH_OPT_ENABLE) {
>> + ret = 0;
>> + goto out;
>> + }
>> +
>> + /*
>> + * Call into the IODA dependent backend in order
>> + * to enable DMA or MMIO for the indicated PE.
>> + */
>> + if (phb->eeh_ops && phb->eeh_ops->set_option) {
>> + if (phb->eeh_ops->set_option(pe, opcode)) {
>> + pr_warn("%s: Failure from backend\n",
>> + __func__);
>> + ret = 1;
>> + }
>> + } else {
>> + pr_warn("%s: Unsupported request\n",
>> + __func__);
>> + ret = 7;
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static int powernv_eeh_vfio_get_addr(struct vfio_eeh_info *info)
>> +{
>> + struct pnv_phb *phb;
>> + struct eeh_dev *edev;
>> + struct eeh_vfio_pci_addr addr;
>> + int opcode = info->addr.option;
>> + int ret = 0;
>> +
>> + /* Check opcode */
>> + if (opcode != 0 && opcode != 1) {
>> + pr_warn("%s: opcode %d out of range (0, 1)\n",
>> + __func__, opcode);
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /* Find EEH device */
>> + addr.buid = info->addr.buid;
>> + addr.config_addr = (info->addr.cfg_addr >> 8 ) & 0xFFFF;
>> + edev = eeh_vfio_dev_get(&addr);
>> + if (!edev) {
>> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
>> + __func__, addr.buid,
>> + (addr.config_addr >> 8) & 0xFF,
>> + PCI_SLOT(addr.config_addr & 0xFF),
>> + PCI_FUNC(addr.config_addr & 0xFF));
>> + ret = 7;
>> + goto out;
>> + }
>> + phb = edev->phb->private_data;
>> +
>> + /* EEH enabled ? */
>> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> + __func__, phb->hose->global_number);
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /* EEH device passed ? */
>> + if (!eeh_dev_passed(edev)) {
>> + pr_warn("%s: EEH dev %llx:%02x:%02x.%01x owned by host\n",
>> + __func__, addr.buid,
>> + (addr.config_addr >> 8) & 0xFF,
>> + PCI_SLOT(addr.config_addr & 0xFF),
>> + PCI_FUNC(addr.config_addr & 0xFF));
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /*
>> + * Fill result according to opcode. We don't differentiate
>> + * PCI bus and device sensitive PE here.
>> + */
>> + if (opcode == 0)
>> + info->addr.ret = edev->pe->guest_addr.pe_addr;
>> + else
>> + info->addr.ret = 1;
>> +out:
>> + return ret;
>> +}
>> +
>> +static int powernv_eeh_vfio_get_state(struct vfio_eeh_info *info)
>> +{
>> + struct pnv_phb *phb;
>> + struct eeh_pe *pe;
>> + struct eeh_vfio_pci_addr addr;
>> + int result, ret = 0;
>> +
>> + /* Locate the PE */
>> + addr.buid = info->state.buid;
>> + addr.pe_addr = info->state.pe_addr;
>> + pe = eeh_vfio_pe_get(&addr);
>> + if (!pe) {
>> + pr_warn("%s: Cannot locate %llx:%x\n",
>> + __func__, addr.buid, addr.pe_addr);
>> + ret = 3;
>> + goto out;
>> + }
>> + phb = pe->phb->private_data;
>> +
>> + /* EEH enabled ? */
>> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> + __func__, phb->hose->global_number);
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /* Call to the IOC dependent function */
>> + if (phb->eeh_ops && phb->eeh_ops->get_state) {
>> + result = phb->eeh_ops->get_state(pe);
>> +
>> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + (result & EEH_STATE_DMA_ENABLED) &&
>> + (result & EEH_STATE_MMIO_ENABLED))
>> + info->state.state = 0;
>> + else if (result & EEH_STATE_RESET_ACTIVE)
>> + info->state.state = 1;
>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + !(result & EEH_STATE_DMA_ENABLED) &&
>> + !(result & EEH_STATE_MMIO_ENABLED))
>> + info->state.state = 2;
>> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>> + (result & EEH_STATE_DMA_ENABLED) &&
>> + !(result & EEH_STATE_MMIO_ENABLED))
>> + info->state.state = 4;
>> + else
>> + info->state.state = 5;
>> +
>> + ret = 0;
>> + } else {
>> + pr_warn("%s: Unsupported request\n", __func__);
>> + ret = 3;
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static int powernv_eeh_vfio_pe_reset(struct vfio_eeh_info *info)
>> +{
>> + struct pnv_phb *phb;
>> + struct eeh_pe *pe;
>> + struct eeh_vfio_pci_addr addr;
>> + int opcode = info->reset.option;
>> + int ret = 0;
>> +
>> + /* Check opcode */
>> + if (opcode != EEH_RESET_DEACTIVATE &&
>> + opcode != EEH_RESET_HOT &&
>> + opcode != EEH_RESET_FUNDAMENTAL) {
>> + pr_warn("%s: Unsupported opcode %d\n",
>> + __func__, opcode);
>
>Console warnings are exploitable DoS attacks.
>
Yep. I'll change all pr_warn() to pr_debug() in next revision.
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /* Locate the PE */
>> + addr.buid = info->reset.buid;
>> + addr.pe_addr = info->reset.pe_addr;
>> + pe = eeh_vfio_pe_get(&addr);
>> + if (!pe) {
>> + pr_warn("%s: Cannot locate %llx:%x\n",
>> + __func__, addr.buid, addr.pe_addr);
>> + ret = 3;
>> + goto out;
>> + }
>> + phb = pe->phb->private_data;
>> +
>> + /* EEH enabled ? */
>> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> + __func__, phb->hose->global_number);
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /* Call into the IODA dependent backend to do the reset */
>> + if (!phb->eeh_ops ||
>> + !phb->eeh_ops->set_option ||
>> + !phb->eeh_ops->reset) {
>> + pr_warn("%s: Unsupported request\n",
>> + __func__);
>> + ret = 7;
>> + } else {
>> + /*
>> + * The frozen PE might be caused by the mechanism called
>> + * PAPR error injection, which is supposed to be one-shot
>> + * without "sticky" bit as being stated by the spec. But
>> + * the reality isn't that, at least on P7IOC. So we have
>> + * to clear that to avoid recrusive error, which fails the
>> + * recovery eventually.
>> + */
>> + if (opcode == EEH_RESET_DEACTIVATE)
>> + opal_pci_reset(phb->opal_id,
>> + OPAL_PHB_ERROR,
>> + OPAL_ASSERT_RESET);
>> +
>> + if (phb->eeh_ops->reset(pe, opcode)) {
>> + pr_warn("%s: Failure from backend\n", __func__);
>> + ret = 1;
>> + goto out;
>> + }
>> +
>> + /*
>> + * The PE is still in frozen state and we need clear that.
>> + * It's good to clear frozen state after deassert to avoid
>> + * messy IO access during reset, which might cause recrusive
>> + * frozen PE.
>> + */
>> + if (opcode == EEH_RESET_DEACTIVATE) {
>> + if (phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO) ||
>> + phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA)) {
>> + pr_warn("%s: Cannot clear frozen state\n",
>> + __func__);
>> + ret = 1;
>> + }
>> +
>> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>> + }
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static int powernv_eeh_vfio_pe_config(struct vfio_eeh_info *info)
>> +{
>> + struct pnv_phb *phb;
>> + struct eeh_pe *pe;
>> + struct eeh_vfio_pci_addr addr;
>> + int ret = 0;
>> +
>> + /* Locate the PE */
>> + addr.buid = info->config.buid;
>> + addr.pe_addr = info->config.pe_addr;
>> + pe = eeh_vfio_pe_get(&addr);
>> + if (!pe) {
>> + pr_warn("%s: Cannot locate %llx:%x\n",
>> + __func__, addr.buid, addr.pe_addr);
>> + ret = 3;
>> + goto out;
>> + }
>> + phb = pe->phb->private_data;
>> +
>> + /* EEH enabled ? */
>> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
>> + pr_warn("%s: EEH disabled on PHB#%d\n",
>> + __func__, phb->hose->global_number);
>> + ret = 3;
>> + goto out;
>> + }
>> +
>> + /*
>> + * The access to PCI config space on VFIO device has some
>> + * limitations. Part of PCI config space, including BAR
>> + * registers are not readable and writable. So the guest
>> + * should have stale values for those registers and we have
>> + * to restore them in host side.
>> + */
>> + eeh_pe_restore_bars(pe);
>> +out:
>> + return ret;
>> +}
>> +
>> +void eeh_vfio_release(struct iommu_table *tbl)
>> +{
>> + struct pnv_ioda_pe *pnv_pe = container_of(tbl, struct pnv_ioda_pe,
>> + tce32_table);
>> + struct pnv_phb *phb = pnv_pe->phb;
>> + struct eeh_pe *phb_pe, *pe;
>> + struct eeh_dev dev, *edev, *tmp;
>> +
>> + /* Find PHB PE */
>> + phb_pe = eeh_phb_pe_get(phb->hose);
>> + if (unlikely(!phb_pe)) {
>> + pr_warn("%s: Cannot find PHB#%d PE\n",
>> + __func__, phb->hose->global_number);
>> + return;
>> + }
>> +
>> + /* Find PE */
>> + memset(&dev, 0, sizeof(struct eeh_dev));
>> + dev.phb = phb->hose;
>> + dev.pe_config_addr = pnv_pe->pe_number;
>> + pe = eeh_pe_get(&dev);
>> + if (unlikely(!pe)) {
>> + pr_warn("%s: Cannot find PE instance for PHB#%d-PE#%d\n",
>> + __func__, phb->hose->global_number,
>> + pnv_pe->pe_number);
>> + return;
>> + }
>> +
>> + /* Release it to host */
>> + if (!eeh_pe_passed(pe))
>> + return;
>> +
>> + eeh_pe_for_each_dev(pe, edev, tmp) {
>> + if (!eeh_dev_passed(edev))
>> + continue;
>> +
>> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
>
>Is guest_addr = { 0 } not valid? As agraf already mentioned, there are
>a number of issues with using a guest_address for a token.
>
For now, PHB BUID can't be "0". Originally, I was planing to have some code
in QEMU to have unique PHB BUID across the system so that guest_address could
be the unique token. But I'm going to remove the address mapping in next revision
as Alex.G suggested.
>> + eeh_dev_set_passed(edev, false);
>> + }
>> +
>> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
>> + eeh_pe_set_passed(pe, false);
>> +}
>> +EXPORT_SYMBOL(eeh_vfio_release);
>> +
>> +int eeh_vfio_ioctl(unsigned long arg)
>> +{
>> + struct vfio_eeh_info info;
>> + int ret = -EINVAL;
>> +
>> + /* Copy over user argument */
>> + if (copy_from_user(&info, (void __user *)arg, sizeof(info))) {
>> + pr_warn("%s: Cannot copy user argument 0x%lx\n",
>> + __func__, arg);
>> + return -EFAULT;
>> + }
>> +
>> + /* Sanity check */
>> + if (info.argsz != sizeof(info)) {
>
>This breaks compatibility if you need to later add a new ops with a
>larger footprint.
>
Ok. I'll fix it in next revision. Thanks for pointing it out.
>> + pr_warn("%s: Invalid argument size (%d, %ld)\n",
>> + __func__, info.argsz, sizeof(info));
>> + return -EINVAL;
>> + }
>> +
>> + /* Route according to operation */
>> + switch (info.op) {
>> + case VFIO_EEH_OP_MAP:
>> + ret = powernv_eeh_vfio_map(&info);
>> + break;
>> + case VFIO_EEH_OP_UNMAP:
>> + ret = powernv_eeh_vfio_unmap(&info);
>> + break;
>> + case VFIO_EEH_OP_SET_OPTION:
>> + ret = powernv_eeh_vfio_set_option(&info);
>> + break;
>> + case VFIO_EEH_OP_GET_ADDR:
>> + ret = powernv_eeh_vfio_get_addr(&info);
>> + break;
>> + case VFIO_EEH_OP_GET_STATE:
>> + ret = powernv_eeh_vfio_get_state(&info);
>> + break;
>> + case VFIO_EEH_OP_PE_RESET:
>> + ret = powernv_eeh_vfio_pe_reset(&info);
>> + break;
>> + case VFIO_EEH_OP_PE_CONFIG:
>> + ret = powernv_eeh_vfio_pe_config(&info);
>> + break;
>> + default:
>> + pr_info("%s: Cannot handle op#%d\n",
>> + __func__, info.op);
>> + }
>> +
>> + /* Copy data back */
>> + if (!ret && copy_to_user((void __user *)arg, &info, sizeof(info))) {
>> + pr_warn("%s: Cannot copy to user 0x%lx\n",
>> + __func__, arg);
>> + return -EFAULT;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(eeh_vfio_ioctl);
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index a84788b..c45dece 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -26,6 +26,11 @@
>> #define DRIVER_AUTHOR "aik@ozlabs.ru"
>> #define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>>
>> +#ifdef CONFIG_VFIO_EEH
>> +extern void eeh_vfio_release(struct iommu_table *tbl);
>> +extern int eeh_vfio_ioctl(unsigned long arg);
>> +#endif
>> +
>> static void tce_iommu_detach_group(void *iommu_data,
>> struct iommu_group *iommu_group);
>>
>> @@ -283,6 +288,10 @@ static long tce_iommu_ioctl(void *iommu_data,
>> tce_iommu_disable(container);
>> mutex_unlock(&container->lock);
>> return 0;
>> +#ifdef CONFIG_VFIO_EEH
>
>I'm not a fan of all these #ifdefs, hide it in eeh_vfio_ioctl() and
>eeh_vfio_release() if needed.
>
Ok. Will do it in next revision.
>> + case VFIO_EEH_INFO:
>> + return eeh_vfio_ioctl(arg);
>> +#endif
>> }
>>
>> return -ENOTTY;
>> @@ -342,6 +351,9 @@ static void tce_iommu_detach_group(void *iommu_data,
>> /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
>> iommu_group_id(iommu_group), iommu_group); */
>> container->tbl = NULL;
>> +#ifdef CONFIG_VFIO_EEH
>> + eeh_vfio_release(tbl);
>> +#endif
>> iommu_release_ownership(tbl);
>> }
>> mutex_unlock(&container->lock);
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index cb9023d..1fd1bfb 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
>>
>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>
>> +/*
>> + * The VFIO EEH info struct provides way to support EEH functionality
>> + * for PCI device that is passed from host to guest via VFIO.
>> + */
>> +#define VFIO_EEH_OP_MAP 0
>> +#define VFIO_EEH_OP_UNMAP 1
>> +#define VFIO_EEH_OP_SET_OPTION 2
>> +#define VFIO_EEH_OP_GET_ADDR 3
>> +#define VFIO_EEH_OP_GET_STATE 4
>> +#define VFIO_EEH_OP_PE_RESET 5
>> +#define VFIO_EEH_OP_PE_CONFIG 6
>
>Is this really an "info" ioctl?
>
Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
>> +
>> +struct vfio_eeh_info {
>> + __u32 argsz;
>> + __u32 op;
>> +
>> + union {
>> + struct vfio_eeh_map {
>> + __u32 host_domain;
>> + __u16 host_cfg_addr;
>> + __u64 guest_buid;
>> + __u16 guest_cfg_addr;
>> + } map;
>> + struct vfio_eeh_unmap {
>> + __u64 buid;
>> + __u16 cfg_addr;
>> + } unmap;
>> + struct vfio_eeh_set_option {
>> + __u64 buid;
>> + __u32 addr;
>> + __u32 option;
>> + } option;
>> + struct vfio_eeh_pe_addr {
>> + __u64 buid;
>> + __u32 cfg_addr;
>> + __u32 option;
>> + __u32 ret;
>> + } addr;
>> + struct vfio_eeh_state {
>> + __u64 buid;
>> + __u32 pe_addr;
>> + __u32 state;
>> + } state;
>> + struct vfio_eeh_reset {
>> + __u64 buid;
>> + __u32 pe_addr;
>> + __u32 option;
>> + } reset;
>> + struct vfio_eeh_config {
>> + __u64 buid;
>> + __u32 pe_addr;
>> + } config;
>> + };
>> +};
>> +
>> +#define VFIO_EEH_INFO _IO(VFIO_TYPE, VFIO_BASE + 21)
>> +
>> /* ***************************************************************** */
>>
>> #endif /* _UAPIVFIO_H */
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH] selftests: PAGE_SIZE may not be defined
From: Benjamin Herrenschmidt @ 2014-05-20 0:29 UTC (permalink / raw)
To: Thierry Fauck ( thierry @ linux.vnet.ibm.com ); +Cc: linuxppc-dev, benh
In-Reply-To: <5374E532.10002@linux.vnet.ibm.com>
On Thu, 2014-05-15 at 18:02 +0200, Thierry Fauck ( thierry @
linux.vnet.ibm.com ) wrote:
> Hi Ben,
>
> The following patch seems relevant for Ubuntu and Debian distro
> testing.
> I know it's minor but I need to know if you do you care about it or
> sif I should remove it from the bugs info.
You should send it to whoever maintains that file...
Cheers,
Ben.
> Thanks
>
> Best regards
> Thierry
>
> #---------------------------------------------------------------------------------------------------
>
> From 48a9a9834377a74b603be12dcc76cda24105e33c Mon Sep 17 00:00:00 2001
>
>
> From: Thierry Fauck <thierry@linux.vnet.ibm.com>
> Date: Fri, 28 Feb 2014 16:17:50 +0100
> Subject: [PATCH] selftests: PAGE_SIZE may not be defined
>
> Some systems have a dynamic PAGE_SIZE value and do not add a
> definition
> for PAGE_SIZE. This value will have to be retrieved using
> getpagesize()
> or sysconf().
>
> Signed-off-by: Thierry Fauck <thierry@linux.vnet.ibm.com>
> ---
> tools/testing/selftests/ptrace/peeksiginfo.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/ptrace/peeksiginfo.c
> b/tools/testing/selftests/ptrace/peeksiginfo.c
> index d46558b..f2ccbbd 100644
> --- a/tools/testing/selftests/ptrace/peeksiginfo.c
> +++ b/tools/testing/selftests/ptrace/peeksiginfo.c
> @@ -35,7 +35,9 @@ static int sys_ptrace(int request, pid_t pid, void
> *addr, void *data)
> fprintf(stderr, \
> "Error (%s:%d): " fmt, \
> __FILE__, __LINE__, ##__VA_ARGS__)
> -
> +#ifndef PAGE_SIZE
> +#define PAGE_SIZE sysconf(_SC_PAGESIZE)
> +#endif
> static int check_error_paths(pid_t child)
> {
> struct ptrace_peeksiginfo_args arg;
> --
> 1.9.0
>
> --
> Thierry Fauck (thierry@linux.vnet.ibm.com)
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH 3/8] drivers/vfio: New IOCTL command VFIO_EEH_INFO
From: Alex Williamson @ 2014-05-20 0:37 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, agraf, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <20140520002208.GA11073@shangw>
On Tue, 2014-05-20 at 10:22 +1000, Gavin Shan wrote:
> On Mon, May 19, 2014 at 04:33:10PM -0600, Alex Williamson wrote:
> >On Wed, 2014-05-14 at 14:11 +1000, Gavin Shan wrote:
> >> The patch adds new IOCTL command VFIO_EEH_INFO to VFIO container
> >> to support EEH functionality for PCI devices, which have been
> >> passed from host to guest via VFIO.
>
> Thanks for your comments, Alex.W :-)
>
> >
> >Some comments throughout, but overall this seems to forgo every bit of
> >the device ownership and protection model used by VFIO and lets the user
> >pick arbitrary host devices and do various operations, mostly unchecked.
> >That's not acceptable.
> >
>
> As what I replied to patch[2], I'm going to let VFIO-PCI-dev fd handle
> the newly introduced IOCTL command. That way, we should follow the VFIO
> design principles (ownership and protection) because VFIO-PCI-dev fd
> is owned by QEMU process usually.
>
> Also, the address mapping maintained in EEH will be removed.
>
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >> arch/powerpc/platforms/powernv/Makefile | 1 +
> >> arch/powerpc/platforms/powernv/eeh-vfio.c | 593 ++++++++++++++++++++++++++++++
> >> drivers/vfio/vfio_iommu_spapr_tce.c | 12 +
> >> include/uapi/linux/vfio.h | 57 +++
> >> 4 files changed, 663 insertions(+)
> >> create mode 100644 arch/powerpc/platforms/powernv/eeh-vfio.c
> >>
> >> diff --git a/arch/powerpc/platforms/powernv/Makefile b/arch/powerpc/platforms/powernv/Makefile
> >> index 63cebb9..2b15a03 100644
> >> --- a/arch/powerpc/platforms/powernv/Makefile
> >> +++ b/arch/powerpc/platforms/powernv/Makefile
> >> @@ -6,5 +6,6 @@ obj-y += opal-msglog.o
> >> obj-$(CONFIG_SMP) += smp.o
> >> obj-$(CONFIG_PCI) += pci.o pci-p5ioc2.o pci-ioda.o
> >> obj-$(CONFIG_EEH) += eeh-ioda.o eeh-powernv.o
> >> +obj-$(CONFIG_VFIO_EEH) += eeh-vfio.o
> >> obj-$(CONFIG_PPC_SCOM) += opal-xscom.o
> >> obj-$(CONFIG_MEMORY_FAILURE) += opal-memory-errors.o
> >> diff --git a/arch/powerpc/platforms/powernv/eeh-vfio.c b/arch/powerpc/platforms/powernv/eeh-vfio.c
> >> new file mode 100644
> >> index 0000000..69d5f2d
> >> --- /dev/null
> >> +++ b/arch/powerpc/platforms/powernv/eeh-vfio.c
> >> @@ -0,0 +1,593 @@
> >> +/*
> >> + * The file intends to support EEH funtionality for those PCI devices,
> >> + * which have been passed through from host to guest via VFIO. So this
> >> + * file is naturally part of VFIO implementation on PowerNV platform.
> >> + *
> >> + * Copyright Benjamin Herrenschmidt & Gavin Shan, IBM Corporation 2014.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + */
> >> +
> >> +#include <linux/init.h>
> >> +#include <linux/io.h>
> >> +#include <linux/irq.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/kvm_host.h>
> >> +#include <linux/msi.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/string.h>
> >> +#include <linux/vfio.h>
> >> +
> >> +#include <asm/eeh.h>
> >> +#include <asm/eeh_event.h>
> >> +#include <asm/io.h>
> >> +#include <asm/iommu.h>
> >> +#include <asm/opal.h>
> >> +#include <asm/msi_bitmap.h>
> >> +#include <asm/pci-bridge.h>
> >> +#include <asm/ppc-pci.h>
> >> +#include <asm/tce.h>
> >> +#include <asm/uaccess.h>
> >> +
> >> +#include "powernv.h"
> >> +#include "pci.h"
> >> +
> >> +static int powernv_eeh_vfio_map(struct vfio_eeh_info *info)
> >> +{
> >> + struct pci_bus *bus, *pe_bus;
> >> + struct pci_dev *pdev;
> >> + struct eeh_dev *edev;
> >> + struct eeh_pe *pe;
> >> + int domain, bus_no, devfn;
> >> +
> >> + /* Host address */
> >> + domain = info->map.host_domain;
> >> + bus_no = (info->map.host_cfg_addr >> 8) & 0xff;
> >> + devfn = info->map.host_cfg_addr & 0xff;
> >
> >Where are we validating that the user has any legitimate claim to be
> >touching this device?
> >
>
> I'll let VFIO-PCI-dev fd handle the IOCTL command. With that, we shouldn't
> have the problem.
>
> >> + /* Find PCI bus */
> >> + bus = pci_find_bus(domain, bus_no);
> >> + if (!bus) {
> >> + pr_warn("%s: PCI bus %04x:%02x not found\n",
> >> + __func__, domain, bus_no);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + /* Find PCI device */
> >> + pdev = pci_get_slot(bus, devfn);
> >> + if (!pdev) {
> >> + pr_warn("%s: PCI device %04x:%02x:%02x.%01x not found\n",
> >> + __func__, domain, bus_no,
> >> + PCI_SLOT(devfn), PCI_FUNC(devfn));
> >> + return -ENODEV;
> >> + }
> >> +
> >> + /* No EEH device - almost impossible */
> >> + edev = pci_dev_to_eeh_dev(pdev);
> >> + if (unlikely(!edev)) {
> >> + pci_dev_put(pdev);
> >> + pr_warn("%s: No EEH dev for PCI device %s\n",
> >> + __func__, pci_name(pdev));
> >> + return -ENODEV;
> >> + }
> >> +
> >> + /* Doesn't support PE migration between different PHBs */
> >> + pe = edev->pe;
> >> + if (!eeh_pe_passed(pe)) {
> >> + pe_bus = eeh_pe_bus_get(pe);
> >> + BUG_ON(!pe_bus);
> >
> >Can a user trigger this maliciously?
> >
> >> +
> >> + /* PE# has format 00BBSS00 */
> >> + pe->guest_addr.buid = info->map.guest_buid;
> >> + pe->guest_addr.pe_addr = pe_bus->number << 16;
> >> + eeh_pe_set_passed(pe, true);
> >> + } else if (pe->guest_addr.buid != info->map.guest_buid) {
> >> + pci_dev_put(pdev);
> >> + pr_warn("%s: Mismatched PHB BUID (0x%llx, 0x%llx)\n",
> >> + __func__, pe->guest_addr.buid, info->map.guest_buid);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + edev->guest_addr.buid = info->map.guest_buid;
> >> + edev->guest_addr.config_addr = info->map.guest_cfg_addr;
> >> + eeh_dev_set_passed(edev, true);
> >> +
> >> + pr_debug("EEH: Host PCI dev %s to %llx-%02x:%02x.%01x\n",
> >> + pci_name(pdev), info->map.guest_buid,
> >> + (info->map.guest_cfg_addr >> 8) & 0xFF,
> >> + PCI_SLOT(info->map.guest_cfg_addr & 0xFF),
> >> + PCI_FUNC(info->map.guest_cfg_addr & 0xFF));
> >> +
> >> + pci_dev_put(pdev);
> >> + return 0;
> >> +}
> >
> >So the effect of this function is that a user gets to setup an arbitrary
> >guest mapping for an arbitrary host device and associated pe. Is that
> >right? It seems bad.
> >
>
> I'm going to remove this mapping in next revision.
>
> >> +
> >> +static int powernv_eeh_vfio_unmap(struct vfio_eeh_info *info)
> >> +{
> >> + struct eeh_vfio_pci_addr addr;
> >> + struct pci_dev *pdev;
> >> + struct eeh_dev *edev, *tmp;
> >> + struct eeh_pe *pe;
> >> + bool passed;
> >> +
> >> + /* Get EEH device */
> >> + addr.buid = info->unmap.buid;
> >> + addr.config_addr = info->unmap.cfg_addr;
> >> + edev = eeh_vfio_dev_get(&addr);
> >
> >eeh_vfio_dev_get() just looks for a "passed" dev and a match for a well
> >known address space. Seems very exploitable.
> >
> >> + if (!edev) {
> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> >> + __func__, info->unmap.buid,
> >> + (info->unmap.cfg_addr >> 8) & 0xFF,
> >> + PCI_SLOT(info->unmap.cfg_addr & 0xFF),
> >> + PCI_FUNC(info->unmap.cfg_addr & 0xFF));
> >> + return -ENODEV;
> >> + }
> >> +
> >> + /* Return EEH device */
> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
> >> + eeh_dev_set_passed(edev, false);
> >> + pdev = eeh_dev_to_pci_dev(edev);
> >> + pr_debug("EEH: Host PCI dev %s returned\n",
> >> + pdev ? pci_name(pdev) : "NULL");
> >> +
> >> + /* Return PE if no EEH device is owned by guest */
> >> + pe = edev->pe;
> >> + passed = false;
> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
> >> + pdev = eeh_dev_to_pci_dev(edev);
> >> + if (pdev && pdev->subordinate)
> >> + continue;
> >> +
> >> + if (eeh_dev_passed(edev)) {
> >> + passed = true;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (!passed) {
> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
> >> + eeh_pe_set_passed(pe, false);
> >> + pr_debug("EEH: PHB#%x-PE#%x returned to host\n",
> >> + pe->phb->global_number, pe->addr);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int powernv_eeh_vfio_set_option(struct vfio_eeh_info *info)
> >> +{
> >> + struct pnv_phb *phb;
> >> + struct eeh_dev *edev;
> >> + struct eeh_pe *pe;
> >> + struct eeh_vfio_pci_addr addr;
> >> + int opcode = info->option.option;
> >> + int ret = 0;
> >> +
> >> + /* Check opcode */
> >> + if (opcode < EEH_OPT_DISABLE || opcode > EEH_OPT_THAW_DMA) {
> >> + pr_warn("%s: opcode %d out of range (%d, %d)\n",
> >> + __func__, opcode, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
> >> + ret = 3;
> >
> >Please don't make up arbitrary return values.
> >
>
> Nope, it will be turned to "-3" eventually by QEMU.
Don't assume QEMU is your userspace.
> That means "Invalid Parameter"
> defined in PAPR spec.
Is there value in matching the PAPR spec (which most people can't read)?
If there is...
> The IOCTL command handler return 3 values:
>
> < 0: Linux kernel error. For example, error from copy_from_user().
> > 0: Error code to the EEH RTAS request, which will be returned to guest.
> = 0: Success
Maybe the ioctl return should match normal ioctl return values and the
EEH error code can be stored somewhere in the structure.
> >> + goto out;
> >> + }
> >> +
> >> + /* Option "enable" uses PCI config address */
> >> + if (opcode == EEH_OPT_ENABLE) {
> >> + addr.buid = info->option.buid;
> >> + addr.config_addr = (info->option.addr >> 8) & 0xFFFF;
> >> + edev = eeh_vfio_dev_get(&addr);
> >> + if (!edev) {
> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> >> + __func__, addr.buid,
> >> + (addr.config_addr >> 8) & 0xFF,
> >> + PCI_SLOT(addr.config_addr & 0xFF),
> >> + PCI_FUNC(addr.config_addr & 0xFF));
> >> + ret = 7;
> >> + goto out;
> >> + }
> >> + phb = edev->phb->private_data;
> >> + } else {
> >> + addr.buid = info->option.buid;
> >> + addr.pe_addr = info->option.addr;
> >> + pe = eeh_vfio_pe_get(&addr);
> >> + if (!pe) {
> >> + pr_warn("%s: Cannot find PE %llx:%x\n",
> >> + __func__, addr.buid, addr.pe_addr);
> >> + ret = 7;
> >> + goto out;
> >> + }
> >> + phb = pe->phb->private_data;
> >> + }
> >> +
> >> + /* Insure that the EEH stuff has been initialized */
> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
> >> + __func__, phb->hose->global_number);
> >> + ret = 7;
> >> + goto out;
> >> + }
> >> +
> >> + /*
> >> + * The EEH functionality has been enabled on all PEs
> >> + * by default. So just return success. The same situation
> >> + * would be applied while we disable EEH functionality.
> >> + * However, the guest isn't expected to disable that
> >> + * at all.
> >> + */
> >> + if (opcode == EEH_OPT_DISABLE ||
> >> + opcode == EEH_OPT_ENABLE) {
> >> + ret = 0;
> >> + goto out;
> >> + }
> >> +
> >> + /*
> >> + * Call into the IODA dependent backend in order
> >> + * to enable DMA or MMIO for the indicated PE.
> >> + */
> >> + if (phb->eeh_ops && phb->eeh_ops->set_option) {
> >> + if (phb->eeh_ops->set_option(pe, opcode)) {
> >> + pr_warn("%s: Failure from backend\n",
> >> + __func__);
> >> + ret = 1;
> >> + }
> >> + } else {
> >> + pr_warn("%s: Unsupported request\n",
> >> + __func__);
> >> + ret = 7;
> >> + }
> >> +
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +static int powernv_eeh_vfio_get_addr(struct vfio_eeh_info *info)
> >> +{
> >> + struct pnv_phb *phb;
> >> + struct eeh_dev *edev;
> >> + struct eeh_vfio_pci_addr addr;
> >> + int opcode = info->addr.option;
> >> + int ret = 0;
> >> +
> >> + /* Check opcode */
> >> + if (opcode != 0 && opcode != 1) {
> >> + pr_warn("%s: opcode %d out of range (0, 1)\n",
> >> + __func__, opcode);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /* Find EEH device */
> >> + addr.buid = info->addr.buid;
> >> + addr.config_addr = (info->addr.cfg_addr >> 8 ) & 0xFFFF;
> >> + edev = eeh_vfio_dev_get(&addr);
> >> + if (!edev) {
> >> + pr_warn("%s: Cannot find %llx:%02x:%02x.%01x\n",
> >> + __func__, addr.buid,
> >> + (addr.config_addr >> 8) & 0xFF,
> >> + PCI_SLOT(addr.config_addr & 0xFF),
> >> + PCI_FUNC(addr.config_addr & 0xFF));
> >> + ret = 7;
> >> + goto out;
> >> + }
> >> + phb = edev->phb->private_data;
> >> +
> >> + /* EEH enabled ? */
> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
> >> + __func__, phb->hose->global_number);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /* EEH device passed ? */
> >> + if (!eeh_dev_passed(edev)) {
> >> + pr_warn("%s: EEH dev %llx:%02x:%02x.%01x owned by host\n",
> >> + __func__, addr.buid,
> >> + (addr.config_addr >> 8) & 0xFF,
> >> + PCI_SLOT(addr.config_addr & 0xFF),
> >> + PCI_FUNC(addr.config_addr & 0xFF));
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /*
> >> + * Fill result according to opcode. We don't differentiate
> >> + * PCI bus and device sensitive PE here.
> >> + */
> >> + if (opcode == 0)
> >> + info->addr.ret = edev->pe->guest_addr.pe_addr;
> >> + else
> >> + info->addr.ret = 1;
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +static int powernv_eeh_vfio_get_state(struct vfio_eeh_info *info)
> >> +{
> >> + struct pnv_phb *phb;
> >> + struct eeh_pe *pe;
> >> + struct eeh_vfio_pci_addr addr;
> >> + int result, ret = 0;
> >> +
> >> + /* Locate the PE */
> >> + addr.buid = info->state.buid;
> >> + addr.pe_addr = info->state.pe_addr;
> >> + pe = eeh_vfio_pe_get(&addr);
> >> + if (!pe) {
> >> + pr_warn("%s: Cannot locate %llx:%x\n",
> >> + __func__, addr.buid, addr.pe_addr);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> + phb = pe->phb->private_data;
> >> +
> >> + /* EEH enabled ? */
> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
> >> + __func__, phb->hose->global_number);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /* Call to the IOC dependent function */
> >> + if (phb->eeh_ops && phb->eeh_ops->get_state) {
> >> + result = phb->eeh_ops->get_state(pe);
> >> +
> >> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
> >> + (result & EEH_STATE_DMA_ENABLED) &&
> >> + (result & EEH_STATE_MMIO_ENABLED))
> >> + info->state.state = 0;
> >> + else if (result & EEH_STATE_RESET_ACTIVE)
> >> + info->state.state = 1;
> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> >> + !(result & EEH_STATE_DMA_ENABLED) &&
> >> + !(result & EEH_STATE_MMIO_ENABLED))
> >> + info->state.state = 2;
> >> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> >> + (result & EEH_STATE_DMA_ENABLED) &&
> >> + !(result & EEH_STATE_MMIO_ENABLED))
> >> + info->state.state = 4;
> >> + else
> >> + info->state.state = 5;
> >> +
> >> + ret = 0;
> >> + } else {
> >> + pr_warn("%s: Unsupported request\n", __func__);
> >> + ret = 3;
> >> + }
> >> +
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +static int powernv_eeh_vfio_pe_reset(struct vfio_eeh_info *info)
> >> +{
> >> + struct pnv_phb *phb;
> >> + struct eeh_pe *pe;
> >> + struct eeh_vfio_pci_addr addr;
> >> + int opcode = info->reset.option;
> >> + int ret = 0;
> >> +
> >> + /* Check opcode */
> >> + if (opcode != EEH_RESET_DEACTIVATE &&
> >> + opcode != EEH_RESET_HOT &&
> >> + opcode != EEH_RESET_FUNDAMENTAL) {
> >> + pr_warn("%s: Unsupported opcode %d\n",
> >> + __func__, opcode);
> >
> >Console warnings are exploitable DoS attacks.
> >
>
> Yep. I'll change all pr_warn() to pr_debug() in next revision.
>
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /* Locate the PE */
> >> + addr.buid = info->reset.buid;
> >> + addr.pe_addr = info->reset.pe_addr;
> >> + pe = eeh_vfio_pe_get(&addr);
> >> + if (!pe) {
> >> + pr_warn("%s: Cannot locate %llx:%x\n",
> >> + __func__, addr.buid, addr.pe_addr);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> + phb = pe->phb->private_data;
> >> +
> >> + /* EEH enabled ? */
> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
> >> + __func__, phb->hose->global_number);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /* Call into the IODA dependent backend to do the reset */
> >> + if (!phb->eeh_ops ||
> >> + !phb->eeh_ops->set_option ||
> >> + !phb->eeh_ops->reset) {
> >> + pr_warn("%s: Unsupported request\n",
> >> + __func__);
> >> + ret = 7;
> >> + } else {
> >> + /*
> >> + * The frozen PE might be caused by the mechanism called
> >> + * PAPR error injection, which is supposed to be one-shot
> >> + * without "sticky" bit as being stated by the spec. But
> >> + * the reality isn't that, at least on P7IOC. So we have
> >> + * to clear that to avoid recrusive error, which fails the
> >> + * recovery eventually.
> >> + */
> >> + if (opcode == EEH_RESET_DEACTIVATE)
> >> + opal_pci_reset(phb->opal_id,
> >> + OPAL_PHB_ERROR,
> >> + OPAL_ASSERT_RESET);
> >> +
> >> + if (phb->eeh_ops->reset(pe, opcode)) {
> >> + pr_warn("%s: Failure from backend\n", __func__);
> >> + ret = 1;
> >> + goto out;
> >> + }
> >> +
> >> + /*
> >> + * The PE is still in frozen state and we need clear that.
> >> + * It's good to clear frozen state after deassert to avoid
> >> + * messy IO access during reset, which might cause recrusive
> >> + * frozen PE.
> >> + */
> >> + if (opcode == EEH_RESET_DEACTIVATE) {
> >> + if (phb->eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO) ||
> >> + phb->eeh_ops->set_option(pe, EEH_OPT_THAW_DMA)) {
> >> + pr_warn("%s: Cannot clear frozen state\n",
> >> + __func__);
> >> + ret = 1;
> >> + }
> >> +
> >> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
> >> + }
> >> + }
> >> +
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +static int powernv_eeh_vfio_pe_config(struct vfio_eeh_info *info)
> >> +{
> >> + struct pnv_phb *phb;
> >> + struct eeh_pe *pe;
> >> + struct eeh_vfio_pci_addr addr;
> >> + int ret = 0;
> >> +
> >> + /* Locate the PE */
> >> + addr.buid = info->config.buid;
> >> + addr.pe_addr = info->config.pe_addr;
> >> + pe = eeh_vfio_pe_get(&addr);
> >> + if (!pe) {
> >> + pr_warn("%s: Cannot locate %llx:%x\n",
> >> + __func__, addr.buid, addr.pe_addr);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> + phb = pe->phb->private_data;
> >> +
> >> + /* EEH enabled ? */
> >> + if (!(phb->flags & PNV_PHB_FLAG_EEH)) {
> >> + pr_warn("%s: EEH disabled on PHB#%d\n",
> >> + __func__, phb->hose->global_number);
> >> + ret = 3;
> >> + goto out;
> >> + }
> >> +
> >> + /*
> >> + * The access to PCI config space on VFIO device has some
> >> + * limitations. Part of PCI config space, including BAR
> >> + * registers are not readable and writable. So the guest
> >> + * should have stale values for those registers and we have
> >> + * to restore them in host side.
> >> + */
> >> + eeh_pe_restore_bars(pe);
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +void eeh_vfio_release(struct iommu_table *tbl)
> >> +{
> >> + struct pnv_ioda_pe *pnv_pe = container_of(tbl, struct pnv_ioda_pe,
> >> + tce32_table);
> >> + struct pnv_phb *phb = pnv_pe->phb;
> >> + struct eeh_pe *phb_pe, *pe;
> >> + struct eeh_dev dev, *edev, *tmp;
> >> +
> >> + /* Find PHB PE */
> >> + phb_pe = eeh_phb_pe_get(phb->hose);
> >> + if (unlikely(!phb_pe)) {
> >> + pr_warn("%s: Cannot find PHB#%d PE\n",
> >> + __func__, phb->hose->global_number);
> >> + return;
> >> + }
> >> +
> >> + /* Find PE */
> >> + memset(&dev, 0, sizeof(struct eeh_dev));
> >> + dev.phb = phb->hose;
> >> + dev.pe_config_addr = pnv_pe->pe_number;
> >> + pe = eeh_pe_get(&dev);
> >> + if (unlikely(!pe)) {
> >> + pr_warn("%s: Cannot find PE instance for PHB#%d-PE#%d\n",
> >> + __func__, phb->hose->global_number,
> >> + pnv_pe->pe_number);
> >> + return;
> >> + }
> >> +
> >> + /* Release it to host */
> >> + if (!eeh_pe_passed(pe))
> >> + return;
> >> +
> >> + eeh_pe_for_each_dev(pe, edev, tmp) {
> >> + if (!eeh_dev_passed(edev))
> >> + continue;
> >> +
> >> + memset(&edev->guest_addr, 0, sizeof(edev->guest_addr));
> >
> >Is guest_addr = { 0 } not valid? As agraf already mentioned, there are
> >a number of issues with using a guest_address for a token.
> >
>
> For now, PHB BUID can't be "0". Originally, I was planing to have some code
> in QEMU to have unique PHB BUID across the system so that guest_address could
> be the unique token. But I'm going to remove the address mapping in next revision
> as Alex.G suggested.
>
> >> + eeh_dev_set_passed(edev, false);
> >> + }
> >> +
> >> + memset(&pe->guest_addr, 0, sizeof(pe->guest_addr));
> >> + eeh_pe_set_passed(pe, false);
> >> +}
> >> +EXPORT_SYMBOL(eeh_vfio_release);
> >> +
> >> +int eeh_vfio_ioctl(unsigned long arg)
> >> +{
> >> + struct vfio_eeh_info info;
> >> + int ret = -EINVAL;
> >> +
> >> + /* Copy over user argument */
> >> + if (copy_from_user(&info, (void __user *)arg, sizeof(info))) {
> >> + pr_warn("%s: Cannot copy user argument 0x%lx\n",
> >> + __func__, arg);
> >> + return -EFAULT;
> >> + }
> >> +
> >> + /* Sanity check */
> >> + if (info.argsz != sizeof(info)) {
> >
> >This breaks compatibility if you need to later add a new ops with a
> >larger footprint.
> >
>
> Ok. I'll fix it in next revision. Thanks for pointing it out.
>
> >> + pr_warn("%s: Invalid argument size (%d, %ld)\n",
> >> + __func__, info.argsz, sizeof(info));
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Route according to operation */
> >> + switch (info.op) {
> >> + case VFIO_EEH_OP_MAP:
> >> + ret = powernv_eeh_vfio_map(&info);
> >> + break;
> >> + case VFIO_EEH_OP_UNMAP:
> >> + ret = powernv_eeh_vfio_unmap(&info);
> >> + break;
> >> + case VFIO_EEH_OP_SET_OPTION:
> >> + ret = powernv_eeh_vfio_set_option(&info);
> >> + break;
> >> + case VFIO_EEH_OP_GET_ADDR:
> >> + ret = powernv_eeh_vfio_get_addr(&info);
> >> + break;
> >> + case VFIO_EEH_OP_GET_STATE:
> >> + ret = powernv_eeh_vfio_get_state(&info);
> >> + break;
> >> + case VFIO_EEH_OP_PE_RESET:
> >> + ret = powernv_eeh_vfio_pe_reset(&info);
> >> + break;
> >> + case VFIO_EEH_OP_PE_CONFIG:
> >> + ret = powernv_eeh_vfio_pe_config(&info);
> >> + break;
> >> + default:
> >> + pr_info("%s: Cannot handle op#%d\n",
> >> + __func__, info.op);
> >> + }
> >> +
> >> + /* Copy data back */
> >> + if (!ret && copy_to_user((void __user *)arg, &info, sizeof(info))) {
> >> + pr_warn("%s: Cannot copy to user 0x%lx\n",
> >> + __func__, arg);
> >> + return -EFAULT;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(eeh_vfio_ioctl);
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> index a84788b..c45dece 100644
> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -26,6 +26,11 @@
> >> #define DRIVER_AUTHOR "aik@ozlabs.ru"
> >> #define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >>
> >> +#ifdef CONFIG_VFIO_EEH
> >> +extern void eeh_vfio_release(struct iommu_table *tbl);
> >> +extern int eeh_vfio_ioctl(unsigned long arg);
> >> +#endif
> >> +
> >> static void tce_iommu_detach_group(void *iommu_data,
> >> struct iommu_group *iommu_group);
> >>
> >> @@ -283,6 +288,10 @@ static long tce_iommu_ioctl(void *iommu_data,
> >> tce_iommu_disable(container);
> >> mutex_unlock(&container->lock);
> >> return 0;
> >> +#ifdef CONFIG_VFIO_EEH
> >
> >I'm not a fan of all these #ifdefs, hide it in eeh_vfio_ioctl() and
> >eeh_vfio_release() if needed.
> >
>
> Ok. Will do it in next revision.
>
> >> + case VFIO_EEH_INFO:
> >> + return eeh_vfio_ioctl(arg);
> >> +#endif
> >> }
> >>
> >> return -ENOTTY;
> >> @@ -342,6 +351,9 @@ static void tce_iommu_detach_group(void *iommu_data,
> >> /* pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
> >> iommu_group_id(iommu_group), iommu_group); */
> >> container->tbl = NULL;
> >> +#ifdef CONFIG_VFIO_EEH
> >> + eeh_vfio_release(tbl);
> >> +#endif
> >> iommu_release_ownership(tbl);
> >> }
> >> mutex_unlock(&container->lock);
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index cb9023d..1fd1bfb 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -455,6 +455,63 @@ struct vfio_iommu_spapr_tce_info {
> >>
> >> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
> >>
> >> +/*
> >> + * The VFIO EEH info struct provides way to support EEH functionality
> >> + * for PCI device that is passed from host to guest via VFIO.
> >> + */
> >> +#define VFIO_EEH_OP_MAP 0
> >> +#define VFIO_EEH_OP_UNMAP 1
> >> +#define VFIO_EEH_OP_SET_OPTION 2
> >> +#define VFIO_EEH_OP_GET_ADDR 3
> >> +#define VFIO_EEH_OP_GET_STATE 4
> >> +#define VFIO_EEH_OP_PE_RESET 5
> >> +#define VFIO_EEH_OP_PE_CONFIG 6
> >
> >Is this really an "info" ioctl?
> >
>
> Yeah, "VFIO_EEH_INFO" isn't a good name. How about to have "VFIO_EEH_HANDLER" ?
VFIO_EEH_OP perhaps. Thanks,
Alex
> >> +
> >> +struct vfio_eeh_info {
> >> + __u32 argsz;
> >> + __u32 op;
> >> +
> >> + union {
> >> + struct vfio_eeh_map {
> >> + __u32 host_domain;
> >> + __u16 host_cfg_addr;
> >> + __u64 guest_buid;
> >> + __u16 guest_cfg_addr;
> >> + } map;
> >> + struct vfio_eeh_unmap {
> >> + __u64 buid;
> >> + __u16 cfg_addr;
> >> + } unmap;
> >> + struct vfio_eeh_set_option {
> >> + __u64 buid;
> >> + __u32 addr;
> >> + __u32 option;
> >> + } option;
> >> + struct vfio_eeh_pe_addr {
> >> + __u64 buid;
> >> + __u32 cfg_addr;
> >> + __u32 option;
> >> + __u32 ret;
> >> + } addr;
> >> + struct vfio_eeh_state {
> >> + __u64 buid;
> >> + __u32 pe_addr;
> >> + __u32 state;
> >> + } state;
> >> + struct vfio_eeh_reset {
> >> + __u64 buid;
> >> + __u32 pe_addr;
> >> + __u32 option;
> >> + } reset;
> >> + struct vfio_eeh_config {
> >> + __u64 buid;
> >> + __u32 pe_addr;
> >> + } config;
> >> + };
> >> +};
> >> +
> >> +#define VFIO_EEH_INFO _IO(VFIO_TYPE, VFIO_BASE + 21)
> >> +
> >> /* ***************************************************************** */
> >>
> >> #endif /* _UAPIVFIO_H */
>
> Thanks,
> Gavin
>
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Kirill A. Shutemov @ 2014-05-20 0:44 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-arch, x86, riel, Madhavan Srinivasan, dave.hansen, peterz,
Rusty Russell, Hugh Dickins, linux-kernel, linux-mm, ak, paulus,
mgorman, linuxppc-dev, mingo, Kirill A. Shutemov
In-Reply-To: <20140519164301.eafd3dd288ccb88361ddcfc7@linux-foundation.org>
Andrew Morton wrote:
> On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:
>
> > Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> > the order of the fault-around size in bytes, and fault_around_pages()
> > use 1UL << (fault_around_order - PAGE_SHIFT)
>
> Yes. And shame on me for missing it (this time!) at review.
>
> There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
I'll sign it off tomorrow after testing.
diff --git a/mm/memory.c b/mm/memory.c
index 037b812a9531..9d6941c9a9e4 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3402,62 +3402,62 @@ void do_set_pte(struct vm_area_struct *vma, unsigned long address,
update_mmu_cache(vma, address, pte);
}
-#define FAULT_AROUND_ORDER 4
+#define FAULT_AROUND_BYTES 65536
#ifdef CONFIG_DEBUG_FS
-static unsigned int fault_around_order = FAULT_AROUND_ORDER;
+static unsigned int fault_around_bytes = FAULT_AROUND_BYTES;
-static int fault_around_order_get(void *data, u64 *val)
+static int fault_around_bytes_get(void *data, u64 *val)
{
- *val = fault_around_order;
+ *val = fault_around_bytes;
return 0;
}
-static int fault_around_order_set(void *data, u64 val)
+static int fault_around_bytes_set(void *data, u64 val)
{
- BUILD_BUG_ON((1UL << FAULT_AROUND_ORDER) > PTRS_PER_PTE);
- if (1UL << val > PTRS_PER_PTE)
+ BUILD_BUG_ON(FAULT_AROUND_BYTES / PAGE_SIZE > PTRS_PER_PTE);
+ if (val / PAGE_SIZE > PTRS_PER_PTE)
return -EINVAL;
- fault_around_order = val;
+ fault_around_bytes = val;
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fault_around_order_fops,
- fault_around_order_get, fault_around_order_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fault_around_bytes_fops,
+ fault_around_bytes_get, fault_around_bytes_set, "%llu\n");
static int __init fault_around_debugfs(void)
{
void *ret;
- ret = debugfs_create_file("fault_around_order", 0644, NULL, NULL,
- &fault_around_order_fops);
+ ret = debugfs_create_file("fault_around_bytes", 0644, NULL, NULL,
+ &fault_around_bytes_fops);
if (!ret)
- pr_warn("Failed to create fault_around_order in debugfs");
+ pr_warn("Failed to create fault_around_bytes in debugfs");
return 0;
}
late_initcall(fault_around_debugfs);
static inline unsigned long fault_around_pages(void)
{
- return 1UL << fault_around_order;
+ return fault_around_bytes / PAGE_SIZE;
}
static inline unsigned long fault_around_mask(void)
{
- return ~((1UL << (PAGE_SHIFT + fault_around_order)) - 1);
+ return ~(round_down(fault_around_bytes, PAGE_SIZE) - 1);
}
#else
static inline unsigned long fault_around_pages(void)
{
unsigned long nr_pages;
- nr_pages = 1UL << FAULT_AROUND_ORDER;
+ nr_pages = FAULT_AROUND_BYTES / PAGE_SIZE;
BUILD_BUG_ON(nr_pages > PTRS_PER_PTE);
return nr_pages;
}
static inline unsigned long fault_around_mask(void)
{
- return ~((1UL << (PAGE_SHIFT + FAULT_AROUND_ORDER)) - 1);
+ return ~(round_down(FAULT_AROUND_BYTES, PAGE_SIZE) - 1);
}
#endif
@@ -3515,7 +3515,7 @@ static int do_read_fault(struct mm_struct *mm, struct vm_area_struct *vma,
* if page by the offset is not ready to be mapped (cold cache or
* something).
*/
- if (vma->vm_ops->map_pages) {
+ if (vma->vm_ops->map_pages && fault_around_pages() > 1) {
pte = pte_offset_map_lock(mm, pmd, address, &ptl);
do_fault_around(vma, address, pte, pgoff, flags);
if (!pte_same(*pte, orig_pte))
--
Kirill A. Shutemov
^ permalink raw reply related
* [PATCH 0/9 net-next] net: of_phy_connect_fixed_link removal
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
Hi all,
This patch set removes of_phy_connect_fixed_link() from the tree now that
we have a better solution for dealing with fixed PHY (emulated PHY) devices
for drivers that require them.
First two patches update the 'fixed-link' Device Tree binding and drivers to
refere to it.
Patches 3 to 7 update the in-tree network drivers that use
of_phy_connect_fixed_link()
Patch 8 removes of_phy_connect_fixed_link
Patch 9 removes the PowerPC code that parsed the 'fixed-link' property.
Patch 9 can be merged via the net-next tree if the PowerPC folks ack it,
but it really has to be merged after the first 8 patches in order to avoid
breakage.
Florian Fainelli (9):
Documentation: devicetree: add old and deprecated 'fixed-link'
Documentation: devicetree: net: refer to fixed-link.txt
net: bcmgenet: use the new fixed PHY helpers
net: systemport: use the new fixed PHY helpers
fs_enet: use the new fixed PHY helpers
gianfar: use the new fixed PHY helpers
ucc_geth: use the new fixed PHY helpers
of: mdio: remove of_phy_connect_fixed_link
powerpc/fsl: fsl_soc: remove 'fixed-link' parsing code
.../devicetree/bindings/net/broadcom-bcmgenet.txt | 2 +-
.../bindings/net/broadcom-systemport.txt | 2 +-
.../devicetree/bindings/net/fixed-link.txt | 12 +++++++
.../devicetree/bindings/net/fsl-tsec-phy.txt | 5 +--
arch/powerpc/sysdev/fsl_soc.c | 32 ------------------
drivers/net/ethernet/broadcom/bcmsysport.c | 17 ++++++++--
drivers/net/ethernet/broadcom/bcmsysport.h | 1 +
drivers/net/ethernet/broadcom/genet/bcmmii.c | 21 +++++++-----
.../net/ethernet/freescale/fs_enet/fs_enet-main.c | 16 +++++----
drivers/net/ethernet/freescale/gianfar.c | 14 ++++++--
drivers/net/ethernet/freescale/ucc_geth.c | 14 ++++++--
drivers/of/of_mdio.c | 38 ----------------------
include/linux/of_mdio.h | 10 ------
13 files changed, 75 insertions(+), 109 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH net-next 1/9] Documentation: devicetree: add old and deprecated 'fixed-link'
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
Update the fixed-link Device Tree binding documentation to contain
information about the old and deprecated 5-digit 'fixed-link' property.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Documentation/devicetree/bindings/net/fixed-link.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
index e956de1be935..34a991eb213f 100644
--- a/Documentation/devicetree/bindings/net/fixed-link.txt
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -18,6 +18,18 @@ properties:
* 'asym-pause' (boolean, optional), to indicate that asym_pause should
be enabled.
+Old, deprecated 'fixed-link' binding:
+
+* A 'fixed-link' property in the Ethernet MAC node, with 5 cells, of the
+ form <a b c d e> with the following accepted values:
+ - a: emulated phy id, choose any but but unique to the all specified
+ fixed-links, from 0 to 31
+ - b: duplex configuration: 0 for half duplex, 1 for full duplex
+ - c: link speed in Mbits/sec, accepted values are: 10, 100 and 1000
+ - d: pause configuration: 0 for no pause, 1 for pause
+ - e: asymetric pause configuration: 0 for no asymetric pause, 1 for asymetric
+ pause
+
Example:
ethernet@0 {
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 2/9] Documentation: devicetree: net: refer to fixed-link.txt
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
Update the Freescale TSEC PHY, Broadcom GENET & SYSTEMPORT Device Tree
binding documentation to refer to the fixed-link Device Tree binding in
fixed-link.txt.
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt | 2 +-
Documentation/devicetree/bindings/net/broadcom-systemport.txt | 2 +-
Documentation/devicetree/bindings/net/fsl-tsec-phy.txt | 5 +----
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt b/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
index f2febb94550e..451fef26b4df 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
@@ -24,7 +24,7 @@ Optional properties:
- fixed-link: When the GENET interface is connected to a MoCA hardware block or
when operating in a RGMII to RGMII type of connection, or when the MDIO bus is
voluntarily disabled, this property should be used to describe the "fixed link".
- See Documentation/devicetree/bindings/net/fsl-tsec-phy.txt for information on
+ See Documentation/devicetree/bindings/net/fixed-link.txt for information on
the property specifics
Required child nodes:
diff --git a/Documentation/devicetree/bindings/net/broadcom-systemport.txt b/Documentation/devicetree/bindings/net/broadcom-systemport.txt
index 1b7600e022dd..c183ea90d9bc 100644
--- a/Documentation/devicetree/bindings/net/broadcom-systemport.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-systemport.txt
@@ -8,7 +8,7 @@ Required properties:
- local-mac-address: Ethernet MAC address (48 bits) of this adapter
- phy-mode: Should be a string describing the PHY interface to the
Ethernet switch/PHY, see Documentation/devicetree/bindings/net/ethernet.txt
-- fixed-link: see Documentation/devicetree/bindings/net/fsl-tsec-phy.txt for
+- fixed-link: see Documentation/devicetree/bindings/net/fixed-link.txt for
the property specific details
Optional properties:
diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
index 737cdef4f903..be6ea8960f20 100644
--- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
+++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
@@ -42,10 +42,7 @@ Properties:
interrupt. For TSEC and eTSEC devices, the first interrupt is
transmit, the second is receive, and the third is error.
- phy-handle : See ethernet.txt file in the same directory.
- - fixed-link : <a b c d e> where a is emulated phy id - choose any,
- but unique to the all specified fixed-links, b is duplex - 0 half,
- 1 full, c is link speed - d#10/d#100/d#1000, d is pause - 0 no
- pause, 1 pause, e is asym_pause - 0 no asym_pause, 1 asym_pause.
+ - fixed-link : See fixed-link.txt in the same directory.
- phy-connection-type : See ethernet.txt file in the same directory.
This property is only really needed if the connection is of type
"rgmii-id", as all other connection types are detected by hardware.
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 3/9] net: bcmgenet: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/genet/bcmmii.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 4608673beaff..add8d8596084 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -298,6 +298,7 @@ int bcmgenet_mii_config(struct net_device *dev)
static int bcmgenet_mii_probe(struct net_device *dev)
{
struct bcmgenet_priv *priv = netdev_priv(dev);
+ struct device_node *dn = priv->pdev->dev.of_node;
struct phy_device *phydev;
unsigned int phy_flags;
int ret;
@@ -307,15 +308,19 @@ static int bcmgenet_mii_probe(struct net_device *dev)
return 0;
}
- if (priv->phy_dn)
- phydev = of_phy_connect(dev, priv->phy_dn,
- bcmgenet_mii_setup, 0,
- priv->phy_interface);
- else
- phydev = of_phy_connect_fixed_link(dev,
- bcmgenet_mii_setup,
- priv->phy_interface);
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(dn)) {
+ ret = of_phy_register_fixed_link(dn);
+ if (ret)
+ return ret;
+
+ priv->phy_dn = dn;
+ }
+ phydev = of_phy_connect(dev, priv->phy_dn, bcmgenet_mii_setup, 0,
+ priv->phy_interface);
if (!phydev) {
pr_err("could not attach to PHY\n");
return -ENODEV;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 4/9] net: systemport: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/broadcom/bcmsysport.c | 17 +++++++++++++++--
drivers/net/ethernet/broadcom/bcmsysport.h | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index d40c5b969e9e..dc708a888f80 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -1327,8 +1327,8 @@ static int bcm_sysport_open(struct net_device *dev)
/* Read CRC forward */
priv->crc_fwd = !!(umac_readl(priv, UMAC_CMD) & CMD_CRC_FWD);
- priv->phydev = of_phy_connect_fixed_link(dev, bcm_sysport_adj_link,
- priv->phy_interface);
+ priv->phydev = of_phy_connect(dev, priv->phy_dn, bcm_sysport_adj_link,
+ 0, priv->phy_interface);
if (!priv->phydev) {
netdev_err(dev, "could not attach to PHY\n");
return -ENODEV;
@@ -1551,6 +1551,19 @@ static int bcm_sysport_probe(struct platform_device *pdev)
if (priv->phy_interface < 0)
priv->phy_interface = PHY_INTERFACE_MODE_GMII;
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(dn)) {
+ ret = of_phy_register_fixed_link(dn);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register fixed PHY\n");
+ goto err;
+ }
+
+ priv->phy_dn = dn;
+ }
+
/* Initialize netdevice members */
macaddr = of_get_mac_address(dn);
if (!macaddr || !is_valid_ether_addr(macaddr)) {
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index abdeb62616df..73fd04a94797 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -656,6 +656,7 @@ struct bcm_sysport_priv {
unsigned int rx_c_index;
/* PHY device */
+ struct device_node *phy_dn;
struct phy_device *phydev;
phy_interface_t phy_interface;
int old_pause;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 5/9] fs_enet: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index dc80db41d6b3..d602711e00e9 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -792,10 +792,6 @@ static int fs_init_phy(struct net_device *dev)
phydev = of_phy_connect(dev, fep->fpi->phy_node, &fs_adjust_link, 0,
iface);
if (!phydev) {
- phydev = of_phy_connect_fixed_link(dev, &fs_adjust_link,
- iface);
- }
- if (!phydev) {
dev_err(&dev->dev, "Could not attach to PHY\n");
return -ENODEV;
}
@@ -1029,9 +1025,15 @@ static int fs_enet_probe(struct platform_device *ofdev)
fpi->use_napi = 1;
fpi->napi_weight = 17;
fpi->phy_node = of_parse_phandle(ofdev->dev.of_node, "phy-handle", 0);
- if ((!fpi->phy_node) && (!of_get_property(ofdev->dev.of_node, "fixed-link",
- NULL)))
- goto out_free_fpi;
+ if (!fpi->phy_node) {
+ if (of_phy_is_fixed_link(ofdev->dev.of_node)) {
+ err = of_phy_register_fixed_link(ofdev->dev.of_node);
+ if (err)
+ goto out_free_fpi;
+
+ fpi->phy_node = ofdev->dev.of_node;
+ }
+ }
if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec")) {
phy_connection_type = of_get_property(ofdev->dev.of_node,
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 6/9] gianfar: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/gianfar.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index e2d42475b006..282674027c92 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -889,6 +889,17 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(np)) {
+ err = of_phy_register_fixed_link(np);
+ if (err)
+ goto err_grp_init;
+
+ priv->phy_node = np;
+ }
+
/* Find the TBI PHY. If it's not there, we don't support SGMII */
priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
@@ -1660,9 +1671,6 @@ static int init_phy(struct net_device *dev)
priv->phydev = of_phy_connect(dev, priv->phy_node, &adjust_link, 0,
interface);
- if (!priv->phydev)
- priv->phydev = of_phy_connect_fixed_link(dev, &adjust_link,
- interface);
if (!priv->phydev) {
dev_err(&dev->dev, "could not attach to PHY\n");
return -ENODEV;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 7/9] ucc_geth: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
of_phy_connect_fixed_link() is becoming obsolete, and also required
platform code to register the fixed PHYs at the specified addresses for
those to be usable. Get rid of it and use the new of_phy_is_fixed_link()
plus of_phy_register_fixed_link() helpers to transition over the new
scheme.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/ethernet/freescale/ucc_geth.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index c8299c31b21f..fab39e295441 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1728,9 +1728,6 @@ static int init_phy(struct net_device *dev)
phydev = of_phy_connect(dev, ug_info->phy_node, &adjust_link, 0,
priv->phy_interface);
- if (!phydev)
- phydev = of_phy_connect_fixed_link(dev, &adjust_link,
- priv->phy_interface);
if (!phydev) {
dev_err(&dev->dev, "Could not attach to PHY\n");
return -ENODEV;
@@ -3790,6 +3787,17 @@ static int ucc_geth_probe(struct platform_device* ofdev)
ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
ug_info->phy_node = of_parse_phandle(np, "phy-handle", 0);
+ if (!ug_info->phy_node) {
+ /* In the case of a fixed PHY, the DT node associated
+ * to the PHY is the Ethernet MAC DT node.
+ */
+ if (of_phy_is_fixed_link(np)) {
+ err = of_phy_register_fixed_link(np);
+ if (err)
+ return err;
+ }
+ ug_info->phy_node = np;
+ }
/* Find the TBI PHY node. If it's not there, we don't support SGMII */
ug_info->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 8/9] of: mdio: remove of_phy_connect_fixed_link
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
All in-tree drivers have been converted to use the new pair of
functions: of_is_fixed_phy_link() plus of_phy_register_fixed_link(), we
can now safely remove of_phy_connect_fixed_link.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/of/of_mdio.c | 38 --------------------------------------
include/linux/of_mdio.h | 10 ----------
2 files changed, 48 deletions(-)
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 1def0bb5cb37..4c1e01ed16dc 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -246,44 +246,6 @@ struct phy_device *of_phy_connect(struct net_device *dev,
EXPORT_SYMBOL(of_phy_connect);
/**
- * of_phy_connect_fixed_link - Parse fixed-link property and return a dummy phy
- * @dev: pointer to net_device claiming the phy
- * @hndlr: Link state callback for the network device
- * @iface: PHY data interface type
- *
- * This function is a temporary stop-gap and will be removed soon. It is
- * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers. Do
- * not call this function from new drivers.
- */
-struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface)
-{
- struct device_node *net_np;
- char bus_id[MII_BUS_ID_SIZE + 3];
- struct phy_device *phy;
- const __be32 *phy_id;
- int sz;
-
- if (!dev->dev.parent)
- return NULL;
-
- net_np = dev->dev.parent->of_node;
- if (!net_np)
- return NULL;
-
- phy_id = of_get_property(net_np, "fixed-link", &sz);
- if (!phy_id || sz < sizeof(*phy_id))
- return NULL;
-
- sprintf(bus_id, PHY_ID_FMT, "fixed-0", be32_to_cpu(phy_id[0]));
-
- phy = phy_connect(dev, bus_id, hndlr, iface);
- return IS_ERR(phy) ? NULL : phy;
-}
-EXPORT_SYMBOL(of_phy_connect_fixed_link);
-
-/**
* of_phy_attach - Attach to a PHY without starting the state machine
* @dev: pointer to net_device claiming the phy
* @phy_np: Node pointer for the PHY
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 0aa367e316cb..d449018d0726 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -22,9 +22,6 @@ extern struct phy_device *of_phy_connect(struct net_device *dev,
struct phy_device *of_phy_attach(struct net_device *dev,
struct device_node *phy_np, u32 flags,
phy_interface_t iface);
-extern struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface);
extern struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np);
@@ -59,13 +56,6 @@ static inline struct phy_device *of_phy_attach(struct net_device *dev,
return NULL;
}
-static inline struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
- void (*hndlr)(struct net_device *),
- phy_interface_t iface)
-{
- return NULL;
-}
-
static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
{
return NULL;
--
1.9.1
^ permalink raw reply related
* [PATCH net-next 9/9] powerpc/fsl: fsl_soc: remove 'fixed-link' parsing code
From: Florian Fainelli @ 2014-05-20 0:56 UTC (permalink / raw)
To: netdev
Cc: Mark Rutland, open list:DOCUMENTATION, Paul Mackerras,
Florian Fainelli, Claudiu Manoil, Grant Likely,
open list:OPEN FIRMWARE AND..., Pawel Moll, Ian Campbell,
Richard Cochran, Rob Herring, Aida Mynzhasova, Thomas Petazzoni,
Sergei Shtylyov, Randy Dunlap, open list, davem, Vitaly Bordug,
Kumar Gala, open list:LINUX FOR POWERPC..., David S. Miller
In-Reply-To: <1400547384-11363-1-git-send-email-f.fainelli@gmail.com>
Parsing and registration of fixed PHY devices was needed with the use of
of_phy_connect_fixed_link() because this function was using the
designated PHY address identifier (first cell of the property) as the
address to bind the PHY on the emulated bus.
Since commit 3be2a49e5c08d268f8af0dd4fe89a24ea8cdc339 ("of: provide a
binding for fixed link PHYs") a new pair of functions has been
introduced which allows for dynamic address allocation of these fixed
PHY devices, but also parses the old 'fixed-link' 5-digit property.
Registration of fixed PHY early in platform code was needed because we
could not issue a fixed MDIO bus re-scan within network drivers. The
fixed PHYs had to be registered before the network drivers would call
of_phy_connect_fixed_link(). All of these caveats are solved now, such
that we can safely remove of_add_fixed_phys() now.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
arch/powerpc/sysdev/fsl_soc.c | 32 --------------------------------
1 file changed, 32 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 228cf91b91c1..ffd1169ebaab 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -25,7 +25,6 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/phy.h>
-#include <linux/phy_fixed.h>
#include <linux/spi/spi.h>
#include <linux/fsl_devices.h>
#include <linux/fs_enet_pd.h>
@@ -178,37 +177,6 @@ u32 get_baudrate(void)
EXPORT_SYMBOL(get_baudrate);
#endif /* CONFIG_CPM2 */
-#ifdef CONFIG_FIXED_PHY
-static int __init of_add_fixed_phys(void)
-{
- int ret;
- struct device_node *np;
- u32 *fixed_link;
- struct fixed_phy_status status = {};
-
- for_each_node_by_name(np, "ethernet") {
- fixed_link = (u32 *)of_get_property(np, "fixed-link", NULL);
- if (!fixed_link)
- continue;
-
- status.link = 1;
- status.duplex = fixed_link[1];
- status.speed = fixed_link[2];
- status.pause = fixed_link[3];
- status.asym_pause = fixed_link[4];
-
- ret = fixed_phy_add(PHY_POLL, fixed_link[0], &status);
- if (ret) {
- of_node_put(np);
- return ret;
- }
- }
-
- return 0;
-}
-arch_initcall(of_add_fixed_phys);
-#endif /* CONFIG_FIXED_PHY */
-
#if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
static __be32 __iomem *rstcr;
--
1.9.1
^ permalink raw reply related
* powerpc/powernv: Add calls to support little endian host
From: Benjamin Herrenschmidt @ 2014-05-20 1:01 UTC (permalink / raw)
To: linuxppc-dev list
When running as a powernv "host" system on P8, we need to switch
the endianness of interrupt handlers. This does it via the appropriate
call to the OPAL firmware which may result in just switching HID0:HILE
but depending on the processor version might need to do a few more
things. This call must be done early before any other processor has
been brought out of firmware.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 0394e91..a2a3d0d 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -129,6 +129,7 @@ extern int opal_enter_rtas(struct rtas_args *args,
#define OPAL_LPC_READ 67
#define OPAL_LPC_WRITE 68
#define OPAL_RETURN_CPU 69
+#define OPAL_REINIT_CPUS 70
#define OPAL_FLASH_VALIDATE 76
#define OPAL_FLASH_MANAGE 77
#define OPAL_FLASH_UPDATE 78
@@ -599,6 +600,11 @@ struct OpalIoPhb3ErrorData {
uint64_t pestB[OPAL_PHB3_NUM_PEST_REGS];
};
+enum {
+ OPAL_REINIT_CPUS_HILE_BE = (1 << 0),
+ OPAL_REINIT_CPUS_HILE_LE = (1 << 1),
+};
+
typedef struct oppanel_line {
const char * line;
uint64_t line_len;
@@ -719,6 +725,7 @@ int64_t opal_pci_next_error(uint64_t phb_id, uint64_t *first_frozen_pe,
uint16_t *pci_error_type, uint16_t *severity);
int64_t opal_pci_poll(uint64_t phb_id);
int64_t opal_return_cpu(void);
+int64_t opal_reinit_cpus(uint64_t flags);
int64_t opal_xscom_read(uint32_t gcid, uint64_t pcb_addr, __be64 *val);
int64_t opal_xscom_write(uint32_t gcid, uint64_t pcb_addr, uint64_t val);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index e780650..3abb518 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -123,6 +123,7 @@ OPAL_CALL(opal_xscom_write, OPAL_XSCOM_WRITE);
OPAL_CALL(opal_lpc_read, OPAL_LPC_READ);
OPAL_CALL(opal_lpc_write, OPAL_LPC_WRITE);
OPAL_CALL(opal_return_cpu, OPAL_RETURN_CPU);
+OPAL_CALL(opal_reinit_cpus, OPAL_REINIT_CPUS);
OPAL_CALL(opal_validate_flash, OPAL_FLASH_VALIDATE);
OPAL_CALL(opal_manage_flash, OPAL_FLASH_MANAGE);
OPAL_CALL(opal_update_flash, OPAL_FLASH_UPDATE);
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 1c798cd..359f18a 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -42,6 +42,21 @@ static DEFINE_SPINLOCK(opal_notifier_lock);
static uint64_t last_notified_mask = 0x0ul;
static atomic_t opal_notifier_hold = ATOMIC_INIT(0);
+static void opal_reinit_cores(void)
+{
+ /* Do the actual re-init, This will clobber all FPRs, VRs, etc...
+ *
+ * It will preserve non volatile GPRs and HSPRG0/1. It will
+ * also restore HIDs and other SPRs to their original value
+ * but it might clobber a bunch.
+ */
+#ifdef __BIG_ENDIAN__
+ opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_BE);
+#else
+ opal_reinit_cpus(OPAL_REINIT_CPUS_HILE_LE);
+#endif
+}
+
int __init early_init_dt_scan_opal(unsigned long node,
const char *uname, int depth, void *data)
{
@@ -77,6 +92,13 @@ int __init early_init_dt_scan_opal(unsigned long node,
printk("OPAL V1 detected !\n");
}
+ /* Reinit all cores with the right endian */
+ opal_reinit_cores();
+
+ /* Restore some bits */
+ if (cur_cpu_spec->cpu_restore)
+ cur_cpu_spec->cpu_restore();
+
return 1;
}
^ permalink raw reply related
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Madhavan Srinivasan @ 2014-05-20 2:06 UTC (permalink / raw)
To: Hugh Dickins
Cc: linux-arch, riel, x86, dave.hansen, peterz, Rusty Russell,
linux-kernel, linux-mm, ak, paulus, mgorman, akpm, linuxppc-dev,
mingo, Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
On Tuesday 20 May 2014 04:53 AM, Hugh Dickins wrote:
> On Mon, 19 May 2014, Madhavan Srinivasan wrote:
>> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
>>> Hugh Dickins <hughd@google.com> writes:
>>>> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
>>>>>
>>>>> Hi Ingo,
>>>>>
>>>>> Do you have any comments for the latest version of the patchset. If
>>>>> not, kindly can you pick it up as is.
>>>>>
>>>>>
>>>>> With regards
>>>>> Maddy
>>>>>
>>>>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
>>>>>> vm_ops->map_pages() for mapping easy accessible pages around
>>>>>> fault address in hope to reduce number of minor page faults.
>>>>>>
>>>>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
>>>>>> value using mm/Kconfig. This will enable architecture maintainers
>>>>>> to decide on suitable FAULT_AROUND_ORDER value based on
>>>>>> performance data for that architecture. First patch also defaults
>>>>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
>>>>>> out the performance numbers for powerpc (platform pseries) and
>>>>>> initialize the fault around order variable for pseries platform of
>>>>>> powerpc.
>>>>
>>>> Sorry for not commenting earlier - just reminded by this ping to Ingo.
>>>>
>>>> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
>>>>
>>>> arch/powerpc/Kconfig suggests that Power supports base page size of
>>>> 4k, 16k, 64k or 256k.
>>>>
>>>> I would expect your optimal fault_around_order to depend very much on
>>>> the base page size.
>>>
>>> It was 64k, which is what PPC64 uses on all the major distributions.
>>> You really only get a choice of 4k and 64k with 64 bit power.
>>>
>> This is true. PPC64 support multiple pagesize and yes the default page
>> size of 64k, is taken as base pagesize for the tests.
>>
>>>> Perhaps fault_around_size would provide a more useful default?
>>>
>>> That seems to fit. With 4k pages and order 4, you're asking for 64k.
>>> Maddy's result shows 64k is also reasonable for 64k pages.
>>>
>>> Perhaps we try to generalize from two data points (a slight improvement
>>> over doing it from 1!), eg:
>>>
>>> /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
>>> unsigned int fault_around_order __read_mostly =
>>> (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
>
> Rusty's bimodal answer doesn't seem the right starting point to me.
>
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
> - when that doesn't wrap, of course!
>
> That would at least have a better chance of being appropriate for
> architectures with 8k and 16k pages (Itanium springs to mind).
>
> Not necessarily right for them, since each architecture may have
> different faulting overheads; but a better chance of being right
> than blindly assuming 4k or 64k pages for everyone.
>
> I'd be glad to see that change go into v3.15: what do you think,
> Kirill, are we too late to make such a change now?
> Or do you see some objection to it?
>
>> This may be right. But these are the concerns, will not this make other
>> arch to pick default without any tuning
>
> Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
> Did other architectures, with other page sizes, back that default?
> Clearly not powerpc.
Ok.
>
>> and also this will remove the
>> compile time option to disable the feature?
>
> Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
> for v3.16?
>
> I'm not sure whether Rusty was arguing against that or not I think
> we are all three concerned to have a more sensible default than what's
> there at present. I don't feel very strongly about your Kconfig
Added it as one way to reset or disable the default value. But then I
guess we decided on having FAULT_AROUND_ORDER as a variable which is
more important than Kconfig option.
> option: I've no objection, if it were to default to byte order 16.
>
Thanks for review
With regards
Maddy
> Hugh
>
^ permalink raw reply
* Re: [PATCH V4 0/2] mm: FAULT_AROUND_ORDER patchset performance data for powerpc
From: Rusty Russell @ 2014-05-20 1:14 UTC (permalink / raw)
To: Hugh Dickins, Madhavan Srinivasan
Cc: linux-arch, riel, ak, dave.hansen, peterz, x86, linux-kernel,
linux-mm, paulus, mgorman, akpm, linuxppc-dev, mingo,
Kirill A. Shutemov
In-Reply-To: <alpine.LSU.2.11.1405191531150.1317@eggly.anvils>
Hugh Dickins <hughd@google.com> writes:
> On Mon, 19 May 2014, Madhavan Srinivasan wrote:
>> On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
>> > Hugh Dickins <hughd@google.com> writes:
>> >> On Thu, 15 May 2014, Madhavan Srinivasan wrote:
>> >>>
>> >>> Hi Ingo,
>> >>>
>> >>> Do you have any comments for the latest version of the patchset. If
>> >>> not, kindly can you pick it up as is.
>> >>>
>> >>>
>> >>> With regards
>> >>> Maddy
>> >>>
>> >>>> Kirill A. Shutemov with 8c6e50b029 commit introduced
>> >>>> vm_ops->map_pages() for mapping easy accessible pages around
>> >>>> fault address in hope to reduce number of minor page faults.
>> >>>>
>> >>>> This patch creates infrastructure to modify the FAULT_AROUND_ORDER
>> >>>> value using mm/Kconfig. This will enable architecture maintainers
>> >>>> to decide on suitable FAULT_AROUND_ORDER value based on
>> >>>> performance data for that architecture. First patch also defaults
>> >>>> FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
>> >>>> out the performance numbers for powerpc (platform pseries) and
>> >>>> initialize the fault around order variable for pseries platform of
>> >>>> powerpc.
>> >>
>> >> Sorry for not commenting earlier - just reminded by this ping to Ingo.
>> >>
>> >> I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
>> >>
>> >> arch/powerpc/Kconfig suggests that Power supports base page size of
>> >> 4k, 16k, 64k or 256k.
>> >>
>> >> I would expect your optimal fault_around_order to depend very much on
>> >> the base page size.
>> >
>> > It was 64k, which is what PPC64 uses on all the major distributions.
>> > You really only get a choice of 4k and 64k with 64 bit power.
>> >
>> This is true. PPC64 support multiple pagesize and yes the default page
>> size of 64k, is taken as base pagesize for the tests.
>>
>> >> Perhaps fault_around_size would provide a more useful default?
>> >
>> > That seems to fit. With 4k pages and order 4, you're asking for 64k.
>> > Maddy's result shows 64k is also reasonable for 64k pages.
>> >
>> > Perhaps we try to generalize from two data points (a slight improvement
>> > over doing it from 1!), eg:
>> >
>> > /* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
>> > unsigned int fault_around_order __read_mostly =
>> > (16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
>
> Rusty's bimodal answer doesn't seem the right starting point to me.
? It's not bimodal, it's graded. I think you misread?
> Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
> the order of the fault-around size in bytes, and fault_around_pages()
> use 1UL << (fault_around_order - PAGE_SHIFT)
> - when that doesn't wrap, of course!
>
> That would at least have a better chance of being appropriate for
> architectures with 8k and 16k pages (Itanium springs to mind).
Well, from our two data points it seems that we want to fault in
64k at a time whatever our page size. Perhaps it's clearer if the
code expresses itself that way.
> Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
> Did other architectures, with other page sizes, back that default?
> Clearly not powerpc.
Yeah, BenH flagged it as "we should test this" for powerpc, which is
what Maddy then did.
>> and also this will remove the
>> compile time option to disable the feature?
>
> Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
> for v3.16?
>
> I'm not sure whether Rusty was arguing against that or not. I think
> we are all three concerned to have a more sensible default than what's
> there at present. I don't feel very strongly about your Kconfig
> option: I've no objection, if it were to default to byte order 16.
I don't mind either.
Cheers,
Rusty.
^ 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