* Re: [PATCH RESEND net-next 4/9] net: systemport: use the new fixed PHY helpers
From: Sergei Shtylyov @ 2014-05-21 21:55 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: Thomas Petazzoni, open list:OPEN FIRMWARE AND..., Aida Mynzhasova,
Richard Cochran, open list, Claudiu Manoil, Vitaly Bordug,
Grant Likely, open list:LINUX FOR POWERPC..., davem
In-Reply-To: <1400708331-18088-5-git-send-email-f.fainelli@gmail.com>
On 05/22/2014 01:38 AM, Florian Fainelli wrote:
> 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);
The continuation line should start on the next character after ( on the
above line, according to the networking coding style.
[...]
WBR, Sergei
^ permalink raw reply
* Re: [PATCH v5 3/4] drivers/vfio: EEH support for VFIO PCI device
From: Benjamin Herrenschmidt @ 2014-05-21 21:56 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537CA50E.9090404@suse.de>
On Wed, 2014-05-21 at 15:07 +0200, Alexander Graf wrote:
> > +#ifdef CONFIG_VFIO_PCI_EEH
> > +int eeh_vfio_open(struct pci_dev *pdev)
>
> Why vfio? Also that config option will not be set if vfio is compiled as
> a module.
>
> > +{
> > + struct eeh_dev *edev;
> > +
> > + /* No PCI device ? */
> > + if (!pdev)
> > + return -ENODEV;
> > +
> > + /* No EEH device ? */
> > + edev = pci_dev_to_eeh_dev(pdev);
> > + if (!edev || !edev->pe)
> > + return -ENODEV;
> > +
> > + eeh_dev_set_passed(edev, true);
> > + eeh_pe_set_passed(edev->pe, true);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(eeh_vfio_open);
Additionally, shouldn't we have some locking here ? (and in release too)
I don't like relying on the caller locking (if it does it at all).
> > + /* Device existing ? */
> > + ret = eeh_vfio_check_dev(pdev, &edev, &pe);
> > + if (ret) {
> > + pr_debug("%s: Cannot find device %s\n",
> > + __func__, pdev ? pci_name(pdev) : "NULL");
> > + *retval = -7;
>
> What are these? Please use proper kernel internal return values for
> errors. I don't want to see anything even remotely tied to RTAS in any
> of these patches.
Hint: -ENODEV
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH RESEND net-next 5/9] fs_enet: use the new fixed PHY helpers
From: Sergei Shtylyov @ 2014-05-21 21:58 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: Thomas Petazzoni, open list:OPEN FIRMWARE AND..., Aida Mynzhasova,
Richard Cochran, open list, Claudiu Manoil, Vitaly Bordug,
Grant Likely, open list:LINUX FOR POWERPC..., davem
In-Reply-To: <1400708331-18088-6-git-send-email-f.fainelli@gmail.com>
On 05/22/2014 01:38 AM, Florian Fainelli wrote:
> 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
[...]
> @@ -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)) {
These two *if* statements could be collapsed, reducing the indentation
level below.
> + err = of_phy_register_fixed_link(ofdev->dev.of_node);
> + if (err)
> + goto out_free_fpi;
> +
> + fpi->phy_node = ofdev->dev.of_node;
> + }
> + }
WBR, Sergei
^ permalink raw reply
* Re: [PATCH RESEND net-next 4/9] net: systemport: use the new fixed PHY helpers
From: Florian Fainelli @ 2014-05-21 22:02 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Thomas Petazzoni, open list:OPEN FIRMWARE AND..., Aida Mynzhasova,
netdev, Richard Cochran, open list, Claudiu Manoil, Vitaly Bordug,
Grant Likely, open list:LINUX FOR POWERPC..., David Miller
In-Reply-To: <537D20EA.1000305@cogentembedded.com>
2014-05-21 14:55 GMT-07:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> On 05/22/2014 01:38 AM, Florian Fainelli wrote:
>
>> 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);
>
>
> The continuation line should start on the next character after ( on the
> above line, according to the networking coding style.
Unless I am once again not following the coding style, the patch in
patchwork has this correctly, and so does my file locally:
http://patchwork.ozlabs.org/patch/351323/
--
Florian
^ permalink raw reply
* Re: [PATCH v5 3/4] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-21 23:48 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537CA50E.9090404@suse.de>
On Wed, May 21, 2014 at 03:07:26PM +0200, Alexander Graf wrote:
>
>On 21.05.14 07:03, Gavin Shan wrote:
>>The patch adds new IOCTL command VFIO_EEH_OP to VFIO PCI device
>>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>
>>---
>> Documentation/vfio.txt | 6 +-
>> arch/powerpc/include/asm/eeh.h | 10 ++
>> arch/powerpc/kernel/eeh.c | 323 +++++++++++++++++++++++++++++++++++++++++
>> drivers/vfio/pci/vfio_pci.c | 99 ++++++++++++-
>> include/uapi/linux/vfio.h | 43 ++++++
>> 5 files changed, 474 insertions(+), 7 deletions(-)
>>
>>diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>>index b9ca023..bb17ec7 100644
>>--- a/Documentation/vfio.txt
>>+++ b/Documentation/vfio.txt
>>@@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
>> an excellent performance which has limitations such as inability to do
>> locked pages accounting in real time.
>>-So 3 additional ioctls have been added:
>>+4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
>>+which works on the basis of additional ioctl command VFIO_EEH_OP.
>>+
>>+So 4 additional ioctls have been added:
>> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
>> of the DMA window on the PCI bus.
>>@@ -316,6 +319,7 @@ So 3 additional ioctls have been added:
>> VFIO_IOMMU_DISABLE - disables the container.
>>+ VFIO_EEH_OP - EEH dependent operations
>
>Please document exactly what the ioctl does. In an ideal world, a
>VFIO user will just look at the documentation and be able to write a
>program against the API with it.
>
Ok. I'll amend it. Also, I'll split it to 5 ioctl commands in next revision.
>> The code flow from the example above should be slightly changed:
>>diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>>index 34a2d83..93922ef 100644
>>--- a/arch/powerpc/include/asm/eeh.h
>>+++ b/arch/powerpc/include/asm/eeh.h
>>@@ -305,6 +305,16 @@ void eeh_add_device_late(struct pci_dev *);
>> void eeh_add_device_tree_late(struct pci_bus *);
>> void eeh_add_sysfs_files(struct pci_bus *);
>> void eeh_remove_device(struct pci_dev *);
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+int eeh_vfio_open(struct pci_dev *pdev);
>>+void eeh_vfio_release(struct pci_dev *pdev);
>>+int eeh_vfio_set_pe_option(struct pci_dev *pdev, int option, int *retval);
>>+int eeh_vfio_get_pe_addr(struct pci_dev *pdev, int option,
>>+ int *retval, int *info);
>>+int eeh_vfio_get_pe_state(struct pci_dev *pdev, int *retval, int *state);
>>+int eeh_vfio_reset_pe(struct pci_dev *pdev, int option, int *retval);
>>+int eeh_vfio_configure_pe(struct pci_dev *pdev, int *retval);
>>+#endif
>> /**
>> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
>>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>index 9c6b899..2aaf90e 100644
>>--- a/arch/powerpc/kernel/eeh.c
>>+++ b/arch/powerpc/kernel/eeh.c
>>@@ -1098,6 +1098,329 @@ void eeh_remove_device(struct pci_dev *dev)
>> edev->mode &= ~EEH_DEV_SYSFS;
>> }
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+int eeh_vfio_open(struct pci_dev *pdev)
>
>Why vfio? Also that config option will not be set if vfio is compiled
>as a module.
>
The interface is expected to be used by VFIO-PCI. I'll change the function
names to following ones in next revision. No "VFIO" will be seen :-)
eeh_dev_open();
eeh_dev_release();
static eeh_dev_check();
eeh_pe_set_option();
eeh_pe_get_addr();
eeh_pe_get_state();
eeh_pe_reset();
eeh_pe_configure();
Yeah, "#ifdef CONFIG_VFIO_PCI_EEH" can be removed safely in next revision.
>>+{
>>+ struct eeh_dev *edev;
>>+
>>+ /* No PCI device ? */
>>+ if (!pdev)
>>+ return -ENODEV;
>>+
>>+ /* No EEH device ? */
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !edev->pe)
>>+ return -ENODEV;
>>+
>>+ eeh_dev_set_passed(edev, true);
>>+ eeh_pe_set_passed(edev->pe, true);
>>+
>>+ return 0;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_open);
>>+
>>+void eeh_vfio_release(struct pci_dev *pdev)
>>+{
>>+ bool release_pe = true;
>>+ struct eeh_pe *pe = NULL;
>>+ struct eeh_dev *tmp, *edev;
>>+
>>+ /* No PCI device ? */
>>+ if (!pdev)
>>+ return;
>>+
>>+ /* No EEH device ? */
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !eeh_dev_passed(edev) ||
>>+ !edev->pe || !eeh_pe_passed(pe))
>>+ return;
>>+
>>+ /* Release device */
>>+ pe = edev->pe;
>>+ eeh_dev_set_passed(edev, false);
>>+
>>+ /* Release PE */
>>+ eeh_pe_for_each_dev(pe, edev, tmp) {
>>+ if (eeh_dev_passed(edev)) {
>>+ release_pe = false;
>>+ break;
>>+ }
>>+ }
>>+
>>+ if (release_pe)
>>+ eeh_pe_set_passed(pe, false);
>>+}
>>+EXPORT_SYMBOL(eeh_vfio_release);
>>+
>>+static int eeh_vfio_check_dev(struct pci_dev *pdev,
>>+ struct eeh_dev **pedev,
>>+ struct eeh_pe **ppe)
>>+{
>>+ struct eeh_dev *edev;
>>+
>>+ /* No device ? */
>>+ if (!pdev)
>>+ return -ENODEV;
>>+
>>+ edev = pci_dev_to_eeh_dev(pdev);
>>+ if (!edev || !eeh_dev_passed(edev) ||
>>+ !edev->pe || !eeh_pe_passed(edev->pe))
>>+ return -ENODEV;
>>+
>>+ if (pedev)
>>+ *pedev = edev;
>>+ if (ppe)
>>+ *ppe = edev->pe;
>>+
>>+ return 0;
>>+}
>>+
>>+int eeh_vfio_set_pe_option(struct pci_dev *pdev, int option, int *retval)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>>+ if (ret) {
>>+ pr_debug("%s: Cannot find device %s\n",
>>+ __func__, pdev ? pci_name(pdev) : "NULL");
>>+ *retval = -7;
>
>What are these? Please use proper kernel internal return values for
>errors. I don't want to see anything even remotely tied to RTAS in
>any of these patches.
>
It's the return value for RTAS call "ibm,set-eeh-option". Yeah, it
makes sense to return kernel internal values for errors and will
amend in next revision.
>>+ goto out;
>>+ }
>>+
>>+ /* Invalid option ? */
>>+ if (option < EEH_OPT_DISABLE ||
>>+ option > EEH_OPT_THAW_DMA) {
>
>This is quite confusing to read because it's not obvious what is in
>between these. Just make this a switch() statement that lists the
>allowed options. Gcc will be smart enough to optimize that into a
>bounds check.
>
Ok. Will use "switch()" in next revision.
>>+ pr_debug("%s: Option %d out of range (%d, %d)\n",
>>+ __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
>>+ *retval = -3;
>>+ ret = -EINVAL;
>>+ goto out;
>>+ }
>>+
>>+ if (option == EEH_OPT_DISABLE ||
>>+ option == EEH_OPT_ENABLE) {
>>+ *retval = 0;
>>+ } else {
>>+ if (!eeh_ops || !eeh_ops->set_option) {
>>+ *retval = -7;
>>+ ret = -ENOENT;
>>+ goto out;
>>+ }
>>+
>>+ ret = eeh_ops->set_option(pe, option);
>>+ if (ret) {
>>+ pr_debug("%s: Failure %d from backend\n",
>>+ __func__, ret);
>>+ *retval = -1;
>>+ goto out;
>>+ }
>>+
>>+ *retval = 0;
>>+ }
>>+out:
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_set_pe_option);
>>+
>>+int eeh_vfio_get_pe_addr(struct pci_dev *pdev, int option,
>>+ int *retval, int *info)
>>+{
>>+ struct pci_bus *bus;
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>>+ if (ret) {
>>+ *retval = -3;
>>+ goto out;
>>+ }
>>+
>>+ /* Invalid option ? */
>>+ if (option != 0 && option != 1) {
>
>0? 1? What? Don't these have names? And again, please use a switch()
>for this function.
>
Will have names in next revision and use "switch()", thanks.
>>+ pr_debug("%s: option %d out of range (0, 1)\n",
>>+ __func__, option);
>>+ *retval = -3;
>>+ ret = -EINVAL;
>>+ goto out;
>>+ }
>>+
>>+ /*
>>+ * Fill result according to option. We don't differentiate
>>+ * PCI bus and device dependent PE here. So all PEs are
>>+ * built in "shared" mode. Also, the PE address has the format
>>+ * of "00BBSS00".
>>+ */
>>+ if (option == 0) {
>>+ bus = eeh_pe_bus_get(pe);
>>+ if (!bus) {
>>+ *retval = -3;
>>+ ret = -ENODEV;
>>+ goto out;
>>+ }
>>+
>>+ *retval = 0;
>>+ *info = bus->number << 16;
>
>How about positive numbers for the number and negative ones for errors?
>
We needn't carry error numbers by "info" because "retval" or "ret" already
had that information :-)
>>+ } else {
>>+ *retval = 0;
>>+ *info = 1;
>>+ }
>>+out:
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_get_pe_addr);
>>+
>>+int eeh_vfio_get_pe_state(struct pci_dev *pdev, int *retval, int *state)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int result, ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>>+ if (ret) {
>>+ *retval = -3;
>>+ goto out;
>>+ }
>>+
>>+ if (!eeh_ops || !eeh_ops->get_state) {
>>+ pr_debug("%s: Unsupported request\n",
>>+ __func__);
>>+ ret = -ENOENT;
>>+ *retval = -3;
>>+ goto out;
>>+ }
>>+
>>+ result = eeh_ops->get_state(pe, NULL);
>>+ if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ (result & EEH_STATE_DMA_ENABLED) &&
>>+ (result & EEH_STATE_MMIO_ENABLED))
>>+ *state = 0;
>>+ else if (result & EEH_STATE_RESET_ACTIVE)
>>+ *state = 1;
>>+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ !(result & EEH_STATE_DMA_ENABLED) &&
>>+ !(result & EEH_STATE_MMIO_ENABLED))
>>+ *state = 2;
>>+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
>>+ (result & EEH_STATE_DMA_ENABLED) &&
>>+ !(result & EEH_STATE_MMIO_ENABLED))
>>+ *state = 4;
>>+ else
>>+ *state = 5;
>
>What are these numbers?
>
Will have names in next revision :)
>>+
>>+ *retval = 0;
>>+out:
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_get_pe_state);
>>+
>>+int eeh_vfio_reset_pe(struct pci_dev *pdev, int option, int *retval)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>>+ if (ret) {
>>+ *retval = -3;
>>+ goto out;
>>+ }
>>+
>>+ /* Invalid option ? */
>>+ if (option != EEH_RESET_DEACTIVATE &&
>>+ option != EEH_RESET_HOT &&
>>+ option != EEH_RESET_FUNDAMENTAL) {
>>+ pr_debug("%s: Unsupported option %d\n",
>>+ __func__, option);
>>+ ret = -EINVAL;
>>+ *retval = -3;
>>+ goto out;
>>+ }
>>+
>>+ if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset) {
>>+ pr_debug("%s: Unsupported request\n",
>>+ __func__);
>>+ ret = -ENOENT;
>>+ *retval = -7;
>>+ goto out;
>>+ }
>>+
>>+ ret = eeh_ops->reset(pe, option);
>>+ if (ret) {
>>+ pr_debug("%s: Failure %d from backend\n",
>>+ __func__, ret);
>>+ *retval = -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
>
>recursive
>
Thanks.
>>+ * frozen PE.
>>+ */
>>+ if (option == EEH_RESET_DEACTIVATE) {
>>+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
>>+ if (ret) {
>>+ pr_debug("%s: Cannot enable IO for PHB#%d-PE#%d (%d)\n",
>>+ __func__, pe->phb->global_number, pe->addr, ret);
>>+ *retval = -1;
>>+ goto out;
>>+ }
>>+
>>+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
>>+ if (ret) {
>>+ pr_debug("%s: Cannot enable DMA for PHB#%d-PE#%d (%d)\n",
>>+ __func__, pe->phb->global_number, pe->addr, ret);
>>+ *retval = -1;
>>+ goto out;
>>+ }
>>+
>>+ eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
>>+ }
>>+
>>+ *retval = 0;
>>+out:
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_reset_pe);
>>+
>>+int eeh_vfio_configure_pe(struct pci_dev *pdev, int *retval)
>>+{
>>+ struct eeh_dev *edev;
>>+ struct eeh_pe *pe;
>>+ int ret = 0;
>>+
>>+ /* Device existing ? */
>>+ ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>>+ if (ret) {
>>+ *retval = -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.
>
>I don't understand this comment. When is "configure_pe" called in the
>first place? Please provide proper function descriptions for each of
>these exported functions that tell someone who may want to use them
>what they do.
>
Yeah, I'll add the description here (also in Documentation/vfio.txt).
>Also, don't mention VFIO or guests in any function inside this file.
>
Ok. I'll avoid mentioning "VFIO" and "guest".
>>+ */
>>+ eeh_pe_restore_bars(pe);
>>+ *retval = 0;
>>+
>>+out:
>>+ return ret;
>>+}
>>+EXPORT_SYMBOL_GPL(eeh_vfio_configure_pe);
>>+
>>+#endif /* CONFIG_VFIO_PCI_EEH */
>>+
>> static int proc_eeh_show(struct seq_file *m, void *v)
>> {
>> if (!eeh_enabled()) {
>>diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>>index 7ba0424..05c3dde 100644
>>--- a/drivers/vfio/pci/vfio_pci.c
>>+++ b/drivers/vfio/pci/vfio_pci.c
>>@@ -25,6 +25,9 @@
>> #include <linux/types.h>
>> #include <linux/uaccess.h>
>> #include <linux/vfio.h>
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+#include <asm/eeh.h>
>>+#endif
>> #include "vfio_pci_private.h"
>>@@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>> pci_restore_state(pdev);
>> }
>>+static void vfio_eeh_pci_release(struct pci_dev *pdev)
>>+{
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+ eeh_vfio_release(pdev);
>>+#endif
>>+}
>>+
>> static void vfio_pci_release(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>>- if (atomic_dec_and_test(&vdev->refcnt))
>>+ if (atomic_dec_and_test(&vdev->refcnt)) {
>>+ vfio_eeh_pci_release(vdev->pdev);
>> vfio_pci_disable(vdev);
>>+ }
>> module_put(THIS_MODULE);
>> }
>>+static int vfio_eeh_pci_open(struct pci_dev *pdev)
>>+{
>>+ int ret = 0;
>>+
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+ ret = eeh_vfio_open(pdev);
>>+#endif
>>+ return ret;
>>+}
>>+
>> static int vfio_pci_open(void *device_data)
>> {
>> struct vfio_pci_device *vdev = device_data;
>>+ int ret;
>> if (!try_module_get(THIS_MODULE))
>> return -ENODEV;
>> if (atomic_inc_return(&vdev->refcnt) == 1) {
>>- int ret = vfio_pci_enable(vdev);
>>- if (ret) {
>>- module_put(THIS_MODULE);
>>- return ret;
>>- }
>>+ ret = vfio_pci_enable(vdev);
>>+ if (ret)
>>+ goto error;
>>+
>>+ ret = vfio_eeh_pci_open(vdev->pdev);
>>+ if (ret)
>>+ goto error;
>> }
>> return 0;
>>+error:
>>+ module_put(THIS_MODULE);
>>+ return ret;
>> }
>> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
>>@@ -321,6 +349,51 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
>> return walk.ret;
>> }
>>+static int vfio_eeh_pci_ioctl(struct pci_dev *pdev, struct vfio_eeh_op *info)
>
>I still don't like the idea of that multiplexing ioctl. I don't see
>any benefit in it whatsoever. Just create 5 individual ioctls with
>their own simple interfaces.
>
Ok. Will split in next revision. Thanks.
>Also, this interface has nothing to do with RTAS. So don't sneak in
>RTAS error numbers anywhere ;). It's QEMU's task to convert from
>kernel error codes to RTAS error codes.
>
Ok. Will do in next revision. Thanks for your comments and time, Alex :-)
Thanks,
Gavin
>>+{
>>+ int ret = 0;
>>+
>>+#ifdef CONFIG_VFIO_PCI_EEH
>>+ switch (info->op) {
>>+ case VFIO_EEH_OP_SET_OPTION:
>>+ ret = eeh_vfio_set_pe_option(pdev,
>>+ info->option.option,
>>+ &info->option.ret);
>>+ break;
>>+ case VFIO_EEH_OP_GET_ADDR:
>>+ ret = eeh_vfio_get_pe_addr(pdev,
>>+ info->addr.option,
>>+ &info->addr.ret,
>>+ &info->addr.info);
>>+ break;
>>+ case VFIO_EEH_OP_GET_STATE:
>>+ ret = eeh_vfio_get_pe_state(pdev,
>>+ &info->state.ret,
>>+ &info->state.reset_state);
>>+ info->state.cfg_cap = 1;
>>+ info->state.pe_unavail_info = 1000;
>>+ info->state.pe_recovery_info = 0;
>>+ break;
>>+ case VFIO_EEH_OP_PE_RESET:
>>+ ret = eeh_vfio_reset_pe(pdev,
>>+ info->reset.option,
>>+ &info->reset.ret);
>>+ break;
>>+ case VFIO_EEH_OP_PE_CONFIG:
>>+ ret = eeh_vfio_configure_pe(pdev,
>>+ &info->config.ret);
>>+ default:
>>+ ret = -EINVAL;
>>+ pr_debug("%s: Cannot handle op#%d\n",
>>+ __func__, info->op);
>>+ }
>>+#else
>>+ ret = -ENOENT;
>>+#endif
>>+
>>+ return ret;
>>+}
>>+
>> static long vfio_pci_ioctl(void *device_data,
>> unsigned int cmd, unsigned long arg)
>> {
>>@@ -682,6 +755,20 @@ hot_reset_release:
>> kfree(groups);
>> return ret;
>>+ } else if (cmd == VFIO_EEH_OP) {
>>+ struct vfio_eeh_op info;
>>+ int ret = 0;
>>+
>>+ minsz = sizeof(info);
>>+ if (copy_from_user(&info, (void __user *)arg, minsz))
>>+ return -EFAULT;
>>+ if (info.argsz < minsz)
>>+ return -EINVAL;
>>+
>>+ ret = vfio_eeh_pci_ioctl(vdev->pdev, &info);
>>+ if (copy_to_user((void __user *)arg, &info, minsz))
>>+ ret = -EFAULT;
>>+ return ret;
>> }
>> return -ENOTTY;
>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>index cb9023d..518961d 100644
>>--- a/include/uapi/linux/vfio.h
>>+++ b/include/uapi/linux/vfio.h
>>@@ -455,6 +455,49 @@ struct vfio_iommu_spapr_tce_info {
>> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>>+/*
>>+ * The VFIO operation struct provides way to support EEH functionality
>>+ * for PCI device that is passed from host to guest via VFIO.
>>+ */
>>+#define VFIO_EEH_OP_SET_OPTION 0
>>+#define VFIO_EEH_OP_GET_ADDR 1
>>+#define VFIO_EEH_OP_GET_STATE 2
>>+#define VFIO_EEH_OP_PE_RESET 3
>>+#define VFIO_EEH_OP_PE_CONFIG 4
>>+
>>+struct vfio_eeh_op {
>>+ __u32 argsz;
>>+ __u32 op;
>>+
>>+ union {
>>+ struct vfio_eeh_set_option {
>>+ __u32 option;
>>+ __s32 ret;
>>+ } option;
>>+ struct vfio_eeh_pe_addr {
>>+ __u32 option;
>>+ __s32 ret;
>>+ __u32 info;
>>+ } addr;
>>+ struct vfio_eeh_pe_state {
>>+ __s32 ret;
>>+ __u32 reset_state;
>>+ __u32 cfg_cap;
>>+ __u32 pe_unavail_info;
>>+ __u32 pe_recovery_info;
>>+ } state;
>>+ struct vfio_eeh_reset {
>>+ __u32 option;
>>+ __s32 ret;
>>+ } reset;
>>+ struct vfio_eeh_config {
>>+ __s32 ret;
>>+ } config;
>>+ };
>>+};
>>+
>>+#define VFIO_EEH_OP _IO(VFIO_TYPE, VFIO_BASE + 21)
>>+
>> /* ***************************************************************** */
>> #endif /* _UAPIVFIO_H */
>
>--
>To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v5 4/4] powerpc/eeh: Avoid event on passed PE
From: Gavin Shan @ 2014-05-22 0:01 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <537CA667.2070906@suse.de>
On Wed, May 21, 2014 at 03:13:11PM +0200, Alexander Graf wrote:
>
>On 21.05.14 07:03, Gavin Shan wrote:
>>If we detects frozen state on PE that has been passed to guest, we
>>needn't handle it. Instead, we rely on the guest to detect and recover
>>it. The patch avoid EEH event on the frozen passed PE so that the guest
>>can have chance to handle that.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>---
>> arch/powerpc/kernel/eeh.c | 8 ++++++++
>> arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>>index 2aaf90e..25fd12d 100644
>>--- a/arch/powerpc/kernel/eeh.c
>>+++ b/arch/powerpc/kernel/eeh.c
>>@@ -400,6 +400,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
>> if (ret > 0)
>> return ret;
>>+ /*
>>+ * If the PE has been passed to guest, we won't check the
>>+ * state. Instead, let the guest handle it if the PE has
>
>What guest? The kernel doesn't care whether we use VFIO for a guest or not.
>
Ok. I'll not mention "guest" and "vfio" in next revision.
Thanks,
Gavin
>
>Alex
>
>>+ * been frozen.
>>+ */
>>+ if (eeh_pe_passed(pe))
>>+ return 0;
>>+
>> /* If we already have a pending isolation event for this
>> * slot, we know it's bad already, we don't need to check.
>> * Do this checking under a lock; as multiple PCI devices
>>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>index 1b5982f..03a3ed2 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>>@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
>> opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
>> OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
>> ret = EEH_NEXT_ERR_NONE;
>>- } else if ((*pe)->state & EEH_PE_ISOLATED) {
>>+ } else if ((*pe)->state & EEH_PE_ISOLATED ||
>>+ eeh_pe_passed(*pe)) {
>> ret = EEH_NEXT_ERR_NONE;
>> } else {
>> pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
>
^ permalink raw reply
* Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets
From: Anshuman Khandual @ 2014-05-22 5:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: mikey, avagin, oleg, linux-kernel, michael, linuxppc-dev
In-Reply-To: <537B2F5E.4040102@redhat.com>
On 05/20/2014 04:03 PM, Pedro Alves wrote:
> On 05/20/2014 09:14 AM, Anshuman Khandual wrote:
>> On 05/19/2014 08:13 PM, Pedro Alves wrote:
>>> On 05/19/2014 12:46 PM, Anshuman Khandual wrote:
>>>
>>>>>> I couldn't actually find any arch that currently returns -ENODEV in
>>>>>> the "active" hook. I see that binfmt_elf.c doesn't handle
>>>>>> regset->active() returning < 0. Guess that may be why. Looks like
>>>>>> something that could be cleaned up, to me.
>>>>>>
>>>> Also it does not consider the return value of regset->active(t->task, regset)
>>>> (whose objective is to figure out whether we need to request regset->n number
>>>> of elements or less than that) in the subsequent call to regset->get function.
>>>
>>> Indeed.
>>>
>>> TBC, do you plan on fixing this? Otherwise ...
>>
>> Sure, thinking something like this as mentioned below. But still not sure how to use
>> the return type of -ENODEV from the function regset->active(). Right now if any
>> regset does have the active hook and it returns anything but positive value, it will
>> be ignored and the control moves to the next regset in view. This prevents the thread
>> core note type being written to the core dump.
>
> Looks to me that that's exactly what should happen for -ENODEV too. The regset
> should be ignored. If regset->active() returns -ENODEV, then the machine
> doesn't have the registers at all, so what makes sense to me is to not write the
> corresponding core note in the dump. IOW, on such a machine, the kernel
> generates a core exactly like if the support for these registers that don't
> make sense for this machine wasn't compiled in at all. And generates a core
> exactly like an older kernel that didn't know about that regset
> (which is fine for that same machine) yet.
>
All of this happen right now even without specifically checking for the return type
of -ENODEV and just checking for a positive value. I guess thats the reason they had
omitted -ENODEV in the first place.
>>
>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> index aa3cb62..80672fb 100644
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -1553,7 +1553,15 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
>> if (regset->core_note_type && regset->get &&
>> (!regset->active || regset->active(t->task, regset))) {
>> int ret;
>
> So, here, this ?
>
> (!regset->active || regset->active(t->task, regset) > 0)) {
>
>
>> - size_t size = regset->n * regset->size;
>> + size_t size;
>> +
>> + /* Request only the active elements in the regset */
>> + if (!regset->active)
>> + size = regset->n * regset->size;
>> + else
>> + size = regset->active(t->task, regset)
>> + * regset->size;
>> +
>
>
> I wonder if it wouldn't be cleaner to add a function like:
>
> int
> regset_active (tast *task, regseg *regset)
> {
> if (!regset->active)
> return regset->n * regset->size;
> else
> return regset->active(task, regset);
> }
>
> And then use it like
>
> if (regset->core_note_type && regset->get) {
> int size = regset_active (t->task, regset);
>
> if (size > 0) {
> ...
> }
>
Yeah this makes sense.
> Though at this point, we don't actually make use of
> the distinction between -ENODEV vs 0. Guess that's what
> we should be thinking about. Seems like there some details that
> need to be sorted out, and some verification that consumers aren't
> broken by outputting smaller notes -- e.g., ia64 makes me
> wonder that.
I agree.
>
> Maybe we should leave this for another day, and have tm_spr_active
> return 0 instead of -ENODEV when the machine doesn't have the hardware,
> or not install that hook at all. Seems like the effect will be the same,
> as the note isn't output if ->get fails.
Agree. Active hooks which return 0 in case of -ENODEV sounds good to me and shall
incorporate this in the next version.
>
>> void *data = kmalloc(size, GFP_KERNEL);
>> if (unlikely(!data))
>> return 0;
>>
>>>
>>>> Now coming to the installation of the .active hooks part for all the new regsets, it
>>>> should be pretty straight forward as well. Though its optional and used for elf_core_dump
>>>> purpose only, its worth adding them here. Example of an active function should be something
>>>> like this. The function is inexpensive as required.
>>>>
>>>> +static int tm_spr_active(struct task_struct *target,
>>>> + const struct user_regset *regset)
>>>> +{
>>>> + if (!cpu_has_feature(CPU_FTR_TM))
>>>> + return -ENODEV;
>>>
>>> ... unfortunately this will do the wrong thing.
>>
>> I am not sure whether I understand this correctly. Are you saying that its wrong to return
>> -ENODEV in this case as above ?
>
> No, sorry for not being clear. The (...)'s were connected:
>
> "do you plan on fixing this? Otherwise ... ... unfortunately
> this will do the wrong thing."
>
:)
^ permalink raw reply
* Re: [PATCH RFC v13 1/5] dmaengine: fix comment typo
From: Vinod Koul @ 2014-05-22 5:07 UTC (permalink / raw)
To: Alexander Popov
Cc: Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
linuxppc-dev
In-Reply-To: <1400163335-29853-2-git-send-email-a13xp0p0v88@gmail.com>
On Thu, May 15, 2014 at 06:15:31PM +0400, Alexander Popov wrote:
> Fix comment typo.
Applied, thanks
--
~Vinod
^ permalink raw reply
* Re: [PATCH RFC v13 2/5] dma: mpc512x: add support for peripheral transfers
From: Vinod Koul @ 2014-05-22 5:08 UTC (permalink / raw)
To: Alexander Popov
Cc: Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
linuxppc-dev
In-Reply-To: <1400163335-29853-3-git-send-email-a13xp0p0v88@gmail.com>
On Thu, May 15, 2014 at 06:15:32PM +0400, Alexander Popov wrote:
> Introduce support for slave s/g transfer preparation and the associated
> device control callback in the MPC512x DMA controller driver, which adds
> support for data transfers between memory and peripheral I/O to the
> previously supported mem-to-mem transfers.
Applied, after fixing sybstem name to "dmaengine"
--
~Vinod
^ permalink raw reply
* Re: [PATCH RFC v13 0/5] MPC512x DMA slave s/g support, OF DMA lookup
From: Vinod Koul @ 2014-05-22 5:10 UTC (permalink / raw)
To: Alexander Popov
Cc: devicetree, Lars-Peter Clausen, Arnd Bergmann, Gerhard Sittig,
Andy Shevchenko, dmaengine, Dan Williams, Anatolij Gustschin,
linuxppc-dev
In-Reply-To: <1400163335-29853-1-git-send-email-a13xp0p0v88@gmail.com>
On Thu, May 15, 2014 at 06:15:30PM +0400, Alexander Popov wrote:
> 2013/7/14 Gerhard Sittig <gsi@denx.de>:
> > this series
> > - introduces slave s/g support (that's support for DMA transfers which
> > involve peripherals in contrast to mem-to-mem transfers)
> > - adds device tree based lookup support for DMA channels
> > - combines floating patches and related feedback which already covered
> > several aspects of what the suggested LPB driver needs, to demonstrate
> > how integration might be done
>
> ...
>
> Changes in v12:
> A new patch (part 2/7) is added to this series.
> Part 6/7:
> - change the description of 'compatible' property according part 2/7;
> - improve the document according Gerhard's feedback;
>
> Parts 1/7, 2/7 and 4/7 have been applied by Vinod Koul and
> are excluded from v13.
>
> Changes in v13:
> A new patch (part 1/5) is added to this series.
> Part 2/5:
> - fix style issue;
> - improve comments;
You need to cc DT- list for 3 to 5 patches. Need ack before we can apply.
I suspect 4/5 should come first out of these 3.
Also in future pls use right subsystem name "dmaengine". Also why are these
tagged as RFC still?
--
~Vinod
^ permalink raw reply
* Re: [PATCH V2 2/3] powerpc, ptrace: Enable support for transactional memory register sets
From: Michael Ellerman @ 2014-05-22 5:30 UTC (permalink / raw)
To: Pedro Alves
Cc: mikey, avagin, Roland McGrath, oleg, linux-kernel, linuxppc-dev,
Anshuman Khandual
In-Reply-To: <5372547D.8040209@redhat.com>
On Tue, 2014-05-13 at 18:21 +0100, Pedro Alves wrote:
> I wonder whether people are getting Roland's address from?
>
> It's frequent that ptrace related patches end up CCed to
> roland@redhat.com, but, he's not been at Red Hat for a few years
> now. Roland, do you still want to be CCed on ptrace-related
> issues? If so, there's probably a script somewhere in the
> kernel that needs updating. If not, well, it'd be good
> if it were updated anyway. :-)
In MAINTAINERS I see:
PTRACE SUPPORT
M: Roland McGrath <roland@redhat.com>
M: Oleg Nesterov <oleg@redhat.com>
S: Maintained
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: fix typo 'CONFIG_PMAC'
From: Paul Bolle @ 2014-05-22 7:10 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <874n0j3yjf.fsf@igel.home>
Andreas,
On Wed, 2014-05-21 at 18:08 +0200, Andreas Schwab wrote:
> Paul Bolle <pebolle@tiscali.nl> writes:
>
> > Commit b0d278b7d3ae ("powerpc/perf_event: Reduce latency of calling
> > perf_event_do_pending") added a check for CONFIG_PMAC were a check for
> > CONFIG_PPC_PMAC was clearly intended.
> >
> > Fixes: b0d278b7d3ae ("powerpc/perf_event: Reduce latency of calling perf_event_do_pending")
> > Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> > ---
> > Untested. Needs testing on 32 bit powermac, I guess.
> >
> > This typo was introduced in v2.6.36. No one noticed because very few
> > people still use 32 bit powermacs?
>
> How does that bug manifest itself?
Do you want to know how to test this patch on a 32 bit powermac? Ie, see
if it has any effect, and whether that effect improves things or make
things worse.
I have no clue how to do that, sorry. Perhaps someone else has.
Paul Bolle
^ permalink raw reply
* Re: [PATCH v5 3/4] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-22 8:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: aik, Alexander Graf, kvm-ppc, Gavin Shan, alex.williamson,
qiudayu, linuxppc-dev
In-Reply-To: <1400709391.29150.24.camel@pasglop>
On Thu, May 22, 2014 at 07:56:31AM +1000, Benjamin Herrenschmidt wrote:
>On Wed, 2014-05-21 at 15:07 +0200, Alexander Graf wrote:
>
>> > +#ifdef CONFIG_VFIO_PCI_EEH
>> > +int eeh_vfio_open(struct pci_dev *pdev)
>>
>> Why vfio? Also that config option will not be set if vfio is compiled as
>> a module.
>>
>> > +{
>> > + struct eeh_dev *edev;
>> > +
>> > + /* No PCI device ? */
>> > + if (!pdev)
>> > + return -ENODEV;
>> > +
>> > + /* No EEH device ? */
>> > + edev = pci_dev_to_eeh_dev(pdev);
>> > + if (!edev || !edev->pe)
>> > + return -ENODEV;
>> > +
>> > + eeh_dev_set_passed(edev, true);
>> > + eeh_pe_set_passed(edev->pe, true);
>> > +
>> > + return 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(eeh_vfio_open);
>
>Additionally, shouldn't we have some locking here ? (and in release too)
>
>I don't like relying on the caller locking (if it does it at all).
>
Ok. I'll add one mutex for open() and release() in next revision.
Thanks for the comment.
>> > + /* Device existing ? */
>> > + ret = eeh_vfio_check_dev(pdev, &edev, &pe);
>> > + if (ret) {
>> > + pr_debug("%s: Cannot find device %s\n",
>> > + __func__, pdev ? pci_name(pdev) : "NULL");
>> > + *retval = -7;
>>
>> What are these? Please use proper kernel internal return values for
>> errors. I don't want to see anything even remotely tied to RTAS in any
>> of these patches.
>
>Hint: -ENODEV
>
In next revision, Those exported functions will have return value as:
>= 0: carrried information to caller.
< 0: error number.
Thanks,
Gavin
^ permalink raw reply
* [PATCH v6 0/3] EEH Support for VFIO PCI device
From: Gavin Shan @ 2014-05-22 8:23 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
The series of patches intends to support EEH for PCI devices, which are
passed through to PowerKVM based guest via VFIO. The implementation is
straightforward based on the issues or problems we have to resolve to
support EEH for PowerKVM based guest.
- Emulation for EEH RTAS requests. All EEH RTAS requests goes to QEMU firstly.
If QEMU can't handle it, the request will be sent to host via newly introduced
VFIO container IOCTL command (VFIO_EEH_OP) and gets handled in host kernel.
The series of patches requires corresponding QEMU changes.
Change log
==========
v1 -> v2:
* EEH RTAS requests are routed to QEMU, and then possiblly to host kerenl.
The mechanism KVM in-kernel handling is dropped.
* Error injection is reimplemented based syscall, instead of KVM in-kerenl
handling. The logic for error injection token management is moved to
QEMU. The error injection request is routed to QEMU and then possiblly
to host kernel.
v2 -> v3:
* Make the fields in struct eeh_vfio_pci_addr, struct vfio_eeh_info based
on the comments from Alexey.
* Define macros for EEH VFIO operations (Alexey).
* Clear frozen state after successful PE reset.
* Merge original [PATCH 1/2/3] to one.
v3 -> v4:
* Remove the error injection from the patchset. Mike or I will work on that
later.
* Rename CONFIG_VFIO_EEH to VFIO_PCI_EEH.
* Rename the IOCTL command to VFIO_EEH_OP and it's handled by VFIO-PCI device
instead of VFIO container.
* Rename the IOCTL argument structure to "vfio_eeh_op" accordingly. Also, more
fields added to hold return values for RTAS requests.
* The address mapping stuff is totally removed. When opening or releasing VFIO
PCI device, notification sent to EEH to update the flags indicates the device
is passed to guest or not.
* Change pr_warn() to pr_debug() to avoid DOS as pointed by Alex.W
* Argument size check issue pointed by Alex.W.
v4 -> v5:
* Functions for VFIO PCI EEH support are moved to eeh.c and exported from there.
VFIO PCI driver just uses those functions to tackle IOCTL command VFIO_EEH_OP.
All of this is to make the code organized in a good way as suggested by Alex.G.
Another potential benefit is PowerNV/pSeries are sharing "eeh_ops" and same
infrastructure could possiblly work for KVM_PR and KVM_HV mode at the same time.
* Don't clear error injection registers after finishing PE reset as the patchset
is doing nothing related to error injection.
* Amending Documentation/vfio.txt, which was missed in last revision.
* No QEMU changes for this revision. "v4" works well. Also, remove "RFC" from the
subject as the design is basically recognized.
v5 -> v6:
* CONFIG_VFIO_PCI_EEH removed. Instead to use CONFIG_EEH.
* Split one ioctl command to 5.
* In eeh.c, description has been added for those exported functions. Also, the
functions have negative return values for error and information with other values.
All digital numbers have been replaced by macros defined in eeh.h. The comments,
including the function names have been amended not to mention "guest" or "vfio".
* Add one mutex to protect flag in eeh_dev_open()/release().
* More information on how to use those ioctl commands to Documentation/vfio.txt.
---
Gavin Shan (3):
powerpc/eeh: Flags for passed device and PE
drivers/vfio: EEH support for VFIO PCI device
powerpc/eeh: Avoid event on passed PE
Documentation/vfio.txt | 88 +++++++-
arch/powerpc/include/asm/eeh.h | 49 +++++
arch/powerpc/kernel/eeh.c | 329 ++++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/eeh-ioda.c | 3 +-
drivers/vfio/pci/vfio_pci.c | 131 +++++++++++-
include/uapi/linux/vfio.h | 53 +++++
6 files changed, 645 insertions(+), 8 deletions(-)
--
1.8.3.2
^ permalink raw reply
* [PATCH v6 1/3] powerpc/eeh: Flags for passed device and PE
From: Gavin Shan @ 2014-05-22 8:23 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-1-git-send-email-gwshan@linux.vnet.ibm.com>
The patch introduces new flags for EEH device and PE to indicate
that the device or PE has been passed through to somebody else.
In turn, we will deliver EEH errors to the device's owner for
further handling, which will be done in subsequent patches.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/eeh.h | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 7782056..34a2d83 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -72,6 +72,7 @@ struct device_node;
#define EEH_PE_RESET (1 << 2) /* PE reset in progress */
#define EEH_PE_KEEP (1 << 8) /* Keep PE on hotplug */
+#define EEH_PE_PASSTHROUGH (1 << 9) /* PE owned by guest */
struct eeh_pe {
int type; /* PE type: PHB/Bus/Device */
@@ -93,6 +94,21 @@ struct eeh_pe {
#define eeh_pe_for_each_dev(pe, edev, tmp) \
list_for_each_entry_safe(edev, tmp, &pe->edevs, list)
+static inline bool eeh_pe_passed(struct eeh_pe *pe)
+{
+ return pe ? !!(pe->state & EEH_PE_PASSTHROUGH) : false;
+}
+
+static inline void eeh_pe_set_passed(struct eeh_pe *pe, bool passed)
+{
+ if (pe) {
+ if (passed)
+ pe->state |= EEH_PE_PASSTHROUGH;
+ else
+ pe->state &= ~EEH_PE_PASSTHROUGH;
+ }
+}
+
/*
* The struct is used to trace EEH state for the associated
* PCI device node or PCI device. In future, it might
@@ -110,6 +126,7 @@ struct eeh_pe {
#define EEH_DEV_SYSFS (1 << 9) /* Sysfs created */
#define EEH_DEV_REMOVED (1 << 10) /* Removed permanently */
#define EEH_DEV_FRESET (1 << 11) /* Fundamental reset */
+#define EEH_DEV_PASSTHROUGH (1 << 12) /* Owned by guest */
struct eeh_dev {
int mode; /* EEH mode */
@@ -138,6 +155,21 @@ static inline struct pci_dev *eeh_dev_to_pci_dev(struct eeh_dev *edev)
return edev ? edev->pdev : NULL;
}
+static inline bool eeh_dev_passed(struct eeh_dev *dev)
+{
+ return dev ? !!(dev->mode & EEH_DEV_PASSTHROUGH) : false;
+}
+
+static inline void eeh_dev_set_passed(struct eeh_dev *dev, bool passed)
+{
+ if (dev) {
+ if (passed)
+ dev->mode |= EEH_DEV_PASSTHROUGH;
+ else
+ dev->mode &= ~EEH_DEV_PASSTHROUGH;
+ }
+}
+
/* Return values from eeh_ops::next_error */
enum {
EEH_NEXT_ERR_NONE = 0,
--
1.8.3.2
^ permalink raw reply related
* [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-22 8:23 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-1-git-send-email-gwshan@linux.vnet.ibm.com>
The patch adds new IOCTL commands for VFIO PCI device to support
EEH functionality for PCI devices, which have been passed through
from host to somebody else via VFIO.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
Documentation/vfio.txt | 88 ++++++++++-
arch/powerpc/include/asm/eeh.h | 17 +++
arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++++++++
drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
include/uapi/linux/vfio.h | 53 +++++++
5 files changed, 603 insertions(+), 7 deletions(-)
diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
index b9ca023..dd13db6 100644
--- a/Documentation/vfio.txt
+++ b/Documentation/vfio.txt
@@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
an excellent performance which has limitations such as inability to do
locked pages accounting in real time.
-So 3 additional ioctls have been added:
+4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
+which works on the basis of additional ioctl commands.
+
+So 8 additional ioctls have been added:
VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
of the DMA window on the PCI bus.
@@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
VFIO_IOMMU_DISABLE - disables the container.
+ VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on the
+ specified device. Also, it can be used to remove IO or DMA
+ stopped state on the frozen PE.
+
+ VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified
+ PE or query PE sharing mode.
+
+ VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state.
+
+ VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps for
+ error recovering.
+
+ VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. It's
+ one of the major steps for error recoverying.
The code flow from the example above should be slightly changed:
@@ -346,6 +363,75 @@ The code flow from the example above should be slightly changed:
ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
.....
+Based on the initial example we have, the following piece of code could be
+reference for EEH setup and error handling:
+
+ struct vfio_eeh_pe_set_option option = { .argsz = sizeof(option) };
+ struct vfio_eeh_pe_get_addr addr = { .argsz = sizeof(addr) };
+ struct vfio_eeh_pe_get_state state = { .argsz = sizeof(state) };
+ struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
+ struct vfio_eeh_pe_configure config = { .argsz = sizeof(config) };
+
+ ....
+
+ /* Get a file descriptor for the device */
+ device = ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
+
+ /* Enable the EEH functionality on the device */
+ option.option = EEH_OPT_ENABLE;
+ ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
+
+ /* Retrieve PE address and create and maintain PE by yourself */
+ addr.option = EEH_OPT_GET_PE_ADDR;
+ ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
+
+ /* Assure EEH is supported on the PE and make PE functional */
+ ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
+
+ /* Save device's state. pci_save_state() would be good enough
+ * as an example.
+ */
+
+ /* Test and setup the device */
+ ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
+
+ ....
+
+ /* When 0xFF's returned from reading PCI config space or IO BARs
+ * of the PCI device. Check the PE state to see if that has been
+ * frozen.
+ */
+ ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
+
+ /* Waiting for pending PCI transactions to be completed and don't
+ * produce any more PCI traffic from/to the affected PE until
+ * recovery is finished.
+ */
+
+ /* Enable IO for the affected PE and collect logs. Usually, the
+ * standard part of PCI config space, AER registers are dumped
+ * as logs for further analysis.
+ */
+ option.option = EEH_OPT_THAW_MMIO;
+ ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
+
+ /* Issue PE reset */
+ reset.option = EEH_RESET_HOT;
+ ioctl(device, VFIO_EEH_PE_RESET, &reset);
+
+ /* Configure the PCI bridges for the affected PE */
+ ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
+
+ /* Restored state we saved at initialization time. pci_restore_state()
+ * is good enough as an example.
+ */
+
+ /* Hopefully, error is recovered successfully. Now, you can resume to
+ * start PCI traffic to/from the affected PE.
+ */
+
+ ....
+
-------------------------------------------------------------------------------
[1] VFIO was originally an acronym for "Virtual Function I/O" in its
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 34a2d83..dd5f1cf 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -191,6 +191,11 @@ enum {
#define EEH_OPT_ENABLE 1 /* EEH enable */
#define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
#define EEH_OPT_THAW_DMA 3 /* DMA enable */
+#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
+#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
+#define EEH_PE_MODE_NONE 0 /* Invalid mode */
+#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
+#define EEH_PE_MODE_SHARED 2 /* Shared mode */
#define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
#define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
#define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
@@ -198,6 +203,11 @@ enum {
#define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
#define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
#define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
+#define EEH_PE_STATE_NORMAL 0 /* Normal state */
+#define EEH_PE_STATE_RESET 1 /* PE reset */
+#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
+#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
+#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
#define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
#define EEH_RESET_HOT 1 /* Hot reset */
#define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
@@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
void eeh_add_device_tree_late(struct pci_bus *);
void eeh_add_sysfs_files(struct pci_bus *);
void eeh_remove_device(struct pci_dev *);
+int eeh_dev_open(struct pci_dev *pdev);
+void eeh_dev_release(struct pci_dev *pdev);
+int eeh_pe_set_option(struct pci_dev *pdev, int option);
+int eeh_pe_get_addr(struct pci_dev *pdev, int option);
+int eeh_pe_get_state(struct pci_dev *pdev);
+int eeh_pe_reset(struct pci_dev *pdev, int option);
+int eeh_pe_configure(struct pci_dev *pdev);
/**
* EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 9c6b899..b90a474 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops = NULL;
/* Lock to avoid races due to multiple reports of an error */
DEFINE_RAW_SPINLOCK(confirm_error_lock);
+/* Lock to protect passed flags */
+static DEFINE_MUTEX(eeh_dev_mutex);
+
/* Buffer for reporting pci register dumps. Its here in BSS, and
* not dynamically alloced, so that it ends up in RMO where RTAS
* can access it.
@@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
edev->mode &= ~EEH_DEV_SYSFS;
}
+/**
+ * eeh_dev_open - Mark EEH device and PE as passed through
+ * @pdev: PCI device
+ *
+ * Mark the indicated EEH device and PE as passed through.
+ * In the result, the EEH errors detected on the PE won't be
+ * reported. The owner of the device will be responsible for
+ * detection and recovery.
+ */
+int eeh_dev_open(struct pci_dev *pdev)
+{
+ struct eeh_dev *edev;
+
+ mutex_lock(&eeh_dev_mutex);
+
+ /* No PCI device ? */
+ if (!pdev) {
+ mutex_unlock(&eeh_dev_mutex);
+ return -ENODEV;
+ }
+
+ /* No EEH device ? */
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !edev->pe) {
+ mutex_unlock(&eeh_dev_mutex);
+ return -ENODEV;
+ }
+
+ eeh_dev_set_passed(edev, true);
+ eeh_pe_set_passed(edev->pe, true);
+ mutex_unlock(&eeh_dev_mutex);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(eeh_dev_open);
+
+/**
+ * eeh_dev_release - Reclaim the ownership of EEH device
+ * @pdev: PCI device
+ *
+ * Reclaim ownership of EEH device, potentially the corresponding
+ * PE. In the result, the EEH errors detected on the PE will be
+ * reported and handled as usual.
+ */
+void eeh_dev_release(struct pci_dev *pdev)
+{
+ bool release_pe = true;
+ struct eeh_pe *pe = NULL;
+ struct eeh_dev *tmp, *edev;
+
+ mutex_lock(&eeh_dev_mutex);
+
+ /* No PCI device ? */
+ if (!pdev) {
+ mutex_unlock(&eeh_dev_mutex);
+ return;
+ }
+
+ /* No EEH device ? */
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !eeh_dev_passed(edev) ||
+ !edev->pe || !eeh_pe_passed(pe)) {
+ mutex_unlock(&eeh_dev_mutex);
+ return;
+ }
+
+ /* Release device */
+ pe = edev->pe;
+ eeh_dev_set_passed(edev, false);
+
+ /* Release PE */
+ eeh_pe_for_each_dev(pe, edev, tmp) {
+ if (eeh_dev_passed(edev)) {
+ release_pe = false;
+ break;
+ }
+ }
+
+ if (release_pe)
+ eeh_pe_set_passed(pe, false);
+
+ mutex_unlock(&eeh_dev_mutex);
+}
+EXPORT_SYMBOL(eeh_dev_release);
+
+static int eeh_dev_check(struct pci_dev *pdev,
+ struct eeh_dev **pedev,
+ struct eeh_pe **ppe)
+{
+ struct eeh_dev *edev;
+
+ /* No device ? */
+ if (!pdev)
+ return -ENODEV;
+
+ edev = pci_dev_to_eeh_dev(pdev);
+ if (!edev || !eeh_dev_passed(edev) ||
+ !edev->pe || !eeh_pe_passed(edev->pe))
+ return -ENODEV;
+
+ if (pedev)
+ *pedev = edev;
+ if (ppe)
+ *ppe = edev->pe;
+
+ return 0;
+}
+
+/**
+ * eeh_pe_set_option - Set options for the indicated PE
+ * @pdev: PCI device
+ * @option: requested option
+ *
+ * The routine is called to enable or disable EEH functionality
+ * on the indicated PE, to enable IO or DMA for the frozen PE.
+ */
+int eeh_pe_set_option(struct pci_dev *pdev, int option)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = eeh_dev_check(pdev, &edev, &pe);
+ if (ret)
+ return ret;
+
+ switch (option) {
+ case EEH_OPT_DISABLE:
+ case EEH_OPT_ENABLE:
+ break;
+ case EEH_OPT_THAW_MMIO:
+ case EEH_OPT_THAW_DMA:
+ if (!eeh_ops || !eeh_ops->set_option) {
+ ret = -ENOENT;
+ break;
+ }
+
+ ret = eeh_ops->set_option(pe, option);
+ break;
+ default:
+ pr_debug("%s: Option %d out of range (%d, %d)\n",
+ __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_pe_set_option);
+
+/**
+ * eeh_pe_get_addr - Retrieve the PE address or sharing mode
+ * @pdev: PCI device
+ * @option: option
+ *
+ * Retrieve the PE address or sharing mode.
+ */
+int eeh_pe_get_addr(struct pci_dev *pdev, int option)
+{
+ struct pci_bus *bus;
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = eeh_dev_check(pdev, &edev, &pe);
+ if (ret)
+ return ret;
+
+ switch (option) {
+ case EEH_OPT_GET_PE_ADDR:
+ bus = eeh_pe_bus_get(pe);
+ if (!bus) {
+ ret = -ENODEV;
+ break;
+ }
+
+ /* PE address has format "00BBSS00" */
+ ret = bus->number << 16;
+ break;
+ case EEH_OPT_GET_PE_MODE:
+ /* Wa always have shared PE */
+ ret = EEH_PE_MODE_SHARED;
+ break;
+ default:
+ pr_debug("%s: option %d out of range (0, 1)\n",
+ __func__, option);
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
+
+/**
+ * eeh_pe_get_state - Retrieve PE's state
+ * @pdev: PCI device
+ *
+ * Retrieve the PE's state, which includes 3 aspects: enabled
+ * DMA, enabled IO and asserted reset.
+ */
+int eeh_pe_get_state(struct pci_dev *pdev)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ int result, ret = 0;
+
+ /* Device existing ? */
+ ret = eeh_dev_check(pdev, &edev, &pe);
+ if (ret)
+ return ret;
+
+ if (!eeh_ops || !eeh_ops->get_state)
+ return -ENOENT;
+
+ result = eeh_ops->get_state(pe, NULL);
+ if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ (result & EEH_STATE_DMA_ENABLED) &&
+ (result & EEH_STATE_MMIO_ENABLED))
+ ret = EEH_PE_STATE_NORMAL;
+ else if (result & EEH_STATE_RESET_ACTIVE)
+ ret = EEH_PE_STATE_RESET;
+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ !(result & EEH_STATE_DMA_ENABLED) &&
+ !(result & EEH_STATE_MMIO_ENABLED))
+ ret = EEH_PE_STATE_STOPPED_IO_DMA;
+ else if (!(result & EEH_STATE_RESET_ACTIVE) &&
+ (result & EEH_STATE_DMA_ENABLED) &&
+ !(result & EEH_STATE_MMIO_ENABLED))
+ ret = EEH_PE_STATE_STOPPED_DMA;
+ else
+ ret = EEH_PE_STATE_UNAVAIL;
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_pe_get_state);
+
+/**
+ * eeh_pe_reset - Issue PE reset according to specified type
+ * @pdev: PCI device
+ * @option: reset type
+ *
+ * The routine is called to reset the specified PE with the
+ * indicated type, either fundamental reset or hot reset.
+ * PE reset is the most important part for error recovery.
+ */
+int eeh_pe_reset(struct pci_dev *pdev, int option)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = eeh_dev_check(pdev, &edev, &pe);
+ if (ret)
+ return ret;
+
+ if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
+ return -ENOENT;
+
+ switch (option) {
+ case EEH_RESET_DEACTIVATE:
+ ret = eeh_ops->reset(pe, option);
+ if (ret)
+ break;
+
+ /*
+ * 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 recursive frozen PE.
+ */
+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
+ if (!ret)
+ ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
+ if (!ret)
+ eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
+ break;
+ case EEH_RESET_HOT:
+ case EEH_RESET_FUNDAMENTAL:
+ ret = eeh_ops->reset(pe, option);
+ break;
+ default:
+ pr_debug("%s: Unsupported option %d\n",
+ __func__, option);
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_pe_reset);
+
+/**
+ * eeh_pe_configure - Configure PCI bridges after PE reset
+ * @pdev: PCI device
+ *
+ * The routine is called to restore the PCI config space for
+ * those PCI devices, especially PCI bridges affected by PE
+ * reset issued previously.
+ */
+int eeh_pe_configure(struct pci_dev *pdev)
+{
+ struct eeh_dev *edev;
+ struct eeh_pe *pe;
+ int ret = 0;
+
+ /* Device existing ? */
+ ret = eeh_dev_check(pdev, &edev, &pe);
+ if (ret)
+ return ret;
+
+ /* Restore config space for the affected devices */
+ eeh_pe_restore_bars(pe);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(eeh_pe_configure);
+
static int proc_eeh_show(struct seq_file *m, void *v)
{
if (!eeh_enabled()) {
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 7ba0424..301ac18 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -25,6 +25,9 @@
#include <linux/types.h>
#include <linux/uaccess.h>
#include <linux/vfio.h>
+#ifdef CONFIG_EEH
+#include <asm/eeh.h>
+#endif
#include "vfio_pci_private.h"
@@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
pci_restore_state(pdev);
}
+static void vfio_eeh_pci_release(struct pci_dev *pdev)
+{
+#ifdef CONFIG_EEH
+ eeh_dev_release(pdev);
+#endif
+}
+
static void vfio_pci_release(void *device_data)
{
struct vfio_pci_device *vdev = device_data;
- if (atomic_dec_and_test(&vdev->refcnt))
+ if (atomic_dec_and_test(&vdev->refcnt)) {
+ vfio_eeh_pci_release(vdev->pdev);
vfio_pci_disable(vdev);
+ }
module_put(THIS_MODULE);
}
+static int vfio_eeh_pci_open(struct pci_dev *pdev)
+{
+ int ret = 0;
+
+#ifdef CONFIG_EEH
+ ret = eeh_dev_open(pdev);
+#endif
+ return ret;
+}
+
static int vfio_pci_open(void *device_data)
{
struct vfio_pci_device *vdev = device_data;
+ int ret;
if (!try_module_get(THIS_MODULE))
return -ENODEV;
if (atomic_inc_return(&vdev->refcnt) == 1) {
- int ret = vfio_pci_enable(vdev);
- if (ret) {
- module_put(THIS_MODULE);
- return ret;
- }
+ ret = vfio_pci_enable(vdev);
+ if (ret)
+ goto error;
+
+ ret = vfio_eeh_pci_open(vdev->pdev);
+ if (ret)
+ goto error;
}
return 0;
+error:
+ module_put(THIS_MODULE);
+ return ret;
}
static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
@@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
return walk.ret;
}
+static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
+ unsigned int cmd,
+ unsigned long arg)
+{
+ unsigned long minsz;
+ int ret = 0;
+
+#ifdef CONFIG_EEH
+ switch (cmd) {
+ case VFIO_EEH_PE_SET_OPTION: {
+ struct vfio_eeh_pe_set_option option;
+
+ minsz = offsetofend(struct vfio_eeh_pe_set_option, option);
+ if (copy_from_user(&option, (void __user *)arg, minsz))
+ return -EFAULT;
+ if (option.argsz < minsz)
+ return -EINVAL;
+
+ ret = eeh_pe_set_option(pdev, option.option);
+ break;
+ }
+ case VFIO_EEH_PE_GET_ADDR: {
+ struct vfio_eeh_pe_get_addr addr;
+
+ minsz = offsetofend(struct vfio_eeh_pe_get_addr, info);
+ if (copy_from_user(&addr, (void __user *)arg, minsz))
+ return -EFAULT;
+ if (addr.argsz < minsz)
+ return -EINVAL;
+
+ ret = eeh_pe_get_addr(pdev, addr.option);
+ if (ret >= 0) {
+ addr.info = ret;
+ if (copy_to_user((void __user *)arg, &addr, minsz))
+ return -EFAULT;
+ ret = 0;
+ }
+
+ break;
+ }
+ case VFIO_EEH_PE_GET_STATE: {
+ struct vfio_eeh_pe_get_state state;
+
+ minsz = offsetofend(struct vfio_eeh_pe_get_state, state);
+ if (copy_from_user(&state, (void __user *)arg, minsz))
+ return -EFAULT;
+ if (state.argsz < minsz)
+ return -EINVAL;
+
+ ret = eeh_pe_get_state(pdev);
+ if (ret >= 0) {
+ state.state = ret;
+ if (copy_to_user((void __user *)arg, &state, minsz))
+ return -EFAULT;
+ ret = 0;
+ }
+ break;
+ }
+ case VFIO_EEH_PE_RESET: {
+ struct vfio_eeh_pe_reset reset;
+
+ minsz = offsetofend(struct vfio_eeh_pe_reset, option);
+ if (copy_from_user(&reset, (void __user *)arg, minsz))
+ return -EFAULT;
+ if (reset.argsz < minsz)
+ return -EINVAL;
+
+ ret = eeh_pe_reset(pdev, reset.option);
+ break;
+ }
+ case VFIO_EEH_PE_CONFIGURE:
+ ret = eeh_pe_configure(pdev);
+ break;
+ default:
+ ret = -EINVAL;
+ pr_debug("%s: Cannot handle command %d\n",
+ __func__, cmd);
+ }
+#else
+ ret = -ENOENT;
+#endif
+
+ return ret;
+}
+
static long vfio_pci_ioctl(void *device_data,
unsigned int cmd, unsigned long arg)
{
@@ -682,6 +795,12 @@ hot_reset_release:
kfree(groups);
return ret;
+ } else if (cmd == VFIO_EEH_PE_SET_OPTION ||
+ cmd == VFIO_EEH_PE_GET_ADDR ||
+ cmd == VFIO_EEH_PE_GET_STATE ||
+ cmd == VFIO_EEH_PE_RESET ||
+ cmd == VFIO_EEH_PE_CONFIGURE) {
+ return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
}
return -ENOTTY;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index cb9023d..ef55682 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
+/*
+ * EEH functionality can be enabled or disabled on one specific device.
+ * Also, the DMA or IO frozen state can be removed from the frozen PE
+ * if required.
+ */
+struct vfio_eeh_pe_set_option {
+ __u32 argsz;
+ __u32 option;
+};
+
+#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
+
+/*
+ * Each EEH PE should have unique address to be identified. The command
+ * helps to retrieve the address and the sharing mode of the PE.
+ */
+struct vfio_eeh_pe_get_addr {
+ __u32 argsz;
+ __u32 option;
+ __u32 info;
+};
+
+#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
+
+/*
+ * EEH PE might have been frozen because of PCI errors. Also, it might
+ * be experiencing reset for error revoery. The following command helps
+ * to get the state.
+ */
+struct vfio_eeh_pe_get_state {
+ __u32 argsz;
+ __u32 state;
+};
+
+#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
+
+/*
+ * Reset is the major step to recover problematic PE. The following
+ * command helps on that.
+ */
+struct vfio_eeh_pe_reset {
+ __u32 argsz;
+ __u32 option;
+};
+
+#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
+
+/*
+ * One of the steps for recovery after PE reset is to configure the
+ * PCI bridges affected by the PE reset.
+ */
+#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
+
/* ***************************************************************** */
#endif /* _UAPIVFIO_H */
--
1.8.3.2
^ permalink raw reply related
* [PATCH v6 3/3] powerpc/eeh: Avoid event on passed PE
From: Gavin Shan @ 2014-05-22 8:23 UTC (permalink / raw)
To: kvm-ppc; +Cc: aik, agraf, Gavin Shan, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-1-git-send-email-gwshan@linux.vnet.ibm.com>
If we detects frozen state on PE that has been passed through to somebody
else. we needn't handle it. Instead, we rely on the device's owner to
detect and recover it. The patch avoid EEH event on the frozen passed PE so
that the device's owner can have chance to handle that.
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
arch/powerpc/kernel/eeh.c | 8 ++++++++
arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index b90a474..aee6cc5 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -403,6 +403,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
if (ret > 0)
return ret;
+ /*
+ * If the PE isn't owned by us, we shouldn't check the
+ * state. Instead, let the owner handle it if the PE has
+ * been frozen.
+ */
+ if (eeh_pe_passed(pe))
+ return 0;
+
/* If we already have a pending isolation event for this
* slot, we know it's bad already, we don't need to check.
* Do this checking under a lock; as multiple PCI devices
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index 1b5982f..03a3ed2 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
ret = EEH_NEXT_ERR_NONE;
- } else if ((*pe)->state & EEH_PE_ISOLATED) {
+ } else if ((*pe)->state & EEH_PE_ISOLATED ||
+ eeh_pe_passed(*pe)) {
ret = EEH_NEXT_ERR_NONE;
} else {
pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH v4 09/11] powerpc/perf: add support for the hv 24x7 interface
From: Ian Munsie @ 2014-05-22 8:19 UTC (permalink / raw)
To: Cody P Schafer
Cc: Stephen Rothwell, Peter Zijlstra, LKML, Michael Ellerman,
Ingo Molnar, Paul Mackerras, Arnaldo Carvalho de Melo, scottwood,
linuxppc-dev
In-Reply-To: <1394064082-6242-10-git-send-email-cody@linux.vnet.ibm.com>
Hi Cody,
I just tried building this with gcc 4.5, which failed with the following
warning (treated as an error):
cc1: warnings being treated as errors
arch/powerpc/perf/hv-24x7.c: In function 'single_24x7_request':
arch/powerpc/perf/hv-24x7.c:346:1: error: the frame size of 8192 bytes is larger than 2048 bytes
make[3]: *** [arch/powerpc/perf/hv-24x7.o] Error 1
make[2]: *** [arch/powerpc/perf] Error 2
My .config has CONFIG_FRAME_WARN=2048 (default on 64bit), but the
alignment constraints in this function may require 8K on the stack -
possibly a bit large?
Notably for some reason this warning no longer seems to trigger on gcc
4.8 (or at least somewhere between 4.5-4.8), though the assembly does
still show it aligning the buffers.
Excerpts from Cody P Schafer's message of 2014-03-06 11:01:08 +1100:
> +static unsigned long single_24x7_request(u8 domain, u32 offset, u16 ix,
<snip>
> + /*
> + * request_buffer and result_buffer are not required to be 4k aligned,
> + * but are not allowed to cross any 4k boundary. Aligning them to 4k is
> + * the simplest way to ensure that.
> + */
> + struct reqb {
> + struct hv_24x7_request_buffer buf;
> + struct hv_24x7_request req;
> + } __packed __aligned(4096) request_buffer = {
<snip>
> + struct resb {
> + struct hv_24x7_data_result_buffer buf;
> + struct hv_24x7_result res;
> + struct hv_24x7_result_element elem;
> + __be64 result;
> + } __packed __aligned(4096) result_buffer = {};
<snip>
Cheers,
-Ian
^ permalink raw reply
* powerpc: CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS?
From: Paul Bolle @ 2014-05-22 9:09 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras; +Cc: linuxppc-dev
In v3.2 checks for CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS and
CONFIG_WSP_DD1_WORKAROUND_BAD_PCIE_CLASS were added to
arch/powerpc/platforms/wsp/wsp_pci.c. The check for
CONFIG_WSP_DD1_WORKAROUND_BAD_PCIE_CLASS was dropped again in v3.3.
Neither of these macros have ever had a matching Kconfig symbol. So they
have always evaluated to false.
Is it perhaps intended that one sets the macro still in the tree,
CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS, manually? And is that macro
still needed?
Paul Bolle
^ permalink raw reply
* Re: [PATCH v6 2/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-22 9:55 UTC (permalink / raw)
To: Gavin Shan, kvm-ppc; +Cc: aik, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-3-git-send-email-gwshan@linux.vnet.ibm.com>
On 22.05.14 10:23, Gavin Shan wrote:
> The patch adds new IOCTL commands for VFIO PCI device to support
> EEH functionality for PCI devices, which have been passed through
> from host to somebody else via VFIO.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
This already looks a *lot* more sane than the previous versions. We're
slowly getting there I think ;).
Ben, could you please check through the exported EEH interface itself
and verify whether it does all the lockings correctly, uses the API
properly and doesn't allow for a user space program to blow up the system?
> ---
> Documentation/vfio.txt | 88 ++++++++++-
> arch/powerpc/include/asm/eeh.h | 17 +++
> arch/powerpc/kernel/eeh.c | 321 +++++++++++++++++++++++++++++++++++++++++
> drivers/vfio/pci/vfio_pci.c | 131 ++++++++++++++++-
> include/uapi/linux/vfio.h | 53 +++++++
> 5 files changed, 603 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> index b9ca023..dd13db6 100644
> --- a/Documentation/vfio.txt
> +++ b/Documentation/vfio.txt
> @@ -305,7 +305,10 @@ faster, the map/unmap handling has been implemented in real mode which provides
> an excellent performance which has limitations such as inability to do
> locked pages accounting in real time.
>
> -So 3 additional ioctls have been added:
> +4) PPC64 guests detect PCI errors and recover from them via EEH RTAS services,
> +which works on the basis of additional ioctl commands.
> +
> +So 8 additional ioctls have been added:
>
> VFIO_IOMMU_SPAPR_TCE_GET_INFO - returns the size and the start
> of the DMA window on the PCI bus.
> @@ -316,6 +319,20 @@ So 3 additional ioctls have been added:
>
> VFIO_IOMMU_DISABLE - disables the container.
>
> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functinality on the
functionality
> + specified device. Also, it can be used to remove IO or DMA
> + stopped state on the frozen PE.
> +
> + VFIO_EEH_PE_GET_ADDR - retrieve the unique address of the specified
> + PE or query PE sharing mode.
What is PE?
> +
> + VFIO_EEH_PE_GET_STATE - retrieve PE's state: frozen or normal state.
> +
> + VFIO_EEH_PE_RESET - do PE reset, which is one of the major steps for
> + error recovering.
> +
> + VFIO_EEH_PE_CONFIGURE - configure the PCI bridges after PE reset. It's
> + one of the major steps for error recoverying.
>
> The code flow from the example above should be slightly changed:
>
> @@ -346,6 +363,75 @@ The code flow from the example above should be slightly changed:
> ioctl(container, VFIO_IOMMU_MAP_DMA, &dma_map);
> .....
>
> +Based on the initial example we have, the following piece of code could be
> +reference for EEH setup and error handling:
> +
> + struct vfio_eeh_pe_set_option option = { .argsz = sizeof(option) };
> + struct vfio_eeh_pe_get_addr addr = { .argsz = sizeof(addr) };
> + struct vfio_eeh_pe_get_state state = { .argsz = sizeof(state) };
> + struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
> + struct vfio_eeh_pe_configure config = { .argsz = sizeof(config) };
> +
> + ....
> +
> + /* Get a file descriptor for the device */
> + device = ioctl(group, VFIO_GROUP_GET_DEVICE_FD, "0000:06:0d.0");
> +
> + /* Enable the EEH functionality on the device */
> + option.option = EEH_OPT_ENABLE;
> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
> +
> + /* Retrieve PE address and create and maintain PE by yourself */
> + addr.option = EEH_OPT_GET_PE_ADDR;
> + ioctl(device, VFIO_EEH_PE_GET_ADDR, &addr);
> +
> + /* Assure EEH is supported on the PE and make PE functional */
> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
> +
> + /* Save device's state. pci_save_state() would be good enough
> + * as an example.
> + */
> +
> + /* Test and setup the device */
> + ioctl(device, VFIO_DEVICE_GET_INFO, &device_info);
> +
> + ....
> +
> + /* When 0xFF's returned from reading PCI config space or IO BARs
> + * of the PCI device. Check the PE state to see if that has been
> + * frozen.
> + */
> + ioctl(device, VFIO_EEH_PE_GET_STATE, &state);
> +
> + /* Waiting for pending PCI transactions to be completed and don't
> + * produce any more PCI traffic from/to the affected PE until
> + * recovery is finished.
> + */
> +
> + /* Enable IO for the affected PE and collect logs. Usually, the
> + * standard part of PCI config space, AER registers are dumped
> + * as logs for further analysis.
> + */
> + option.option = EEH_OPT_THAW_MMIO;
> + ioctl(device, VFIO_EEH_PE_SET_OPTION, &option);
> +
> + /* Issue PE reset */
> + reset.option = EEH_RESET_HOT;
> + ioctl(device, VFIO_EEH_PE_RESET, &reset);
> +
> + /* Configure the PCI bridges for the affected PE */
> + ioctl(device, VFIO_EEH_PE_CONFIGURE, NULL);
> +
> + /* Restored state we saved at initialization time. pci_restore_state()
> + * is good enough as an example.
> + */
> +
> + /* Hopefully, error is recovered successfully. Now, you can resume to
> + * start PCI traffic to/from the affected PE.
> + */
> +
> + ....
> +
> -------------------------------------------------------------------------------
>
> [1] VFIO was originally an acronym for "Virtual Function I/O" in its
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index 34a2d83..dd5f1cf 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -191,6 +191,11 @@ enum {
> #define EEH_OPT_ENABLE 1 /* EEH enable */
> #define EEH_OPT_THAW_MMIO 2 /* MMIO enable */
> #define EEH_OPT_THAW_DMA 3 /* DMA enable */
> +#define EEH_OPT_GET_PE_ADDR 0 /* Get PE addr */
> +#define EEH_OPT_GET_PE_MODE 1 /* Get PE mode */
> +#define EEH_PE_MODE_NONE 0 /* Invalid mode */
> +#define EEH_PE_MODE_NOT_SHARED 1 /* Not shared */
> +#define EEH_PE_MODE_SHARED 2 /* Shared mode */
> #define EEH_STATE_UNAVAILABLE (1 << 0) /* State unavailable */
> #define EEH_STATE_NOT_SUPPORT (1 << 1) /* EEH not supported */
> #define EEH_STATE_RESET_ACTIVE (1 << 2) /* Active reset */
> @@ -198,6 +203,11 @@ enum {
> #define EEH_STATE_DMA_ACTIVE (1 << 4) /* Active DMA */
> #define EEH_STATE_MMIO_ENABLED (1 << 5) /* MMIO enabled */
> #define EEH_STATE_DMA_ENABLED (1 << 6) /* DMA enabled */
> +#define EEH_PE_STATE_NORMAL 0 /* Normal state */
> +#define EEH_PE_STATE_RESET 1 /* PE reset */
> +#define EEH_PE_STATE_STOPPED_IO_DMA 2 /* Stopped */
> +#define EEH_PE_STATE_STOPPED_DMA 4 /* Stopped DMA */
> +#define EEH_PE_STATE_UNAVAIL 5 /* Unavailable */
> #define EEH_RESET_DEACTIVATE 0 /* Deactivate the PE reset */
> #define EEH_RESET_HOT 1 /* Hot reset */
> #define EEH_RESET_FUNDAMENTAL 3 /* Fundamental reset */
> @@ -305,6 +315,13 @@ void eeh_add_device_late(struct pci_dev *);
> void eeh_add_device_tree_late(struct pci_bus *);
> void eeh_add_sysfs_files(struct pci_bus *);
> void eeh_remove_device(struct pci_dev *);
> +int eeh_dev_open(struct pci_dev *pdev);
> +void eeh_dev_release(struct pci_dev *pdev);
> +int eeh_pe_set_option(struct pci_dev *pdev, int option);
> +int eeh_pe_get_addr(struct pci_dev *pdev, int option);
> +int eeh_pe_get_state(struct pci_dev *pdev);
> +int eeh_pe_reset(struct pci_dev *pdev, int option);
> +int eeh_pe_configure(struct pci_dev *pdev);
>
> /**
> * EEH_POSSIBLE_ERROR() -- test for possible MMIO failure.
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index 9c6b899..b90a474 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -108,6 +108,9 @@ struct eeh_ops *eeh_ops = NULL;
> /* Lock to avoid races due to multiple reports of an error */
> DEFINE_RAW_SPINLOCK(confirm_error_lock);
>
> +/* Lock to protect passed flags */
> +static DEFINE_MUTEX(eeh_dev_mutex);
> +
> /* Buffer for reporting pci register dumps. Its here in BSS, and
> * not dynamically alloced, so that it ends up in RMO where RTAS
> * can access it.
> @@ -1098,6 +1101,324 @@ void eeh_remove_device(struct pci_dev *dev)
> edev->mode &= ~EEH_DEV_SYSFS;
> }
>
> +/**
> + * eeh_dev_open - Mark EEH device and PE as passed through
> + * @pdev: PCI device
> + *
> + * Mark the indicated EEH device and PE as passed through.
> + * In the result, the EEH errors detected on the PE won't be
> + * reported. The owner of the device will be responsible for
> + * detection and recovery.
> + */
> +int eeh_dev_open(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> +
> + mutex_lock(&eeh_dev_mutex);
> +
> + /* No PCI device ? */
> + if (!pdev) {
> + mutex_unlock(&eeh_dev_mutex);
> + return -ENODEV;
> + }
> +
> + /* No EEH device ? */
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !edev->pe) {
> + mutex_unlock(&eeh_dev_mutex);
> + return -ENODEV;
> + }
> +
> + eeh_dev_set_passed(edev, true);
> + eeh_pe_set_passed(edev->pe, true);
> + mutex_unlock(&eeh_dev_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(eeh_dev_open);
> +
> +/**
> + * eeh_dev_release - Reclaim the ownership of EEH device
> + * @pdev: PCI device
> + *
> + * Reclaim ownership of EEH device, potentially the corresponding
> + * PE. In the result, the EEH errors detected on the PE will be
> + * reported and handled as usual.
> + */
> +void eeh_dev_release(struct pci_dev *pdev)
> +{
> + bool release_pe = true;
> + struct eeh_pe *pe = NULL;
> + struct eeh_dev *tmp, *edev;
> +
> + mutex_lock(&eeh_dev_mutex);
> +
> + /* No PCI device ? */
> + if (!pdev) {
> + mutex_unlock(&eeh_dev_mutex);
> + return;
> + }
> +
> + /* No EEH device ? */
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !eeh_dev_passed(edev) ||
> + !edev->pe || !eeh_pe_passed(pe)) {
> + mutex_unlock(&eeh_dev_mutex);
> + return;
> + }
> +
> + /* Release device */
> + pe = edev->pe;
> + eeh_dev_set_passed(edev, false);
> +
> + /* Release PE */
> + eeh_pe_for_each_dev(pe, edev, tmp) {
> + if (eeh_dev_passed(edev)) {
> + release_pe = false;
> + break;
> + }
> + }
> +
> + if (release_pe)
> + eeh_pe_set_passed(pe, false);
> +
> + mutex_unlock(&eeh_dev_mutex);
> +}
> +EXPORT_SYMBOL(eeh_dev_release);
> +
> +static int eeh_dev_check(struct pci_dev *pdev,
> + struct eeh_dev **pedev,
> + struct eeh_pe **ppe)
> +{
> + struct eeh_dev *edev;
> +
> + /* No device ? */
> + if (!pdev)
> + return -ENODEV;
> +
> + edev = pci_dev_to_eeh_dev(pdev);
> + if (!edev || !eeh_dev_passed(edev) ||
> + !edev->pe || !eeh_pe_passed(edev->pe))
> + return -ENODEV;
> +
> + if (pedev)
> + *pedev = edev;
> + if (ppe)
> + *ppe = edev->pe;
> +
> + return 0;
> +}
> +
> +/**
> + * eeh_pe_set_option - Set options for the indicated PE
> + * @pdev: PCI device
> + * @option: requested option
> + *
> + * The routine is called to enable or disable EEH functionality
> + * on the indicated PE, to enable IO or DMA for the frozen PE.
> + */
> +int eeh_pe_set_option(struct pci_dev *pdev, int option)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + switch (option) {
> + case EEH_OPT_DISABLE:
> + case EEH_OPT_ENABLE:
This deserves a comment
> + break;
> + case EEH_OPT_THAW_MMIO:
> + case EEH_OPT_THAW_DMA:
> + if (!eeh_ops || !eeh_ops->set_option) {
> + ret = -ENOENT;
> + break;
> + }
> +
> + ret = eeh_ops->set_option(pe, option);
> + break;
> + default:
> + pr_debug("%s: Option %d out of range (%d, %d)\n",
> + __func__, option, EEH_OPT_DISABLE, EEH_OPT_THAW_DMA);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_set_option);
> +
> +/**
> + * eeh_pe_get_addr - Retrieve the PE address or sharing mode
> + * @pdev: PCI device
> + * @option: option
> + *
> + * Retrieve the PE address or sharing mode.
> + */
> +int eeh_pe_get_addr(struct pci_dev *pdev, int option)
> +{
> + struct pci_bus *bus;
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
Probably safer to write this as ret < 0. Positive return values are
success now.
> + return ret;
> +
> + switch (option) {
> + case EEH_OPT_GET_PE_ADDR:
> + bus = eeh_pe_bus_get(pe);
> + if (!bus) {
> + ret = -ENODEV;
> + break;
> + }
> +
> + /* PE address has format "00BBSS00" */
> + ret = bus->number << 16;
> + break;
> + case EEH_OPT_GET_PE_MODE:
> + /* Wa always have shared PE */
Wa?
> + ret = EEH_PE_MODE_SHARED;
> + break;
> + default:
> + pr_debug("%s: option %d out of range (0, 1)\n",
> + __func__, option);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_get_addr);
> +
> +/**
> + * eeh_pe_get_state - Retrieve PE's state
> + * @pdev: PCI device
> + *
> + * Retrieve the PE's state, which includes 3 aspects: enabled
> + * DMA, enabled IO and asserted reset.
> + */
> +int eeh_pe_get_state(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int result, ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + if (!eeh_ops || !eeh_ops->get_state)
> + return -ENOENT;
> +
> + result = eeh_ops->get_state(pe, NULL);
> + if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + (result & EEH_STATE_DMA_ENABLED) &&
> + (result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_NORMAL;
> + else if (result & EEH_STATE_RESET_ACTIVE)
> + ret = EEH_PE_STATE_RESET;
> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + !(result & EEH_STATE_DMA_ENABLED) &&
> + !(result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_STOPPED_IO_DMA;
> + else if (!(result & EEH_STATE_RESET_ACTIVE) &&
> + (result & EEH_STATE_DMA_ENABLED) &&
> + !(result & EEH_STATE_MMIO_ENABLED))
> + ret = EEH_PE_STATE_STOPPED_DMA;
> + else
> + ret = EEH_PE_STATE_UNAVAIL;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_get_state);
> +
> +/**
> + * eeh_pe_reset - Issue PE reset according to specified type
> + * @pdev: PCI device
> + * @option: reset type
> + *
> + * The routine is called to reset the specified PE with the
> + * indicated type, either fundamental reset or hot reset.
> + * PE reset is the most important part for error recovery.
> + */
> +int eeh_pe_reset(struct pci_dev *pdev, int option)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + if (!eeh_ops || !eeh_ops->set_option || !eeh_ops->reset)
> + return -ENOENT;
> +
> + switch (option) {
> + case EEH_RESET_DEACTIVATE:
> + ret = eeh_ops->reset(pe, option);
> + if (ret)
> + break;
> +
> + /*
> + * The PE is still in frozen state and we need clear
to
> + * that. It's good to clear frozen state after deassert
> + * to avoid messy IO access during reset, which might
> + * cause recursive frozen PE.
> + */
> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
> + if (!ret)
> + ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
> + if (!ret)
> + eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
> + break;
> + case EEH_RESET_HOT:
> + case EEH_RESET_FUNDAMENTAL:
> + ret = eeh_ops->reset(pe, option);
> + break;
> + default:
> + pr_debug("%s: Unsupported option %d\n",
> + __func__, option);
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_reset);
> +
> +/**
> + * eeh_pe_configure - Configure PCI bridges after PE reset
> + * @pdev: PCI device
> + *
> + * The routine is called to restore the PCI config space for
> + * those PCI devices, especially PCI bridges affected by PE
> + * reset issued previously.
So would it make sense to combine this with the reset call?
> + */
> +int eeh_pe_configure(struct pci_dev *pdev)
> +{
> + struct eeh_dev *edev;
> + struct eeh_pe *pe;
> + int ret = 0;
> +
> + /* Device existing ? */
> + ret = eeh_dev_check(pdev, &edev, &pe);
> + if (ret)
> + return ret;
> +
> + /* Restore config space for the affected devices */
> + eeh_pe_restore_bars(pe);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(eeh_pe_configure);
> +
> static int proc_eeh_show(struct seq_file *m, void *v)
> {
> if (!eeh_enabled()) {
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 7ba0424..301ac18 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -25,6 +25,9 @@
> #include <linux/types.h>
> #include <linux/uaccess.h>
> #include <linux/vfio.h>
> +#ifdef CONFIG_EEH
> +#include <asm/eeh.h>
> +#endif
>
> #include "vfio_pci_private.h"
>
> @@ -152,32 +155,57 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
> pci_restore_state(pdev);
> }
>
> +static void vfio_eeh_pci_release(struct pci_dev *pdev)
> +{
> +#ifdef CONFIG_EEH
> + eeh_dev_release(pdev);
> +#endif
> +}
> +
> static void vfio_pci_release(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
>
> - if (atomic_dec_and_test(&vdev->refcnt))
> + if (atomic_dec_and_test(&vdev->refcnt)) {
> + vfio_eeh_pci_release(vdev->pdev);
> vfio_pci_disable(vdev);
> + }
>
> module_put(THIS_MODULE);
> }
>
> +static int vfio_eeh_pci_open(struct pci_dev *pdev)
> +{
> + int ret = 0;
> +
> +#ifdef CONFIG_EEH
> + ret = eeh_dev_open(pdev);
> +#endif
> + return ret;
> +}
> +
> static int vfio_pci_open(void *device_data)
> {
> struct vfio_pci_device *vdev = device_data;
> + int ret;
>
> if (!try_module_get(THIS_MODULE))
> return -ENODEV;
>
> if (atomic_inc_return(&vdev->refcnt) == 1) {
> - int ret = vfio_pci_enable(vdev);
> - if (ret) {
> - module_put(THIS_MODULE);
> - return ret;
> - }
> + ret = vfio_pci_enable(vdev);
> + if (ret)
> + goto error;
> +
> + ret = vfio_eeh_pci_open(vdev->pdev);
> + if (ret)
> + goto error;
> }
>
> return 0;
> +error:
> + module_put(THIS_MODULE);
> + return ret;
> }
>
> static int vfio_pci_get_irq_count(struct vfio_pci_device *vdev, int irq_type)
> @@ -321,6 +349,91 @@ static int vfio_pci_for_each_slot_or_bus(struct pci_dev *pdev,
> return walk.ret;
> }
>
> +static int vfio_eeh_pci_ioctl(struct pci_dev *pdev,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + unsigned long minsz;
> + int ret = 0;
> +
> +#ifdef CONFIG_EEH
> + switch (cmd) {
> + case VFIO_EEH_PE_SET_OPTION: {
> + struct vfio_eeh_pe_set_option option;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_set_option, option);
> + if (copy_from_user(&option, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (option.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_set_option(pdev, option.option);
> + break;
> + }
> + case VFIO_EEH_PE_GET_ADDR: {
> + struct vfio_eeh_pe_get_addr addr;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_get_addr, info);
> + if (copy_from_user(&addr, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (addr.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_get_addr(pdev, addr.option);
> + if (ret >= 0) {
> + addr.info = ret;
> + if (copy_to_user((void __user *)arg, &addr, minsz))
> + return -EFAULT;
> + ret = 0;
> + }
> +
> + break;
> + }
> + case VFIO_EEH_PE_GET_STATE: {
> + struct vfio_eeh_pe_get_state state;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_get_state, state);
> + if (copy_from_user(&state, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (state.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_get_state(pdev);
> + if (ret >= 0) {
> + state.state = ret;
> + if (copy_to_user((void __user *)arg, &state, minsz))
> + return -EFAULT;
> + ret = 0;
> + }
> + break;
> + }
> + case VFIO_EEH_PE_RESET: {
> + struct vfio_eeh_pe_reset reset;
> +
> + minsz = offsetofend(struct vfio_eeh_pe_reset, option);
> + if (copy_from_user(&reset, (void __user *)arg, minsz))
> + return -EFAULT;
> + if (reset.argsz < minsz)
> + return -EINVAL;
> +
> + ret = eeh_pe_reset(pdev, reset.option);
> + break;
> + }
> + case VFIO_EEH_PE_CONFIGURE:
> + ret = eeh_pe_configure(pdev);
> + break;
> + default:
> + ret = -EINVAL;
> + pr_debug("%s: Cannot handle command %d\n",
> + __func__, cmd);
> + }
> +#else
> + ret = -ENOENT;
> +#endif
> +
> + return ret;
> +}
> +
> static long vfio_pci_ioctl(void *device_data,
> unsigned int cmd, unsigned long arg)
> {
> @@ -682,6 +795,12 @@ hot_reset_release:
>
> kfree(groups);
> return ret;
> + } else if (cmd == VFIO_EEH_PE_SET_OPTION ||
> + cmd == VFIO_EEH_PE_GET_ADDR ||
> + cmd == VFIO_EEH_PE_GET_STATE ||
> + cmd == VFIO_EEH_PE_RESET ||
> + cmd == VFIO_EEH_PE_CONFIGURE) {
> + return vfio_eeh_pci_ioctl(vdev->pdev, cmd, arg);
> }
>
> return -ENOTTY;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index cb9023d..ef55682 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -455,6 +455,59 @@ struct vfio_iommu_spapr_tce_info {
>
> #define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>
> +/*
> + * EEH functionality can be enabled or disabled on one specific device.
> + * Also, the DMA or IO frozen state can be removed from the frozen PE
> + * if required.
> + */
> +struct vfio_eeh_pe_set_option {
> + __u32 argsz;
What is this argsz thing? Is this your way of maintaining backwards
compatibility when we introduce new fields? A new field will change the
ioctl number, so I don't think that makes a lot of sense :).
Just make the ioctl have a u32 as incoming argument. No fancy structs,
no complicated code.
The same applies for a number of structs below.
> + __u32 option;
> +};
> +
> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
> +
> +/*
> + * Each EEH PE should have unique address to be identified. The command
> + * helps to retrieve the address and the sharing mode of the PE.
> + */
> +struct vfio_eeh_pe_get_addr {
> + __u32 argsz;
> + __u32 option;
> + __u32 info;
Any particular reason you need the info field? Can't the return value of
the ioctl hold this? Then you only have a single u32 argument left to
the ioctl again.
Alex
> +};
> +
> +#define VFIO_EEH_PE_GET_ADDR _IO(VFIO_TYPE, VFIO_BASE + 22)
> +
> +/*
> + * EEH PE might have been frozen because of PCI errors. Also, it might
> + * be experiencing reset for error revoery. The following command helps
> + * to get the state.
> + */
> +struct vfio_eeh_pe_get_state {
> + __u32 argsz;
> + __u32 state;
> +};
> +
> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 23)
> +
> +/*
> + * Reset is the major step to recover problematic PE. The following
> + * command helps on that.
> + */
> +struct vfio_eeh_pe_reset {
> + __u32 argsz;
> + __u32 option;
> +};
> +
> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 24)
> +
> +/*
> + * One of the steps for recovery after PE reset is to configure the
> + * PCI bridges affected by the PE reset.
> + */
> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 25)
> +
> /* ***************************************************************** */
>
> #endif /* _UAPIVFIO_H */
^ permalink raw reply
* Re: [PATCH v6 3/3] powerpc/eeh: Avoid event on passed PE
From: Alexander Graf @ 2014-05-22 9:55 UTC (permalink / raw)
To: Gavin Shan, kvm-ppc; +Cc: aik, alex.williamson, qiudayu, linuxppc-dev
In-Reply-To: <1400747034-15045-4-git-send-email-gwshan@linux.vnet.ibm.com>
On 22.05.14 10:23, Gavin Shan wrote:
> If we detects frozen state on PE that has been passed through to somebody
> else. we needn't handle it. Instead, we rely on the device's owner to
> detect and recover it. The patch avoid EEH event on the frozen passed PE so
> that the device's owner can have chance to handle that.
>
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
I think you want to fold this with patch 1/3.
Alex
> ---
> arch/powerpc/kernel/eeh.c | 8 ++++++++
> arch/powerpc/platforms/powernv/eeh-ioda.c | 3 ++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> index b90a474..aee6cc5 100644
> --- a/arch/powerpc/kernel/eeh.c
> +++ b/arch/powerpc/kernel/eeh.c
> @@ -403,6 +403,14 @@ int eeh_dev_check_failure(struct eeh_dev *edev)
> if (ret > 0)
> return ret;
>
> + /*
> + * If the PE isn't owned by us, we shouldn't check the
> + * state. Instead, let the owner handle it if the PE has
> + * been frozen.
> + */
> + if (eeh_pe_passed(pe))
> + return 0;
> +
> /* If we already have a pending isolation event for this
> * slot, we know it's bad already, we don't need to check.
> * Do this checking under a lock; as multiple PCI devices
> diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
> index 1b5982f..03a3ed2 100644
> --- a/arch/powerpc/platforms/powernv/eeh-ioda.c
> +++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
> @@ -890,7 +890,8 @@ static int ioda_eeh_next_error(struct eeh_pe **pe)
> opal_pci_eeh_freeze_clear(phb->opal_id, frozen_pe_no,
> OPAL_EEH_ACTION_CLEAR_FREEZE_ALL);
> ret = EEH_NEXT_ERR_NONE;
> - } else if ((*pe)->state & EEH_PE_ISOLATED) {
> + } else if ((*pe)->state & EEH_PE_ISOLATED ||
> + eeh_pe_passed(*pe)) {
> ret = EEH_NEXT_ERR_NONE;
> } else {
> pr_err("EEH: Frozen PHB#%x-PE#%x (%s) detected\n",
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/perf: Adjust callchain based on DWARF debug
From: Jiri Olsa @ 2014-05-22 10:00 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: linuxppc-dev, Anton Blanchard, linux-kernel,
Arnaldo Carvalho de Melo, michael, Ulrich.Weigand,
Maynard Johnson
In-Reply-To: <20140521012644.GA2890@us.ibm.com>
On Tue, May 20, 2014 at 06:26:44PM -0700, Sukadev Bhattiprolu wrote:
SNIP
> + *
> + * The value in LR is only needed when it holds a return address. If the
> + * return address is on the stack, we should ignore the LR value.
> + *
> + * Further, when the return address is in the LR, if a new frame was just
> + * allocated but the LR was not saved into it, then the LR contains the
> + * caller, slot 4: contains the caller's caller and the contents of slot 3:
> + * (chain->ips[3]) is undefined and must be ignored.
> + *
> + * Use DWARF debug information to determine if any entries need to be skipped.
> + *
> + * Return:
> + * index: of callchain entry that needs to be ignored (if any)
> + * -1 if no entry needs to be ignored or in case of errors
> + *
> + * TODO:
> + * Rather than returning an index into the callchain and have the
> + * caller skip that entry, we could modify the callchain in-place
> + * by putting a PERF_CONTEXT_IGNORE marker in the affected entry.
> + *
> + * But @chain points to read-only mmap, so the caller needs to
> + * duplicate the callchain to modify in-place - something like:
> + *
> + * new_callchain = arch_duplicate_callchain();
> + * arch_adjust_callchain(new_callchain);
> + * ...
> + * arch_free_callchain(new_callchain);
> + *
> + * Since we only expect to adjust <= 1 entry for now, just return
> + * the index.
yep, that sounds more clear to me.. something like below?
calling callchain_dup from within arch_adjust_callchain in case
you want to change it and returning != 0 in this case, so
we could free the new callchain
but it might be to much overhead in case you have the support to skip
just one entry now right? is there a plan for more cleaning? ;)
thanks,
jirka
---
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 7409ac8..ec18310 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1279,6 +1279,11 @@ struct branch_info *sample__resolve_bstack(struct perf_sample *sample,
return bi;
}
+struct ip_callchain *callchain_dup(struct ip_callchain *chain)
+{
+ return memdup(chain, sizeof(*chain) + chain->nr * sizeof(u64));
+}
+
static int machine__resolve_callchain_sample(struct machine *machine,
struct thread *thread,
struct ip_callchain *chain,
@@ -1298,6 +1303,8 @@ static int machine__resolve_callchain_sample(struct machine *machine,
return 0;
}
+ adjusted = arch_adjust_callchain(machine, thread, &chain);
+
for (i = 0; i < chain_nr; i++) {
u64 ip;
struct addr_location al;
@@ -1326,7 +1333,7 @@ static int machine__resolve_callchain_sample(struct machine *machine,
* Discard all.
*/
callchain_cursor_reset(&callchain_cursor);
- return 0;
+ goto out;
}
continue;
}
@@ -1350,10 +1357,14 @@ static int machine__resolve_callchain_sample(struct machine *machine,
err = callchain_cursor_append(&callchain_cursor,
ip, al.map, al.sym);
if (err)
- return err;
+ goto out;
}
- return 0;
+ out:
+ if (adjusted)
+ free(chain);
+
+ return err;
}
static int unwind_entry(struct unwind_entry *entry, void *arg)
^ permalink raw reply related
* Re: [PATCH 1/1] powerpc/perf: Adjust callchain based on DWARF debug
From: Jiri Olsa @ 2014-05-22 10:04 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: linuxppc-dev, Anton Blanchard, linux-kernel,
Arnaldo Carvalho de Melo, michael, Ulrich.Weigand,
Maynard Johnson
In-Reply-To: <20140521012644.GA2890@us.ibm.com>
On Tue, May 20, 2014 at 06:26:44PM -0700, Sukadev Bhattiprolu wrote:
SNIP
> + * TODO:
> + * Rather than returning an index into the callchain and have the
> + * caller skip that entry, we could modify the callchain in-place
> + * by putting a PERF_CONTEXT_IGNORE marker in the affected entry.
> + *
> + * But @chain points to read-only mmap, so the caller needs to
> + * duplicate the callchain to modify in-place - something like:
> + *
> + * new_callchain = arch_duplicate_callchain();
> + * arch_adjust_callchain(new_callchain);
> + * ...
> + * arch_free_callchain(new_callchain);
> + *
> + * Since we only expect to adjust <= 1 entry for now, just return
> + * the index.
> + */
> +int arch_adjust_callchain(struct machine *machine, struct thread *thread,
> + struct ip_callchain *chain)
> +{
so in case we dont do the real adjusting of the callchain (see the other email)
and just returning the index, I think we should rename this accordingly,
so it's not confusing.. arch_callchain_skip_idx ? ;-)
SNIP
> --- a/tools/perf/util/callchain.h
> +++ b/tools/perf/util/callchain.h
> @@ -165,4 +165,16 @@ int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *samp
>
> extern const char record_callchain_help[];
> int parse_callchain_report_opt(const char *arg);
> +
> +#ifdef HAVE_ADJUST_CALLCHAIN
> +extern int arch_adjust_callchain(struct machine *machine,
> + struct thread *thread, struct ip_callchain *chain);
> +#else
> +static inline int arch_adjust_callchain(struct machine *machine,
> + struct thread *thread, struct ip_callchain *chain)
this needs 'maybe_unused' attribute at each parameter, otherwise
it won't compile for arch != ppc
thanks,
jirka
^ permalink raw reply
* Re: powerpc: CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS?
From: Benjamin Herrenschmidt @ 2014-05-22 13:17 UTC (permalink / raw)
To: Paul Bolle; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <1400749765.16832.18.camel@x220>
On Thu, 2014-05-22 at 11:09 +0200, Paul Bolle wrote:
> In v3.2 checks for CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS and
> CONFIG_WSP_DD1_WORKAROUND_BAD_PCIE_CLASS were added to
> arch/powerpc/platforms/wsp/wsp_pci.c. The check for
> CONFIG_WSP_DD1_WORKAROUND_BAD_PCIE_CLASS was dropped again in v3.3.
>
> Neither of these macros have ever had a matching Kconfig symbol. So they
> have always evaluated to false.
>
> Is it perhaps intended that one sets the macro still in the tree,
> CONFIG_WSP_DD1_WORKAROUND_DD1_TCE_BUGS, manually? And is that macro
> still needed?
To be honest we can kill all of the wsp code I think ... The project
got canned and I don't think anybody still uses them.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH RESEND 0/9 net-next] net: of_phy_connect_fixed_link removal
From: David Miller @ 2014-05-22 15:53 UTC (permalink / raw)
To: f.fainelli
Cc: thomas.petazzoni, devicetree, aida.mynzhasova, sergei.shtylyov,
netdev, richardcochran, linux-kernel, claudiu.manoil, vbordug,
grant.likely, linuxppc-dev
In-Reply-To: <1400708331-18088-1-git-send-email-f.fainelli@gmail.com>
Please address Sergei's feedback, except the indentation one which
as you stated is correct.
^ 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