* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Alex Williamson @ 2014-05-28 16:17 UTC (permalink / raw)
To: Alexander Graf; +Cc: aik, Gavin Shan, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <5385CA86.3010700@suse.de>
On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
> On 28.05.14 02:57, Alex Williamson wrote:
> > On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
> >> On 28.05.14 02:39, Alex Williamson wrote:
> >>> On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
> >>>> On 27.05.14 20:15, Alex Williamson wrote:
> >>>>> On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
> >>>>>> The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
> >>>>>> drivers/vfio/pci/Makefile | 1 +
> >>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
> >>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
> >>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
> >>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
> >>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
> >>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
> >>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
> >>>> [...]
> >>>>
> >>>>>> +
> >>>>>> + return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> static long tce_iommu_ioctl(void *iommu_data,
> >>>>>> unsigned int cmd, unsigned long arg)
> >>>>>> {
> >>>>>> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>>>>> tce_iommu_disable(container);
> >>>>>> mutex_unlock(&container->lock);
> >>>>>> return 0;
> >>>>>> + case VFIO_EEH_PE_SET_OPTION:
> >>>>>> + case VFIO_EEH_PE_GET_STATE:
> >>>>>> + case VFIO_EEH_PE_RESET:
> >>>>>> + case VFIO_EEH_PE_CONFIGURE:
> >>>>>> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
> >>>>> This is where it would have really made sense to have a single
> >>>>> VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
> >>>>> AlexG, are you really attached to splitting these out into separate
> >>>>> ioctls?
> >>>> I don't see the problem. We need to forward 4 ioctls to a separate piece
> >>>> of code, so we forward 4 ioctls to a separate piece of code :). Putting
> >>>> them into one ioctl just moves the switch() into another function.
> >>> And uses an extra 3 ioctl numbers and gives us extra things to update if
> >>> we ever need to add more ioctls, etc. ioctl numbers are an address
> >>> space, how much address space do we really want to give to EEH? It's
> >>> not a big difference, but I don't think it's completely even either.
> >>> Thanks,
> >> Yes, that's the point. I by far prefer to have you push back on anyone
> >> who introduces useless ioctls rather than have a separate EEH number
> >> space that people can just throw anything in they like ;).
> > Well, I appreciate that, but having them as separate ioctls doesn't
> > really prevent that either. Any one of these 4 could be set to take a
> > sub-option to extend and contort the EEH interface. The only way to
> > prevent that would be to avoid the argsz+flags hack that make the ioctl
> > extendable. Thanks,
>
> Sure, that's what patch review is about. I'm really more concerned about
> whose court the number space is in - you or Gavin. If we're talking
> about top level ioctls you will care a lot more.
>
> But I'm not religious about this. You're the VFIO maintainer, so it's
> your call. I just personally cringe when I see an ioctl that gets an
> "opcode" and a "parameter" argument where the "parameter" argument is a
> union with one struct for each opcode.
Well, what would it look like...
struct vfio_eeh_pe_op {
__u32 argsz;
__u32 flags;
__u32 op;
};
Couldn't every single one of these be a separate "op"? Are there any
cases where we can't use the ioctl return value?
VFIO_EEH_PE_DISABLE
VFIO_EEH_PE_ENABLE
VFIO_EEH_PE_UNFREEZE_IO
VFIO_EEH_PE_UNFREEZE_DMA
VFIO_EEH_PE_GET_MODE
VFIO_EEH_PE_RESET_DEACTIVATE
VFIO_EEH_PE_RESET_HOT
VFIO_EEH_PE_RESET_FUNDAMENTAL
VFIO_EEH_PE_CONFIGURE
It doesn't look that bad to me, what am I missing? Thanks,
Alex
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Alex Williamson @ 2014-05-28 16:32 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, agraf, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <20140528005532.GA5528@shangw>
On Wed, 2014-05-28 at 10:55 +1000, Gavin Shan wrote:
> On Tue, May 27, 2014 at 12:15:27PM -0600, Alex Williamson wrote:
> >On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
> >> The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
> >> drivers/vfio/pci/Makefile | 1 +
> >> drivers/vfio/pci/vfio_pci.c | 20 +++++---
> >> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
> >> drivers/vfio/pci/vfio_pci_private.h | 5 ++
> >> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
> >> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
> >> 7 files changed, 308 insertions(+), 7 deletions(-)
> >> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
> >>
> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> index b9ca023..d890fed 100644
> >> --- a/Documentation/vfio.txt
> >> +++ b/Documentation/vfio.txt
> >> @@ -305,7 +305,15 @@ 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) According to sPAPR specification, A Partitionable Endpoint (PE) is an I/O
> >> +subtree that can be treated as a unit for the purposes of partitioning and
> >> +error recovery. A PE may be a single or multi-function IOA (IO Adapter), a
> >> +function of a multi-function IOA, or multiple IOAs (possibly including switch
> >> +and bridge structures above the multiple IOAs). PPC64 guests detect PCI errors
> >> +and recover from them via EEH RTAS services, which works on the basis of
> >> +additional ioctl commands.
> >> +
> >> +So 7 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 +324,17 @@ So 3 additional ioctls have been added:
> >>
> >> VFIO_IOMMU_DISABLE - disables the container.
> >>
> >> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functionality on the
> >> + specified device. Also, it can be used to remove IO or DMA
> >> + stopped state on the frozen 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 +365,77 @@ 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_state state = { .argsz = sizeof(state) };
> >> + struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
> >> + struct vfio_eeh_pe_configure configure = { .argsz = sizeof(configure) };
> >> +
> >> + ....
> >> +
> >> + /* 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 = VFIO_EEH_PE_SET_OPT_ENABLE;
> >> + ioctl(container, VFIO_EEH_PE_SET_OPTION, &option);
> >> +
> >> + /* You're suggested to create additional data struct to represent
> >> + * PE, and put child devices belonging to same IOMMU group to the
> >> + * PE instance for later reference.
> >> + */
> >> +
> >> + /* Check the PE's state and make sure it's in functional state */
> >> + ioctl(container, 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(container, 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 = VFIO_EEH_PE_SET_OPT_IO;
> >> + ioctl(container, VFIO_EEH_PE_SET_OPTION, &option);
> >> +
> >> + /* Issue PE reset */
> >> + reset.option = VFIO_EEH_PE_RESET_HOT;
> >> + ioctl(container, VFIO_EEH_PE_RESET, &reset);
> >> + reset.option = VFIO_EEH_PE_RESET_DEACTIVATE;
> >> + ioctl(container, VFIO_EEH_PE_RESET, &reset);
> >> +
> >> + /* Configure the PCI bridges for the affected PE */
> >> + ioctl(container, VFIO_EEH_PE_CONFIGURE, &configure);
> >> +
> >> + /* 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/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >> index 1310792..faad885 100644
> >> --- a/drivers/vfio/pci/Makefile
> >> +++ b/drivers/vfio/pci/Makefile
> >> @@ -1,4 +1,5 @@
> >>
> >> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >> +vfio-pci-y += vfio_pci_eeh.o
> >
> >Why do we build this w/o CONFIG_EEH? Can't we define the functions as
> >static inline in the header in that case?
> >
>
> Ok. Will do in next revision.
>
> >> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> >> index 7ba0424..7c8d26a 100644
> >> --- a/drivers/vfio/pci/vfio_pci.c
> >> +++ b/drivers/vfio/pci/vfio_pci.c
> >> @@ -156,8 +156,10 @@ 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_pci_eeh_release(vdev->pdev);
> >> vfio_pci_disable(vdev);
> >> + }
> >>
> >> module_put(THIS_MODULE);
> >> }
> >> @@ -165,19 +167,25 @@ static void vfio_pci_release(void *device_data)
> >> 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_pci_eeh_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)
> >> diff --git a/drivers/vfio/pci/vfio_pci_eeh.c b/drivers/vfio/pci/vfio_pci_eeh.c
> >> new file mode 100644
> >> index 0000000..9c25207
> >> --- /dev/null
> >> +++ b/drivers/vfio/pci/vfio_pci_eeh.c
> >> @@ -0,0 +1,46 @@
> >> +/*
> >> + * EEH functionality support for VFIO PCI devices.
> >> + *
> >> + * Copyright Gavin Shan, IBM Corporation 2014.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#include <linux/device.h>
> >> +#include <linux/eventfd.h>
> >> +#include <linux/file.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/iommu.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/notifier.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/types.h>
> >> +#include <linux/uaccess.h>
> >> +#include <linux/vfio.h>
> >
> >Cleanup the includes
> >
>
> Ok. Will do in next revision.
>
> >> +#ifdef CONFIG_EEH
> >> +#include <asm/eeh.h>
> >> +#endif
> >
> >This shouldn't even be compiles w/o CONFIG_EEH
> >
>
> Yeah. Will fix in next revision as you suggested.
>
> >> +
> >> +#include "vfio_pci_private.h"
> >> +
> >> +int vfio_pci_eeh_open(struct pci_dev *pdev)
> >> +{
> >> + int ret = 0;
> >> +
> >> +#ifdef CONFIG_EEH
> >> + ret = eeh_dev_open(pdev);
> >> +#endif
> >> + return ret;
> >> +}
> >> +
> >> +void vfio_pci_eeh_release(struct pci_dev *pdev)
> >> +{
> >> +#ifdef CONFIG_EEH
> >> + eeh_dev_release(pdev);
> >> +#endif
> >> +}
> >> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> >> index 9c6d5d0..c3cbe40 100644
> >> --- a/drivers/vfio/pci/vfio_pci_private.h
> >> +++ b/drivers/vfio/pci/vfio_pci_private.h
> >> @@ -90,4 +90,9 @@ extern void vfio_pci_virqfd_exit(void);
> >>
> >> extern int vfio_config_init(struct vfio_pci_device *vdev);
> >> extern void vfio_config_free(struct vfio_pci_device *vdev);
> >> +
> >> +/* EEH stub */
> >> +extern int vfio_pci_eeh_open(struct pci_dev *pdev);
> >> +extern void vfio_pci_eeh_release(struct pci_dev *pdev);
> >
> >The #ifdef with static inlines should be here.
> >
>
> thanks for the detailed comments.
>
> >> +
> >> #endif /* VFIO_PCI_PRIVATE_H */
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> index a84788b..666691b 100644
> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -21,6 +21,9 @@
> >> #include <linux/vfio.h>
> >> #include <asm/iommu.h>
> >> #include <asm/tce.h>
> >> +#ifdef CONFIG_EEH
> >> +#include <asm/eeh.h>
> >> +#endif
> >
> >Now we're infecting another file with EEH, can't we put it in a central
> >location for vfio?
>
> How about something like this:
>
> - Create drivers/vfio/vfio_eeh.c and it will be built when having CONFIG_EEH
> - Declare all EEH functions in include/linux/vfio.h (it's the only central
> header file I can find except you have a better one).
>
> #ifdef CONFIG_EEH
> extern int vfio_pci_eeh_open(struct pci_dev *pdev);
> extern void vfio_pci_eeh_release(struct pci_dev *pdev);
> extern long vfio_iommu_spapr_eeh_ioctl(struct iommu_table *tbl,
> unsigned int cmd, unsigned long arg);
> #else
> static int vfio_pci_eeh_open(struct pci_dev *pdev)
> {
> return 0;
> }
>
> static void vfio_pci_eeh_release(struct pci_dev *pdev)
> {
> }
>
> static inline vfio_iommu_spapr_eeh_ioctl(struct iommu_table *tbl,
> unsigned int cmd, unsigned long arg);
> {
> return -ENOTTY;
> }
> #endif
Seems like an improvement
> >>
> >> #define DRIVER_VERSION "0.1"
> >> #define DRIVER_AUTHOR "aik@ozlabs.ru"
> >> @@ -147,6 +150,83 @@ static void tce_iommu_release(void *iommu_data)
> >> kfree(container);
> >> }
> >>
> >> +static long tce_iommu_eeh_ioctl(void *iommu_data,
> >> + unsigned int cmd, unsigned long arg)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> + 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(eeh_iommu_table_to_pe(container->tbl),
> >> + option.option);
> >> + 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(eeh_iommu_table_to_pe(container->tbl));
> >> + if (ret >= 0) {
> >> + state.state = ret;
> >> + if (copy_to_user((void __user *)arg, &state, minsz))
> >> + return -EFAULT;
> >> + ret = 0;
> >> + }
> >
> >This looks like one of those cases where you should just use the ioctl
> >return value.
> >
>
> Ok. I'll use ioctl return value to carry output information. Note it might
> have "0" as the output information.
I think that's fine.
> >> + 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(eeh_iommu_table_to_pe(container->tbl),
> >> + reset.option);
> >> + break;
> >> + }
> >> + case VFIO_EEH_PE_CONFIGURE: {
> >> + struct vfio_eeh_pe_configure configure;
> >> +
> >> + minsz = offsetofend(struct vfio_eeh_pe_configure, flags);
> >> + if (copy_from_user(&configure, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> + if (configure.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + ret = eeh_pe_configure(eeh_iommu_table_to_pe(container->tbl));
> >> + break;
> >> + }
> >> + default:
> >> + ret = -EINVAL;
> >> + pr_debug("%s: Cannot handle command %d\n",
> >> + __func__, cmd);
> >> + }
> >> +#else
> >> + ret = -ENOENT;
> >> +#endif
> >
> >Hmm, more like a BUG in the default case the way it's coded here (not
> >even sure it's worth the default entry). The #else case should probably
> >be -ENOTTY like other unimplemented ioctls.
> >
>
> Yeah, we don't need default case here and I'll remove it in next revision.
> Also, "-ENOTTY" will be used in future for unimplemented functions.
>
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static long tce_iommu_ioctl(void *iommu_data,
> >> unsigned int cmd, unsigned long arg)
> >> {
> >> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
> >> tce_iommu_disable(container);
> >> mutex_unlock(&container->lock);
> >> return 0;
> >> + case VFIO_EEH_PE_SET_OPTION:
> >> + case VFIO_EEH_PE_GET_STATE:
> >> + case VFIO_EEH_PE_RESET:
> >> + case VFIO_EEH_PE_CONFIGURE:
> >> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
> >
> >This is where it would have really made sense to have a single
> >VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
> >AlexG, are you really attached to splitting these out into separate
> >ioctls?
> >
>
> I really don't know. Alex.G want separate ioctl commands, but you
> suggested to combine them. Could you guys please just tell me which
> one (separate vs combined) I need to have in next revision? :-)
>
> >> }
> >>
> >> return -ENOTTY;
> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> >> index cb9023d..c5fac36 100644
> >> --- a/include/uapi/linux/vfio.h
> >> +++ b/include/uapi/linux/vfio.h
> >> @@ -455,6 +455,72 @@ 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 flags;
> >> + __u32 option;
> >> +#define VFIO_EEH_PE_SET_OPT_DISABLE 0 /* Disable EEH */
> >> +#define VFIO_EEH_PE_SET_OPT_ENABLE 1 /* Enable EEH */
> >> +#define VFIO_EEH_PE_SET_OPT_IO 2 /* Enable IO */
> >> +#define VFIO_EEH_PE_SET_OPT_DMA 3 /* Enable DMA */
> >
> >This is more of a "command" than an "option" isn't it? Each of these
> >probably needs a more significant description.
> >
>
> Yeah, it would be regarded as "opcode" and I'll add more description about
> them in next revision.
>
> >> +};
> >> +
> >> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
> >> +
> >> +/*
> >> + * Each EEH PE should have unique address to be identified. PE's
> >> + * sharing mode is also useful information as well.
> >> + */
> >> +#define VFIO_EEH_PE_GET_ADDRESS 0 /* Get address */
> >> +#define VFIO_EEH_PE_GET_MODE 1 /* Query mode */
> >> +#define VFIO_EEH_PE_MODE_NONE 0 /* Not a PE */
> >> +#define VFIO_EEH_PE_MODE_NOT_SHARED 1 /* Exclusive */
> >> +#define VFIO_EEH_PE_MODE_SHARED 2 /* Shared mode */
> >> +
> >> +/*
> >> + * 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 flags;
> >> + __u32 state;
> >> +};
> >
> >Should state be a union to better describe the value returned? What
> >exactly is the address and why does the user need to know it? Does this
> >need user input or could we just return the address and mode regardless?
> >
>
> Ok. I think you want enum (not union) for state. I'll have macros for the
> state in next revision as I did that for other cases.
>
> Those macros defined for "address" just for ABI stuff as Alex.G mentioned.
> There isn't corresponding ioctl command for host to get address any more
> because QEMU (user) will have to figure it out by himself. The "address"
> here means PE address and user has to figure it out according to PE
> segmentation.
>
> >> +
> >> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 22)
> >> +
> >> +/*
> >> + * Reset is the major step to recover problematic PE. The following
> >> + * command helps on that.
> >> + */
> >> +struct vfio_eeh_pe_reset {
> >> + __u32 argsz;
> >> + __u32 flags;
> >> + __u32 option;
> >> +#define VFIO_EEH_PE_RESET_DEACTIVATE 0 /* Deactivate reset */
> >> +#define VFIO_EEH_PE_RESET_HOT 1 /* Hot reset */
> >> +#define VFIO_EEH_PE_RESET_FUNDAMENTAL 3 /* Fundamental reset */
> >
> >How does a user know which of these to use?
> >
>
> I think Ben already helped explaining it for a lot in the subsequent
> replies. Thanks to Ben :-)
>
> >> +};
> >> +
> >> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 23)
> >> +
> >> +/*
> >> + * One of the steps for recovery after PE reset is to configure the
> >> + * PCI bridges affected by the PE reset.
> >> + */
> >> +struct vfio_eeh_pe_configure {
> >> + __u32 argsz;
> >> + __u32 flags;
> >> +};
> >
> >Parameters are probably not necessary here.
> >
>
> Yep. I'll remove it in next revision. Thanks for your comments, Alex.
>
> >> +
> >> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 24)
> >> +
> >> /* ***************************************************************** */
> >>
> >> #endif /* _UAPIVFIO_H */
> >
> >How does a user learn that a device supports EEH? Do they just start
> >making ioctl calls and getting a failure? Shouldn't we make use of one
> >of the flag bits on the device or add a capability on the container for
> >the user to query?
> >
>
> User needs to make some ioctl calls to make sure EEH can be supported on
> one specific device:
>
> struct vfio_eeh_pe_set_option set_option;
>
> /* User have to make sure the device isn't a bridge and there
> * has one PE for it, which means that case of VFIO_EEH_PE_MODE_NONE.
> */
>
> set_option.argsz = sizeof(set_option);
> set_option.option = VFIO_EEH_PE_SET_OPT_ENABLE;
> ret = ioctl(container, VFIO_EEH_PE_SET_OPTION, &set_option);
> if (ret < 0) {
> /* EEH can't supported */
> }
No, the user should be able to query whether it's available with
VFIO_CHECK_EXTENSION.
I think I'm also still looking for an explanation of how the user learns
about the error that occurred, not just that it occurred, but what it
was. The description of the recovery process indicates a lot retrieval
step, but there's no interface here for the user to get logs. Is that a
TBD? Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] tty/hvc/hvc_console: Fix wakeup of HVC thread on hvc_kick()
From: Greg Kroah-Hartman @ 2014-05-28 20:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev list, Jiri Slaby, Linux Kernel list
In-Reply-To: <1401235290.8262.9.camel@pasglop>
On Wed, May 28, 2014 at 10:01:30AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2014-05-23 at 19:41 +1000, Benjamin Herrenschmidt wrote:
> > Some backends call hvc_kick() to wakeup the HVC thread from its
> > slumber upon incoming characters. This however doesn't work
> > properly because it uses msleep_interruptible() which is mostly
> > immune to wake_up_process(). It will basically go back to sleep
> > until the timeout is expired (only signals can really wake it).
> >
> > Replace it with a simple shedule_timeout_interruptible() instead,
> > which may wakeup earlier every now and then but we really don't
> > care in this case.
>
> Nobody commented ? :-)
>
> Greg, do you want to take this in the tty tree or can I stick it in
> powerpc ?
I'll take it, thanks.
greg k-h
^ permalink raw reply
* Re: NUMA topology question wrt. d4edc5b6
From: Nishanth Aravamudan @ 2014-05-28 20:37 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Dave Hansen, Srikar Dronamraju,
linuxppc-dev@lists.ozlabs.org list, Linux MM, Aneesh Kumar K.V,
nfont, Cody P Schafer, Anton Blanchard
In-Reply-To: <537E6285.3050000@linux.vnet.ibm.com>
On 23.05.2014 [02:18:05 +0530], Srivatsa S. Bhat wrote:
>
> [ Adding a few more CC's ]
>
> On 05/22/2014 01:34 AM, Nishanth Aravamudan wrote:
> > Hi Srivatsa,
> >
> > After d4edc5b6 ("powerpc: Fix the setup of CPU-to-Node mappings during
> > CPU online"), cpu_to_node() looks like:
> >
> > static inline int cpu_to_node(int cpu)
> > {
> > int nid;
> >
> > nid = numa_cpu_lookup_table[cpu];
> >
> > /*
> > * During early boot, the numa-cpu lookup table might not have been
> > * setup for all CPUs yet. In such cases, default to node 0.
> > */
> > return (nid < 0) ? 0 : nid;
> > }
> >
> > However, I'm curious if this is correct in all cases. I have seen
> > several LPARs that do not have any CPUs on node 0. In fact, because node
> > 0 is statically set online in the initialization of the N_ONLINE
> > nodemask, 0 is always present to Linux, whether it is present on the
> > system. I'm not sure what the best thing to do here is, but I'm curious
> > if you have any ideas? I would like to remove the static initialization
> > of node 0, as it's confusing to users to see an empty node (particularly
> > when it's completely separate in the numbering from other nodes), but
> > we trip a panic (refer to:
> > http://www.spinics.net/lists/linux-mm/msg73321.html).
> >
>
> Ah, I see. I didn't have any particular reason to default it to zero.
> I just did that because the existing code before this patch did the same
> thing. (numa_cpu_lookup_table[] is a global array, so it will be initialized
> with zeros. So if we access it before populating it via numa_setup_cpu(),
> it would return 0. So I retained that behaviour with the above conditional).
Ok, that seems reasonable to me (keeping the behavior the same as it was
before).
> Will something like the below [totally untested] patch solve the boot-panic?
> I understand that as of today first_online_node will still pick 0 since
> N_ONLINE is initialized statically, but with your proposed change to that
> init code, I guess the following patch should avoid the boot panic.
>
> [ But note that first_online_node is hard-coded to 0, if MAX_NUMNODES is = 1.
> So we'll have to fix that if powerpc can have a single node system whose node
> is numbered something other than 0. Can that happen as well? ]
I think all single-node systems are only Node 0, but I'm not 100% on
that.
> And regarding your question about what is the best way to fix this
> whole Linux MM's assumption about node0, I'm not really sure.. since I
> am not really aware of the extent to which the MM subsystem is
> intertwined with this assumption and what it would take to cure that
> :-(
Well, at this point, it might be fine to just leave it alone, as it
seems to be more trouble than it's worth -- and really the only
confusion is on those LPARs where there really isn't a Node 0. I'll take
another look later this week.
Thanks,
Nish
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Benjamin Herrenschmidt @ 2014-05-28 21:58 UTC (permalink / raw)
To: Gavin Shan
Cc: aik, Alexander Graf, kvm-ppc, Alex Williamson, qiudayu,
linuxppc-dev
In-Reply-To: <20140528124947.GA19346@shangw>
On Wed, 2014-05-28 at 22:49 +1000, Gavin Shan wrote:
>
> I will remove those "address" related macros in next revision because it's
> user-level bussiness, not related to host kernel any more.
>
> If the user is QEMU + guest, we need the address to identify the PE though PHB
> BUID could be used as same purpose to get PHB, which is one-to-one mapping with
> IOMMU group on sPAPR platform. However, once the PE address is built and returned
> to guest, guest will use the PE address as input parameter in subsequent RTAS
> calls.
>
> If the user is some kind of application who just uses the ioctl() without supporting
> RTAS calls. We don't need care about PE address.
I am a bit reluctant with that PE==PHB equation we seem to be introducing.
This isn't the case in HW. It's possible that this is how we handle VFIO *today*
in qemu but it doesn't have to be does it ?
It also paints us into a corner if we want to start implementing some kind of
emulated EEH for selected emulated devices and/or virtio.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 11/16] byteorder: provide a linux/byteorder.h with {be, le}_to_cpu() and cpu_to_{be, le}() macros
From: Cody P Schafer @ 2014-05-28 22:05 UTC (permalink / raw)
To: David Laight; +Cc: Sukadev Bhattiprolu, Linux PPC, LKML
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1724EEAF@AcuExch.aculab.com>
On Wed, May 28, 2014 at 3:45 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Cody P Schafer
>> Rather manually specifying the size of the integer to be converted, key
>> off of the type size. Reduces duplicate size info and the occurance of
>> certain types of bugs (using the wrong sized conversion).
> ...
>> +#define be_to_cpu(v) \
>> + __builtin_choose_expr(sizeof(v) == sizeof(uint8_t) , v, \
>> + __builtin_choose_expr(sizeof(v) == sizeof(uint16_t), be16_to_cpu(v), \
>> + __builtin_choose_expr(sizeof(v) == sizeof(uint32_t), be32_to_cpu(v), \
>> + __builtin_choose_expr(sizeof(v) == sizeof(uint64_t), be64_to_cpu(v), \
>> + (void)0))))
> ...
>
> I'm not at all sure that using the 'size' of the constant will reduce
> the number of bugs - it just introduces a whole new category of bugs.
Certainly, if you mis-size the argument (and thus have missized one of
the variables containing the be value, probably a bug anyhow), there
will be problems.
I put this interface together because of an actual bug I wrote into
the initial code of the hv_24x7 driver (resized a struct member
without adjusting the be*_to_cpu() sizing).
Having this "auto sizing" macro means I can avoid encoding the size of
a struct field in multiple places.
^ permalink raw reply
* Re: [PATCH 11/16] byteorder: provide a linux/byteorder.h with {be,le}_to_cpu() and cpu_to_{be,le}() macros
From: Cody P Schafer @ 2014-05-28 22:07 UTC (permalink / raw)
To: Joe Perches; +Cc: Sukadev Bhattiprolu, Linux PPC, LKML
In-Reply-To: <1401237869.20587.12.camel@joe-AO725>
On Tue, May 27, 2014 at 7:44 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-05-27 at 17:22 -0700, Cody P Schafer wrote:
>> Rather manually specifying the size of the integer to be converted, key
>> off of the type size. Reduces duplicate size info and the occurance of
>> certain types of bugs (using the wrong sized conversion).
> []
>> diff --git a/include/linux/byteorder.h b/include/linux/byteorder.h
> []
>> @@ -0,0 +1,34 @@
>> +#ifndef LINUX_BYTEORDER_H_
>> +#define LINUX_BYTEORDER_H_
>> +
>> +#include <asm/byteorder.h>
>> +
>> +#define be_to_cpu(v) \
>> + __builtin_choose_expr(sizeof(v) =3D=3D sizeof(uint8_t) , v, \
>> + __builtin_choose_expr(sizeof(v) =3D=3D sizeof(uint16_t), be16_to_c=
pu(v), \
>> + __builtin_choose_expr(sizeof(v) =3D=3D sizeof(uint32_t), be32_to_c=
pu(v), \
>> + __builtin_choose_expr(sizeof(v) =3D=3D sizeof(uint64_t), be64_to_c=
pu(v), \
>> + (void)0))))
>
> probably better to use BUILD_BUG instead of these 0 returns
>
They aren't 0 returns.
$ echo "int main(void) { int x =3D (void)0; return x; }" | gcc -x c -
<stdin>: In function =E2=80=98main=E2=80=99:
<stdin>:1:26: error: void value not ignored as it ought to be
^ permalink raw reply
* Re: [PATCH 11/16] byteorder: provide a linux/byteorder.h with {be, le}_to_cpu() and cpu_to_{be, le}() macros
From: Cody P Schafer @ 2014-05-28 22:11 UTC (permalink / raw)
To: David Laight; +Cc: Sukadev Bhattiprolu, Linux PPC, LKML
In-Reply-To: <CAPoQQ-17OHotfbczJvEoq=vAzu_+6-sV3M1ZAhDaQ45wB3NB4A@mail.gmail.com>
On Wed, May 28, 2014 at 5:05 PM, Cody P Schafer <dev@codyps.com> wrote:
> On Wed, May 28, 2014 at 3:45 AM, David Laight <David.Laight@aculab.com> wrote:
>> From: Cody P Schafer
>>> Rather manually specifying the size of the integer to be converted, key
>>> off of the type size. Reduces duplicate size info and the occurance of
>>> certain types of bugs (using the wrong sized conversion).
>> ...
>>> +#define be_to_cpu(v) \
>>> + __builtin_choose_expr(sizeof(v) == sizeof(uint8_t) , v, \
>>> + __builtin_choose_expr(sizeof(v) == sizeof(uint16_t), be16_to_cpu(v), \
>>> + __builtin_choose_expr(sizeof(v) == sizeof(uint32_t), be32_to_cpu(v), \
>>> + __builtin_choose_expr(sizeof(v) == sizeof(uint64_t), be64_to_cpu(v), \
>>> + (void)0))))
>> ...
>>
>> I'm not at all sure that using the 'size' of the constant will reduce
>> the number of bugs - it just introduces a whole new category of bugs.
>
> Certainly, if you mis-size the argument (and thus have missized one of
> the variables containing the be value, probably a bug anyhow), there
> will be problems.
>
> I put this interface together because of an actual bug I wrote into
> the initial code of the hv_24x7 driver (resized a struct member
> without adjusting the be*_to_cpu() sizing).
> Having this "auto sizing" macro means I can avoid encoding the size of
> a struct field in multiple places.
To clarify, the point I'm making here is that this simply cuts out 1
more place we can screw up endianness conversion sizing.
^ permalink raw reply
* [PATCH] powerpc/perf: Never program book3s PMCs with values >= 0x80000000
From: Anton Blanchard @ 2014-05-28 22:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, mpe; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]
We are seeing a lot of PMU warnings on POWER8:
Can't find PMC that caused IRQ
Looking closer, the active PMC is 0 at this point and we took a PMU
exception on the transition from negative to 0. Some versions of POWER8
have an issue where they edge detect and not level detect PMC overflows.
A number of places program the PMC with (0x80000000 - period_left),
where period_left can be negative. We can either fix all of these or
just ensure that period_left is always >= 1.
This patch takes the second option.
Cc: <stable@vger.kernel.org>
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Attached is a graph that shows how this bug also causes problems in the
initial period calculation for samples. Once fixed we converge on
the correct period very quickly.
Index: b/arch/powerpc/perf/core-book3s.c
===================================================================
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -996,7 +996,22 @@ static void power_pmu_read(struct perf_e
} while (local64_cmpxchg(&event->hw.prev_count, prev, val) != prev);
local64_add(delta, &event->count);
- local64_sub(delta, &event->hw.period_left);
+
+ /*
+ * A number of places program the PMC with (0x80000000 - period_left).
+ * We never want period_left to be less than 1 because we will program
+ * the PMC with a value >= 0x800000000 and an edge detected PMC will
+ * roll around to 0 before taking an exception. We have seen this
+ * on POWER8.
+ *
+ * To fix this, clamp the minimum value of period_left to 1.
+ */
+ do {
+ prev = local64_read(&event->hw.period_left);
+ val = prev - delta;
+ if (val < 1)
+ val = 1;
+ } while (local64_cmpxchg(&event->hw.period_left, prev, val) != prev);
}
/*
[-- Attachment #2: perf_record_sampling_period.png --]
[-- Type: image/png, Size: 30902 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-28 22:40 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, qiudayu, linuxppc-dev
In-Reply-To: <1401293853.2412.36.camel@ul30vt.home>
On 28.05.14 18:17, Alex Williamson wrote:
> On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>> On 28.05.14 02:57, Alex Williamson wrote:
>>> On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
>>>> On 28.05.14 02:39, Alex Williamson wrote:
>>>>> On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
>>>>>> On 27.05.14 20:15, Alex Williamson wrote:
>>>>>>> On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>>> The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>>>>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>>>>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>>>>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>>>>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>>>> [...]
>>>>>>
>>>>>>>> +
>>>>>>>> + return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>> unsigned int cmd, unsigned long arg)
>>>>>>>> {
>>>>>>>> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>> tce_iommu_disable(container);
>>>>>>>> mutex_unlock(&container->lock);
>>>>>>>> return 0;
>>>>>>>> + case VFIO_EEH_PE_SET_OPTION:
>>>>>>>> + case VFIO_EEH_PE_GET_STATE:
>>>>>>>> + case VFIO_EEH_PE_RESET:
>>>>>>>> + case VFIO_EEH_PE_CONFIGURE:
>>>>>>>> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>>>>>>> This is where it would have really made sense to have a single
>>>>>>> VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>>>>>>> AlexG, are you really attached to splitting these out into separate
>>>>>>> ioctls?
>>>>>> I don't see the problem. We need to forward 4 ioctls to a separate piece
>>>>>> of code, so we forward 4 ioctls to a separate piece of code :). Putting
>>>>>> them into one ioctl just moves the switch() into another function.
>>>>> And uses an extra 3 ioctl numbers and gives us extra things to update if
>>>>> we ever need to add more ioctls, etc. ioctl numbers are an address
>>>>> space, how much address space do we really want to give to EEH? It's
>>>>> not a big difference, but I don't think it's completely even either.
>>>>> Thanks,
>>>> Yes, that's the point. I by far prefer to have you push back on anyone
>>>> who introduces useless ioctls rather than have a separate EEH number
>>>> space that people can just throw anything in they like ;).
>>> Well, I appreciate that, but having them as separate ioctls doesn't
>>> really prevent that either. Any one of these 4 could be set to take a
>>> sub-option to extend and contort the EEH interface. The only way to
>>> prevent that would be to avoid the argsz+flags hack that make the ioctl
>>> extendable. Thanks,
>> Sure, that's what patch review is about. I'm really more concerned about
>> whose court the number space is in - you or Gavin. If we're talking
>> about top level ioctls you will care a lot more.
>>
>> But I'm not religious about this. You're the VFIO maintainer, so it's
>> your call. I just personally cringe when I see an ioctl that gets an
>> "opcode" and a "parameter" argument where the "parameter" argument is a
>> union with one struct for each opcode.
> Well, what would it look like...
>
> struct vfio_eeh_pe_op {
> __u32 argsz;
> __u32 flags;
> __u32 op;
> };
>
> Couldn't every single one of these be a separate "op"? Are there any
> cases where we can't use the ioctl return value?
>
> VFIO_EEH_PE_DISABLE
> VFIO_EEH_PE_ENABLE
> VFIO_EEH_PE_UNFREEZE_IO
> VFIO_EEH_PE_UNFREEZE_DMA
> VFIO_EEH_PE_GET_MODE
> VFIO_EEH_PE_RESET_DEACTIVATE
> VFIO_EEH_PE_RESET_HOT
> VFIO_EEH_PE_RESET_FUNDAMENTAL
> VFIO_EEH_PE_CONFIGURE
>
> It doesn't look that bad to me, what am I missing? Thanks,
Yup, that looks well to me as well :)
Alex
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-28 22:46 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Gavin Shan
Cc: aik, qiudayu, Alex Williamson, linuxppc-dev, kvm-ppc
In-Reply-To: <1401314311.8262.26.camel@pasglop>
On 28.05.14 23:58, Benjamin Herrenschmidt wrote:
> On Wed, 2014-05-28 at 22:49 +1000, Gavin Shan wrote:
>> I will remove those "address" related macros in next revision because it's
>> user-level bussiness, not related to host kernel any more.
>>
>> If the user is QEMU + guest, we need the address to identify the PE though PHB
>> BUID could be used as same purpose to get PHB, which is one-to-one mapping with
>> IOMMU group on sPAPR platform. However, once the PE address is built and returned
>> to guest, guest will use the PE address as input parameter in subsequent RTAS
>> calls.
>>
>> If the user is some kind of application who just uses the ioctl() without supporting
>> RTAS calls. We don't need care about PE address.
> I am a bit reluctant with that PE==PHB equation we seem to be introducing.
>
> This isn't the case in HW. It's possible that this is how we handle VFIO *today*
> in qemu but it doesn't have to be does it ?
Right, but that's pure QEMU internals. From the VFIO kernel interface's
point of view, a VFIO group is a PE context, as that's what gets IOMMU
controlled together too.
> It also paints us into a corner if we want to start implementing some kind of
> emulated EEH for selected emulated devices and/or virtio.
I don't think so :). In QEMU the PHB emulation would have to notify the
"container" (IOMMU emulation layer -> PE) that a PE operation happened.
It's that emulation code's responsibility to broadcast operations across
its own emulated operations (recover config space access, reconfigure
BARs, etc) and the VFIO PE operations.
So from a kernel interface point of view, I think leaving out any
address information is the right way to go. Whether we managed to get
all QEMU internal interfaces modeled correctly yet has to be seen on the
next patch set revision :).
Alex
^ permalink raw reply
* Re: [PATCH 11/16] byteorder: provide a linux/byteorder.h with {be, le}_to_cpu() and cpu_to_{be, le}() macros
From: Joe Perches @ 2014-05-28 23:00 UTC (permalink / raw)
To: Cody P Schafer; +Cc: Sukadev Bhattiprolu, David Laight, Linux PPC, LKML
In-Reply-To: <CAPoQQ-11vW3Q0=LcS-myNZ6hXeYBeu==_prwHJXHvLfkUSmZHQ@mail.gmail.com>
On Wed, 2014-05-28 at 17:11 -0500, Cody P Schafer wrote:
> On Wed, May 28, 2014 at 5:05 PM, Cody P Schafer <dev@codyps.com> wrote:
> > On Wed, May 28, 2014 at 3:45 AM, David Laight <David.Laight@aculab.com> wrote:
> >> From: Cody P Schafer
> >>> Rather manually specifying the size of the integer to be converted, key
> >>> off of the type size. Reduces duplicate size info and the occurance of
> >>> certain types of bugs (using the wrong sized conversion).
> >> ...
> >>> +#define be_to_cpu(v) \
> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint8_t) , v, \
> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint16_t), be16_to_cpu(v), \
> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint32_t), be32_to_cpu(v), \
> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint64_t), be64_to_cpu(v), \
> >>> + (void)0))))
> >> ...
> >>
> >> I'm not at all sure that using the 'size' of the constant will reduce
> >> the number of bugs - it just introduces a whole new category of bugs.
> >
> > Certainly, if you mis-size the argument (and thus have missized one of
> > the variables containing the be value, probably a bug anyhow), there
> > will be problems.
> >
> > I put this interface together because of an actual bug I wrote into
> > the initial code of the hv_24x7 driver (resized a struct member
> > without adjusting the be*_to_cpu() sizing).
> > Having this "auto sizing" macro means I can avoid encoding the size of
> > a struct field in multiple places.
>
> To clarify, the point I'm making here is that this simply cuts out 1
> more place we can screw up endianness conversion sizing.
It does screw up other types when you do things like:
u8 foo = some_function();
cpu_to_be(foo + 1);
the return value is sizeof(int) not u8
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-28 23:13 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <5385E0C3.8020000@suse.de>
On Wed, May 28, 2014 at 03:12:35PM +0200, Alexander Graf wrote:
>
>On 28.05.14 14:49, Gavin Shan wrote:
>>On Wed, May 28, 2014 at 01:41:35PM +0200, Alexander Graf wrote:
>>>On 28.05.14 02:55, Gavin Shan wrote:
>>>>On Tue, May 27, 2014 at 12:15:27PM -0600, Alex Williamson wrote:
>>>>>On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>[...]
>>>
>>>>>>diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>>>>index cb9023d..c5fac36 100644
>>>>>>--- a/include/uapi/linux/vfio.h
>>>>>>+++ b/include/uapi/linux/vfio.h
>>>>>>@@ -455,6 +455,72 @@ 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 flags;
>>>>>>+ __u32 option;
>>>>>>+#define VFIO_EEH_PE_SET_OPT_DISABLE 0 /* Disable EEH */
>>>>>>+#define VFIO_EEH_PE_SET_OPT_ENABLE 1 /* Enable EEH */
>>>>>>+#define VFIO_EEH_PE_SET_OPT_IO 2 /* Enable IO */
>>>>>>+#define VFIO_EEH_PE_SET_OPT_DMA 3 /* Enable DMA */
>>>>>This is more of a "command" than an "option" isn't it? Each of these
>>>>>probably needs a more significant description.
>>>>>
>>>>Yeah, it would be regarded as "opcode" and I'll add more description about
>>>>them in next revision.
>>>Please just call them commands.
>>>
>>Ok. I guess you want me to change the macro names like this ?
>>
>>#define VFIO_EEH_CMD_DISABLE 0 /* Disable EEH functionality */
>>#define VFIO_EEH_CMD_ENABLE 1 /* Enable EEH functionality */
>>#define VFIO_EEH_CMD_ENABLE_IO 2 /* Enable IO for frozen PE */
>>#define VFIO_EEH_CMD_ENABLE_DMA 3 /* Enable DMA for frozen PE */
>
>Yes, the ioctl name too.
>
Ok. Thanks. I will also to rename those "option" / "command" related macros
to "VFIO_EEH_CMD_xxxx" in next revision.
>>
>>>>>>+};
>>>>>>+
>>>>>>+#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>>>>>>+
>>>>>>+/*
>>>>>>+ * Each EEH PE should have unique address to be identified. PE's
>>>>>>+ * sharing mode is also useful information as well.
>>>>>>+ */
>>>>>>+#define VFIO_EEH_PE_GET_ADDRESS 0 /* Get address */
>>>>>>+#define VFIO_EEH_PE_GET_MODE 1 /* Query mode */
>>>>>>+#define VFIO_EEH_PE_MODE_NONE 0 /* Not a PE */
>>>>>>+#define VFIO_EEH_PE_MODE_NOT_SHARED 1 /* Exclusive */
>>>>>>+#define VFIO_EEH_PE_MODE_SHARED 2 /* Shared mode */
>>>>>>+
>>>>>>+/*
>>>>>>+ * 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 flags;
>>>>>>+ __u32 state;
>>>>>>+};
>>>>>Should state be a union to better describe the value returned? What
>>>>>exactly is the address and why does the user need to know it? Does this
>>>>>need user input or could we just return the address and mode regardless?
>>>>>
>>>>Ok. I think you want enum (not union) for state. I'll have macros for the
>>>>state in next revision as I did that for other cases.
>>>>
>>>>Those macros defined for "address" just for ABI stuff as Alex.G mentioned.
>>>>There isn't corresponding ioctl command for host to get address any more
>>>>because QEMU (user) will have to figure it out by himself. The "address"
>>>>here means PE address and user has to figure it out according to PE
>>>>segmentation.
>>>Why would the user ever need the address?
>>>
>>I will remove those "address" related macros in next revision because it's
>>user-level bussiness, not related to host kernel any more.
>
>Ok, so the next question is whether there will be any state outside
>of GET_MODE left in the future.
>
That's also user-level business and those macros should be removed as well :-)
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Benjamin Herrenschmidt @ 2014-05-28 23:18 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <53866747.50703@suse.de>
On Thu, 2014-05-29 at 00:46 +0200, Alexander Graf wrote:
> I don't think so :). In QEMU the PHB emulation would have to notify the
> "container" (IOMMU emulation layer -> PE) that a PE operation happened.
> It's that emulation code's responsibility to broadcast operations across
> its own emulated operations (recover config space access, reconfigure
> BARs, etc) and the VFIO PE operations.
>
> So from a kernel interface point of view, I think leaving out any
> address information is the right way to go. Whether we managed to get
> all QEMU internal interfaces modeled correctly yet has to be seen on the
> next patch set revision :).
For a kernel interface I absolutely agree, all we care about is the group fd.
>From a qemu internal, I suppose it makes things easier to operate at the PHB
level for now and we can revisit later if the need arises.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 11/16] byteorder: provide a linux/byteorder.h with {be, le}_to_cpu() and cpu_to_{be, le}() macros
From: Cody P Schafer @ 2014-05-28 23:26 UTC (permalink / raw)
To: Joe Perches; +Cc: Sukadev Bhattiprolu, David Laight, Linux PPC, LKML
In-Reply-To: <1401318025.19335.15.camel@joe-AO725>
On Wed, May 28, 2014 at 6:00 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-05-28 at 17:11 -0500, Cody P Schafer wrote:
>> On Wed, May 28, 2014 at 5:05 PM, Cody P Schafer <dev@codyps.com> wrote:
>> > On Wed, May 28, 2014 at 3:45 AM, David Laight <David.Laight@aculab.com> wrote:
>> >> From: Cody P Schafer
>> >>> Rather manually specifying the size of the integer to be converted, key
>> >>> off of the type size. Reduces duplicate size info and the occurance of
>> >>> certain types of bugs (using the wrong sized conversion).
>> >> ...
>> >>> +#define be_to_cpu(v) \
>> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint8_t) , v, \
>> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint16_t), be16_to_cpu(v), \
>> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint32_t), be32_to_cpu(v), \
>> >>> + __builtin_choose_expr(sizeof(v) == sizeof(uint64_t), be64_to_cpu(v), \
>> >>> + (void)0))))
>> >> ...
>> >>
>> >> I'm not at all sure that using the 'size' of the constant will reduce
>> >> the number of bugs - it just introduces a whole new category of bugs.
>> >
>> > Certainly, if you mis-size the argument (and thus have missized one of
>> > the variables containing the be value, probably a bug anyhow), there
>> > will be problems.
>> >
>> > I put this interface together because of an actual bug I wrote into
>> > the initial code of the hv_24x7 driver (resized a struct member
>> > without adjusting the be*_to_cpu() sizing).
>> > Having this "auto sizing" macro means I can avoid encoding the size of
>> > a struct field in multiple places.
>>
>> To clarify, the point I'm making here is that this simply cuts out 1
>> more place we can screw up endianness conversion sizing.
>
> It does screw up other types when you do things like:
>
> u8 foo = some_function();
>
> cpu_to_be(foo + 1);
>
> the return value is sizeof(int) not u8
Yep, that is a very good argument against the cpu_to_{be,le}()
variants. It might make sense to remove them and just have the
{be,le}_to_cpu() ones.
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] powerpc: numa: enable USE_PERCPU_NUMA_NODE_ID
From: Andrew Morton @ 2014-05-28 23:36 UTC (permalink / raw)
To: Nishanth Aravamudan
Cc: Lee Schermerhorn, Christoph Lameter, Mel Gorman, linux-mm,
Anton Blanchard, David Rientjes, Joonsoo Kim, linuxppc-dev
In-Reply-To: <20140528000958.GF4104@linux.vnet.ibm.com>
On Tue, 27 May 2014 17:09:58 -0700 Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote:
> On 28.05.2014 [09:56:14 +1000], Benjamin Herrenschmidt wrote:
> > On Tue, 2014-05-27 at 16:44 -0700, Nishanth Aravamudan wrote:
> > > > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > >
> > > Ping on this and patch 2/2. Ben, would you be willing to pull these
> > > into
> > > your -next branch so they'd get some testing?
> > >
> > > http://patchwork.ozlabs.org/patch/350368/
> > > http://patchwork.ozlabs.org/patch/349838/
> > >
> > > Without any further changes, these two help quite a bit with the slab
> > > consumption on CONFIG_SLUB kernels when memoryless nodes are present.
> >
> > I don't mind at all :-) I haven't really been following that story
> > so I was waiting for the dust to settle and maybe acks from MM people
> > but if you tell me they are good I'm prepared to trust you.
>
> The patches themselves are pretty minimal and similar to the ia64
> changes (and the affected code seems like it hasn't changed in some
> time, beyond in the common code). I'd mostly like to get some
> broad-range build & boot testing.
>
> Also, is NUMA a sufficient symbol to depend, you think? I figure most of
> the PPC NUMA systems are the pSeries/IBM variety, which is where I've
> run into memoryless nodes in the first place.
It's a powerpc-only patchset so I didn't do anything.
Nits:
- When referring to git commits, use 12 digits of hash and include
the name of the patch as well (because patches can have different
hashes in different trees). So
Based on 3bccd996276b ("numa: ia64: use generic percpu
var numa_node_id() implementation") for ia64.
- It's nice to include a diffstat. If there's a [0/n] patch then
that's a great place for it, so people can see the overall impact at
a glance.
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-28 23:37 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <538665DA.8090000@suse.de>
On Thu, May 29, 2014 at 12:40:26AM +0200, Alexander Graf wrote:
>On 28.05.14 18:17, Alex Williamson wrote:
>>On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>>>On 28.05.14 02:57, Alex Williamson wrote:
>>>>On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
>>>>>On 28.05.14 02:39, Alex Williamson wrote:
>>>>>>On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
>>>>>>>On 27.05.14 20:15, Alex Williamson wrote:
>>>>>>>>On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>>>>The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>>>>>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>>>>>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>>>>>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>>>>>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>>>>>[...]
>>>>>>>
>>>>>>>>>+
>>>>>>>>>+ return ret;
>>>>>>>>>+}
>>>>>>>>>+
>>>>>>>>> static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>> unsigned int cmd, unsigned long arg)
>>>>>>>>> {
>>>>>>>>>@@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>> tce_iommu_disable(container);
>>>>>>>>> mutex_unlock(&container->lock);
>>>>>>>>> return 0;
>>>>>>>>>+ case VFIO_EEH_PE_SET_OPTION:
>>>>>>>>>+ case VFIO_EEH_PE_GET_STATE:
>>>>>>>>>+ case VFIO_EEH_PE_RESET:
>>>>>>>>>+ case VFIO_EEH_PE_CONFIGURE:
>>>>>>>>>+ return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>>>>>>>>This is where it would have really made sense to have a single
>>>>>>>>VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>>>>>>>>AlexG, are you really attached to splitting these out into separate
>>>>>>>>ioctls?
>>>>>>>I don't see the problem. We need to forward 4 ioctls to a separate piece
>>>>>>>of code, so we forward 4 ioctls to a separate piece of code :). Putting
>>>>>>>them into one ioctl just moves the switch() into another function.
>>>>>>And uses an extra 3 ioctl numbers and gives us extra things to update if
>>>>>>we ever need to add more ioctls, etc. ioctl numbers are an address
>>>>>>space, how much address space do we really want to give to EEH? It's
>>>>>>not a big difference, but I don't think it's completely even either.
>>>>>>Thanks,
>>>>>Yes, that's the point. I by far prefer to have you push back on anyone
>>>>>who introduces useless ioctls rather than have a separate EEH number
>>>>>space that people can just throw anything in they like ;).
>>>>Well, I appreciate that, but having them as separate ioctls doesn't
>>>>really prevent that either. Any one of these 4 could be set to take a
>>>>sub-option to extend and contort the EEH interface. The only way to
>>>>prevent that would be to avoid the argsz+flags hack that make the ioctl
>>>>extendable. Thanks,
>>>Sure, that's what patch review is about. I'm really more concerned about
>>>whose court the number space is in - you or Gavin. If we're talking
>>>about top level ioctls you will care a lot more.
>>>
>>>But I'm not religious about this. You're the VFIO maintainer, so it's
>>>your call. I just personally cringe when I see an ioctl that gets an
>>>"opcode" and a "parameter" argument where the "parameter" argument is a
>>>union with one struct for each opcode.
>>Well, what would it look like...
>>
>>struct vfio_eeh_pe_op {
>> __u32 argsz;
>> __u32 flags;
>> __u32 op;
>>};
>>
>>Couldn't every single one of these be a separate "op"? Are there any
>>cases where we can't use the ioctl return value?
>>
>>VFIO_EEH_PE_DISABLE
>>VFIO_EEH_PE_ENABLE
>>VFIO_EEH_PE_UNFREEZE_IO
>>VFIO_EEH_PE_UNFREEZE_DMA
>>VFIO_EEH_PE_GET_MODE
>>VFIO_EEH_PE_RESET_DEACTIVATE
>>VFIO_EEH_PE_RESET_HOT
>>VFIO_EEH_PE_RESET_FUNDAMENTAL
>>VFIO_EEH_PE_CONFIGURE
>>
>>It doesn't look that bad to me, what am I missing? Thanks,
>
>Yup, that looks well to me as well :)
>
s/VFIO_EEH_PE_GET_MODE/VFIO_EEH_PE_GET_STATE.
I'll include this in next revision. Thanks, Alex.
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Alexander Graf @ 2014-05-28 23:38 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <20140528233753.GA8150@shangw>
On 29.05.14 01:37, Gavin Shan wrote:
> On Thu, May 29, 2014 at 12:40:26AM +0200, Alexander Graf wrote:
>> On 28.05.14 18:17, Alex Williamson wrote:
>>> On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>>>> On 28.05.14 02:57, Alex Williamson wrote:
>>>>> On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
>>>>>> On 28.05.14 02:39, Alex Williamson wrote:
>>>>>>> On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
>>>>>>>> On 27.05.14 20:15, Alex Williamson wrote:
>>>>>>>>> On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>>>>> The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>>>>>>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>>>>>>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>>>>>>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>>>>>>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> + return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>>> unsigned int cmd, unsigned long arg)
>>>>>>>>>> {
>>>>>>>>>> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>>> tce_iommu_disable(container);
>>>>>>>>>> mutex_unlock(&container->lock);
>>>>>>>>>> return 0;
>>>>>>>>>> + case VFIO_EEH_PE_SET_OPTION:
>>>>>>>>>> + case VFIO_EEH_PE_GET_STATE:
>>>>>>>>>> + case VFIO_EEH_PE_RESET:
>>>>>>>>>> + case VFIO_EEH_PE_CONFIGURE:
>>>>>>>>>> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>>>>>>>>> This is where it would have really made sense to have a single
>>>>>>>>> VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>>>>>>>>> AlexG, are you really attached to splitting these out into separate
>>>>>>>>> ioctls?
>>>>>>>> I don't see the problem. We need to forward 4 ioctls to a separate piece
>>>>>>>> of code, so we forward 4 ioctls to a separate piece of code :). Putting
>>>>>>>> them into one ioctl just moves the switch() into another function.
>>>>>>> And uses an extra 3 ioctl numbers and gives us extra things to update if
>>>>>>> we ever need to add more ioctls, etc. ioctl numbers are an address
>>>>>>> space, how much address space do we really want to give to EEH? It's
>>>>>>> not a big difference, but I don't think it's completely even either.
>>>>>>> Thanks,
>>>>>> Yes, that's the point. I by far prefer to have you push back on anyone
>>>>>> who introduces useless ioctls rather than have a separate EEH number
>>>>>> space that people can just throw anything in they like ;).
>>>>> Well, I appreciate that, but having them as separate ioctls doesn't
>>>>> really prevent that either. Any one of these 4 could be set to take a
>>>>> sub-option to extend and contort the EEH interface. The only way to
>>>>> prevent that would be to avoid the argsz+flags hack that make the ioctl
>>>>> extendable. Thanks,
>>>> Sure, that's what patch review is about. I'm really more concerned about
>>>> whose court the number space is in - you or Gavin. If we're talking
>>>> about top level ioctls you will care a lot more.
>>>>
>>>> But I'm not religious about this. You're the VFIO maintainer, so it's
>>>> your call. I just personally cringe when I see an ioctl that gets an
>>>> "opcode" and a "parameter" argument where the "parameter" argument is a
>>>> union with one struct for each opcode.
>>> Well, what would it look like...
>>>
>>> struct vfio_eeh_pe_op {
>>> __u32 argsz;
>>> __u32 flags;
>>> __u32 op;
>>> };
>>>
>>> Couldn't every single one of these be a separate "op"? Are there any
>>> cases where we can't use the ioctl return value?
>>>
>>> VFIO_EEH_PE_DISABLE
>>> VFIO_EEH_PE_ENABLE
>>> VFIO_EEH_PE_UNFREEZE_IO
>>> VFIO_EEH_PE_UNFREEZE_DMA
>>> VFIO_EEH_PE_GET_MODE
>>> VFIO_EEH_PE_RESET_DEACTIVATE
>>> VFIO_EEH_PE_RESET_HOT
>>> VFIO_EEH_PE_RESET_FUNDAMENTAL
>>> VFIO_EEH_PE_CONFIGURE
>>>
>>> It doesn't look that bad to me, what am I missing? Thanks,
>> Yup, that looks well to me as well :)
>>
> s/VFIO_EEH_PE_GET_MODE/VFIO_EEH_PE_GET_STATE.
>
> I'll include this in next revision. Thanks, Alex.
Yup, no need for CMD anymore then either :)
Alex
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-28 23:41 UTC (permalink / raw)
To: Alexander Graf
Cc: aik, Gavin Shan, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <53867386.9060403@suse.de>
On Thu, May 29, 2014 at 01:38:46AM +0200, Alexander Graf wrote:
>On 29.05.14 01:37, Gavin Shan wrote:
>>On Thu, May 29, 2014 at 12:40:26AM +0200, Alexander Graf wrote:
>>>On 28.05.14 18:17, Alex Williamson wrote:
>>>>On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>>>>>On 28.05.14 02:57, Alex Williamson wrote:
>>>>>>On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
>>>>>>>On 28.05.14 02:39, Alex Williamson wrote:
>>>>>>>>On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
>>>>>>>>>On 27.05.14 20:15, Alex Williamson wrote:
>>>>>>>>>>On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>>>>>>>>>>>The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>>>>>>>>>>> drivers/vfio/pci/Makefile | 1 +
>>>>>>>>>>> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>>>>>>>>>>> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>>>>>>>>>>> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>>>>>>>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>>>>>>>>>>> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>>>>>>>>>>> 7 files changed, 308 insertions(+), 7 deletions(-)
>>>>>>>>>>> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>>>>>>>>>[...]
>>>>>>>>>
>>>>>>>>>>>+
>>>>>>>>>>>+ return ret;
>>>>>>>>>>>+}
>>>>>>>>>>>+
>>>>>>>>>>> static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>>>> unsigned int cmd, unsigned long arg)
>>>>>>>>>>> {
>>>>>>>>>>>@@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>>>>>>>> tce_iommu_disable(container);
>>>>>>>>>>> mutex_unlock(&container->lock);
>>>>>>>>>>> return 0;
>>>>>>>>>>>+ case VFIO_EEH_PE_SET_OPTION:
>>>>>>>>>>>+ case VFIO_EEH_PE_GET_STATE:
>>>>>>>>>>>+ case VFIO_EEH_PE_RESET:
>>>>>>>>>>>+ case VFIO_EEH_PE_CONFIGURE:
>>>>>>>>>>>+ return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>>>>>>>>>>This is where it would have really made sense to have a single
>>>>>>>>>>VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>>>>>>>>>>AlexG, are you really attached to splitting these out into separate
>>>>>>>>>>ioctls?
>>>>>>>>>I don't see the problem. We need to forward 4 ioctls to a separate piece
>>>>>>>>>of code, so we forward 4 ioctls to a separate piece of code :). Putting
>>>>>>>>>them into one ioctl just moves the switch() into another function.
>>>>>>>>And uses an extra 3 ioctl numbers and gives us extra things to update if
>>>>>>>>we ever need to add more ioctls, etc. ioctl numbers are an address
>>>>>>>>space, how much address space do we really want to give to EEH? It's
>>>>>>>>not a big difference, but I don't think it's completely even either.
>>>>>>>>Thanks,
>>>>>>>Yes, that's the point. I by far prefer to have you push back on anyone
>>>>>>>who introduces useless ioctls rather than have a separate EEH number
>>>>>>>space that people can just throw anything in they like ;).
>>>>>>Well, I appreciate that, but having them as separate ioctls doesn't
>>>>>>really prevent that either. Any one of these 4 could be set to take a
>>>>>>sub-option to extend and contort the EEH interface. The only way to
>>>>>>prevent that would be to avoid the argsz+flags hack that make the ioctl
>>>>>>extendable. Thanks,
>>>>>Sure, that's what patch review is about. I'm really more concerned about
>>>>>whose court the number space is in - you or Gavin. If we're talking
>>>>>about top level ioctls you will care a lot more.
>>>>>
>>>>>But I'm not religious about this. You're the VFIO maintainer, so it's
>>>>>your call. I just personally cringe when I see an ioctl that gets an
>>>>>"opcode" and a "parameter" argument where the "parameter" argument is a
>>>>>union with one struct for each opcode.
>>>>Well, what would it look like...
>>>>
>>>>struct vfio_eeh_pe_op {
>>>> __u32 argsz;
>>>> __u32 flags;
>>>> __u32 op;
>>>>};
>>>>
>>>>Couldn't every single one of these be a separate "op"? Are there any
>>>>cases where we can't use the ioctl return value?
>>>>
>>>>VFIO_EEH_PE_DISABLE
>>>>VFIO_EEH_PE_ENABLE
>>>>VFIO_EEH_PE_UNFREEZE_IO
>>>>VFIO_EEH_PE_UNFREEZE_DMA
>>>>VFIO_EEH_PE_GET_MODE
>>>>VFIO_EEH_PE_RESET_DEACTIVATE
>>>>VFIO_EEH_PE_RESET_HOT
>>>>VFIO_EEH_PE_RESET_FUNDAMENTAL
>>>>VFIO_EEH_PE_CONFIGURE
>>>>
>>>>It doesn't look that bad to me, what am I missing? Thanks,
>>>Yup, that looks well to me as well :)
>>>
>>s/VFIO_EEH_PE_GET_MODE/VFIO_EEH_PE_GET_STATE.
>>
>>I'll include this in next revision. Thanks, Alex.
>
>Yup, no need for CMD anymore then either :)
>
Yep. Thanks, Guys :)
Thanks,
Gavin
^ permalink raw reply
* Re: [RFC PATCH v2 1/2] powerpc: numa: enable USE_PERCPU_NUMA_NODE_ID
From: Nishanth Aravamudan @ 2014-05-28 23:43 UTC (permalink / raw)
To: Andrew Morton
Cc: Lee Schermerhorn, Christoph Lameter, Mel Gorman, linux-mm,
Anton Blanchard, David Rientjes, Joonsoo Kim, linuxppc-dev
In-Reply-To: <20140528163607.e1f0ced83a5b736cca7fa530@linux-foundation.org>
On 28.05.2014 [16:36:07 -0700], Andrew Morton wrote:
> On Tue, 27 May 2014 17:09:58 -0700 Nishanth Aravamudan <nacc@linux.vnet.ibm.com> wrote:
>
> > On 28.05.2014 [09:56:14 +1000], Benjamin Herrenschmidt wrote:
> > > On Tue, 2014-05-27 at 16:44 -0700, Nishanth Aravamudan wrote:
> > > > > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > > >
> > > > Ping on this and patch 2/2. Ben, would you be willing to pull these
> > > > into
> > > > your -next branch so they'd get some testing?
> > > >
> > > > http://patchwork.ozlabs.org/patch/350368/
> > > > http://patchwork.ozlabs.org/patch/349838/
> > > >
> > > > Without any further changes, these two help quite a bit with the slab
> > > > consumption on CONFIG_SLUB kernels when memoryless nodes are present.
> > >
> > > I don't mind at all :-) I haven't really been following that story
> > > so I was waiting for the dust to settle and maybe acks from MM people
> > > but if you tell me they are good I'm prepared to trust you.
> >
> > The patches themselves are pretty minimal and similar to the ia64
> > changes (and the affected code seems like it hasn't changed in some
> > time, beyond in the common code). I'd mostly like to get some
> > broad-range build & boot testing.
> >
> > Also, is NUMA a sufficient symbol to depend, you think? I figure most of
> > the PPC NUMA systems are the pSeries/IBM variety, which is where I've
> > run into memoryless nodes in the first place.
>
> It's a powerpc-only patchset so I didn't do anything.
>
> Nits:
>
> - When referring to git commits, use 12 digits of hash and include
> the name of the patch as well (because patches can have different
> hashes in different trees). So
>
> Based on 3bccd996276b ("numa: ia64: use generic percpu
> var numa_node_id() implementation") for ia64.
>
> - It's nice to include a diffstat. If there's a [0/n] patch then
> that's a great place for it, so people can see the overall impact at
> a glance.
Thanks for the notes, I'll include those in any updated patches.
-Nish
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Gavin Shan @ 2014-05-29 0:05 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, Gavin Shan, kvm-ppc, agraf, qiudayu, linuxppc-dev
In-Reply-To: <1401294731.2412.42.camel@ul30vt.home>
On Wed, May 28, 2014 at 10:32:11AM -0600, Alex Williamson wrote:
>On Wed, 2014-05-28 at 10:55 +1000, Gavin Shan wrote:
>> On Tue, May 27, 2014 at 12:15:27PM -0600, Alex Williamson wrote:
>> >On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
>> >> The patch adds new IOCTL commands for sPAPR VFIO container 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 | 92 ++++++++++++++++++++++++++++++++++++-
>> >> drivers/vfio/pci/Makefile | 1 +
>> >> drivers/vfio/pci/vfio_pci.c | 20 +++++---
>> >> drivers/vfio/pci/vfio_pci_eeh.c | 46 +++++++++++++++++++
>> >> drivers/vfio/pci/vfio_pci_private.h | 5 ++
>> >> drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
>> >> include/uapi/linux/vfio.h | 66 ++++++++++++++++++++++++++
>> >> 7 files changed, 308 insertions(+), 7 deletions(-)
>> >> create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
>> >>
>> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
>> >> index b9ca023..d890fed 100644
>> >> --- a/Documentation/vfio.txt
>> >> +++ b/Documentation/vfio.txt
>> >> @@ -305,7 +305,15 @@ 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) According to sPAPR specification, A Partitionable Endpoint (PE) is an I/O
>> >> +subtree that can be treated as a unit for the purposes of partitioning and
>> >> +error recovery. A PE may be a single or multi-function IOA (IO Adapter), a
>> >> +function of a multi-function IOA, or multiple IOAs (possibly including switch
>> >> +and bridge structures above the multiple IOAs). PPC64 guests detect PCI errors
>> >> +and recover from them via EEH RTAS services, which works on the basis of
>> >> +additional ioctl commands.
>> >> +
>> >> +So 7 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 +324,17 @@ So 3 additional ioctls have been added:
>> >>
>> >> VFIO_IOMMU_DISABLE - disables the container.
>> >>
>> >> + VFIO_EEH_PE_SET_OPTION - enables or disables EEH functionality on the
>> >> + specified device. Also, it can be used to remove IO or DMA
>> >> + stopped state on the frozen 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 +365,77 @@ 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_state state = { .argsz = sizeof(state) };
>> >> + struct vfio_eeh_pe_reset reset = { .argsz = sizeof(reset) };
>> >> + struct vfio_eeh_pe_configure configure = { .argsz = sizeof(configure) };
>> >> +
>> >> + ....
>> >> +
>> >> + /* 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 = VFIO_EEH_PE_SET_OPT_ENABLE;
>> >> + ioctl(container, VFIO_EEH_PE_SET_OPTION, &option);
>> >> +
>> >> + /* You're suggested to create additional data struct to represent
>> >> + * PE, and put child devices belonging to same IOMMU group to the
>> >> + * PE instance for later reference.
>> >> + */
>> >> +
>> >> + /* Check the PE's state and make sure it's in functional state */
>> >> + ioctl(container, 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(container, 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 = VFIO_EEH_PE_SET_OPT_IO;
>> >> + ioctl(container, VFIO_EEH_PE_SET_OPTION, &option);
>> >> +
>> >> + /* Issue PE reset */
>> >> + reset.option = VFIO_EEH_PE_RESET_HOT;
>> >> + ioctl(container, VFIO_EEH_PE_RESET, &reset);
>> >> + reset.option = VFIO_EEH_PE_RESET_DEACTIVATE;
>> >> + ioctl(container, VFIO_EEH_PE_RESET, &reset);
>> >> +
>> >> + /* Configure the PCI bridges for the affected PE */
>> >> + ioctl(container, VFIO_EEH_PE_CONFIGURE, &configure);
>> >> +
>> >> + /* 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/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>> >> index 1310792..faad885 100644
>> >> --- a/drivers/vfio/pci/Makefile
>> >> +++ b/drivers/vfio/pci/Makefile
>> >> @@ -1,4 +1,5 @@
>> >>
>> >> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>> >> +vfio-pci-y += vfio_pci_eeh.o
>> >
>> >Why do we build this w/o CONFIG_EEH? Can't we define the functions as
>> >static inline in the header in that case?
>> >
>>
>> Ok. Will do in next revision.
>>
>> >> obj-$(CONFIG_VFIO_PCI) += vfio-pci.o
>> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> >> index 7ba0424..7c8d26a 100644
>> >> --- a/drivers/vfio/pci/vfio_pci.c
>> >> +++ b/drivers/vfio/pci/vfio_pci.c
>> >> @@ -156,8 +156,10 @@ 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_pci_eeh_release(vdev->pdev);
>> >> vfio_pci_disable(vdev);
>> >> + }
>> >>
>> >> module_put(THIS_MODULE);
>> >> }
>> >> @@ -165,19 +167,25 @@ static void vfio_pci_release(void *device_data)
>> >> 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_pci_eeh_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)
>> >> diff --git a/drivers/vfio/pci/vfio_pci_eeh.c b/drivers/vfio/pci/vfio_pci_eeh.c
>> >> new file mode 100644
>> >> index 0000000..9c25207
>> >> --- /dev/null
>> >> +++ b/drivers/vfio/pci/vfio_pci_eeh.c
>> >> @@ -0,0 +1,46 @@
>> >> +/*
>> >> + * EEH functionality support for VFIO PCI devices.
>> >> + *
>> >> + * Copyright Gavin Shan, IBM Corporation 2014.
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or modify
>> >> + * it under the terms of the GNU General Public License version 2 as
>> >> + * published by the Free Software Foundation.
>> >> + */
>> >> +
>> >> +#include <linux/device.h>
>> >> +#include <linux/eventfd.h>
>> >> +#include <linux/file.h>
>> >> +#include <linux/interrupt.h>
>> >> +#include <linux/iommu.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/notifier.h>
>> >> +#include <linux/pci.h>
>> >> +#include <linux/pm_runtime.h>
>> >> +#include <linux/slab.h>
>> >> +#include <linux/types.h>
>> >> +#include <linux/uaccess.h>
>> >> +#include <linux/vfio.h>
>> >
>> >Cleanup the includes
>> >
>>
>> Ok. Will do in next revision.
>>
>> >> +#ifdef CONFIG_EEH
>> >> +#include <asm/eeh.h>
>> >> +#endif
>> >
>> >This shouldn't even be compiles w/o CONFIG_EEH
>> >
>>
>> Yeah. Will fix in next revision as you suggested.
>>
>> >> +
>> >> +#include "vfio_pci_private.h"
>> >> +
>> >> +int vfio_pci_eeh_open(struct pci_dev *pdev)
>> >> +{
>> >> + int ret = 0;
>> >> +
>> >> +#ifdef CONFIG_EEH
>> >> + ret = eeh_dev_open(pdev);
>> >> +#endif
>> >> + return ret;
>> >> +}
>> >> +
>> >> +void vfio_pci_eeh_release(struct pci_dev *pdev)
>> >> +{
>> >> +#ifdef CONFIG_EEH
>> >> + eeh_dev_release(pdev);
>> >> +#endif
>> >> +}
>> >> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
>> >> index 9c6d5d0..c3cbe40 100644
>> >> --- a/drivers/vfio/pci/vfio_pci_private.h
>> >> +++ b/drivers/vfio/pci/vfio_pci_private.h
>> >> @@ -90,4 +90,9 @@ extern void vfio_pci_virqfd_exit(void);
>> >>
>> >> extern int vfio_config_init(struct vfio_pci_device *vdev);
>> >> extern void vfio_config_free(struct vfio_pci_device *vdev);
>> >> +
>> >> +/* EEH stub */
>> >> +extern int vfio_pci_eeh_open(struct pci_dev *pdev);
>> >> +extern void vfio_pci_eeh_release(struct pci_dev *pdev);
>> >
>> >The #ifdef with static inlines should be here.
>> >
>>
>> thanks for the detailed comments.
>>
>> >> +
>> >> #endif /* VFIO_PCI_PRIVATE_H */
>> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> index a84788b..666691b 100644
>> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> >> @@ -21,6 +21,9 @@
>> >> #include <linux/vfio.h>
>> >> #include <asm/iommu.h>
>> >> #include <asm/tce.h>
>> >> +#ifdef CONFIG_EEH
>> >> +#include <asm/eeh.h>
>> >> +#endif
>> >
>> >Now we're infecting another file with EEH, can't we put it in a central
>> >location for vfio?
>>
>> How about something like this:
>>
>> - Create drivers/vfio/vfio_eeh.c and it will be built when having CONFIG_EEH
>> - Declare all EEH functions in include/linux/vfio.h (it's the only central
>> header file I can find except you have a better one).
>>
>> #ifdef CONFIG_EEH
>> extern int vfio_pci_eeh_open(struct pci_dev *pdev);
>> extern void vfio_pci_eeh_release(struct pci_dev *pdev);
>> extern long vfio_iommu_spapr_eeh_ioctl(struct iommu_table *tbl,
>> unsigned int cmd, unsigned long arg);
>> #else
>> static int vfio_pci_eeh_open(struct pci_dev *pdev)
>> {
>> return 0;
>> }
>>
>> static void vfio_pci_eeh_release(struct pci_dev *pdev)
>> {
>> }
>>
>> static inline vfio_iommu_spapr_eeh_ioctl(struct iommu_table *tbl,
>> unsigned int cmd, unsigned long arg);
>> {
>> return -ENOTTY;
>> }
>> #endif
>
>Seems like an improvement
>
So I'll include the change in next revision.
>> >>
>> >> #define DRIVER_VERSION "0.1"
>> >> #define DRIVER_AUTHOR "aik@ozlabs.ru"
>> >> @@ -147,6 +150,83 @@ static void tce_iommu_release(void *iommu_data)
>> >> kfree(container);
>> >> }
>> >>
>> >> +static long tce_iommu_eeh_ioctl(void *iommu_data,
>> >> + unsigned int cmd, unsigned long arg)
>> >> +{
>> >> + struct tce_container *container = iommu_data;
>> >> + 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(eeh_iommu_table_to_pe(container->tbl),
>> >> + option.option);
>> >> + 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(eeh_iommu_table_to_pe(container->tbl));
>> >> + if (ret >= 0) {
>> >> + state.state = ret;
>> >> + if (copy_to_user((void __user *)arg, &state, minsz))
>> >> + return -EFAULT;
>> >> + ret = 0;
>> >> + }
>> >
>> >This looks like one of those cases where you should just use the ioctl
>> >return value.
>> >
>>
>> Ok. I'll use ioctl return value to carry output information. Note it might
>> have "0" as the output information.
>
>I think that's fine.
>
>
Ok. Thanks. I'll going to have "combined" ioctl command as you just suggested.
Also to use the return value from ioctl() for informtion carriage.
>> >> + 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(eeh_iommu_table_to_pe(container->tbl),
>> >> + reset.option);
>> >> + break;
>> >> + }
>> >> + case VFIO_EEH_PE_CONFIGURE: {
>> >> + struct vfio_eeh_pe_configure configure;
>> >> +
>> >> + minsz = offsetofend(struct vfio_eeh_pe_configure, flags);
>> >> + if (copy_from_user(&configure, (void __user *)arg, minsz))
>> >> + return -EFAULT;
>> >> + if (configure.argsz < minsz)
>> >> + return -EINVAL;
>> >> +
>> >> + ret = eeh_pe_configure(eeh_iommu_table_to_pe(container->tbl));
>> >> + break;
>> >> + }
>> >> + default:
>> >> + ret = -EINVAL;
>> >> + pr_debug("%s: Cannot handle command %d\n",
>> >> + __func__, cmd);
>> >> + }
>> >> +#else
>> >> + ret = -ENOENT;
>> >> +#endif
>> >
>> >Hmm, more like a BUG in the default case the way it's coded here (not
>> >even sure it's worth the default entry). The #else case should probably
>> >be -ENOTTY like other unimplemented ioctls.
>> >
>>
>> Yeah, we don't need default case here and I'll remove it in next revision.
>> Also, "-ENOTTY" will be used in future for unimplemented functions.
>>
>> >> +
>> >> + return ret;
>> >> +}
>> >> +
>> >> static long tce_iommu_ioctl(void *iommu_data,
>> >> unsigned int cmd, unsigned long arg)
>> >> {
>> >> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
>> >> tce_iommu_disable(container);
>> >> mutex_unlock(&container->lock);
>> >> return 0;
>> >> + case VFIO_EEH_PE_SET_OPTION:
>> >> + case VFIO_EEH_PE_GET_STATE:
>> >> + case VFIO_EEH_PE_RESET:
>> >> + case VFIO_EEH_PE_CONFIGURE:
>> >> + return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
>> >
>> >This is where it would have really made sense to have a single
>> >VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
>> >AlexG, are you really attached to splitting these out into separate
>> >ioctls?
>> >
>>
>> I really don't know. Alex.G want separate ioctl commands, but you
>> suggested to combine them. Could you guys please just tell me which
>> one (separate vs combined) I need to have in next revision? :-)
>>
>> >> }
>> >>
>> >> return -ENOTTY;
>> >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> >> index cb9023d..c5fac36 100644
>> >> --- a/include/uapi/linux/vfio.h
>> >> +++ b/include/uapi/linux/vfio.h
>> >> @@ -455,6 +455,72 @@ 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 flags;
>> >> + __u32 option;
>> >> +#define VFIO_EEH_PE_SET_OPT_DISABLE 0 /* Disable EEH */
>> >> +#define VFIO_EEH_PE_SET_OPT_ENABLE 1 /* Enable EEH */
>> >> +#define VFIO_EEH_PE_SET_OPT_IO 2 /* Enable IO */
>> >> +#define VFIO_EEH_PE_SET_OPT_DMA 3 /* Enable DMA */
>> >
>> >This is more of a "command" than an "option" isn't it? Each of these
>> >probably needs a more significant description.
>> >
>>
>> Yeah, it would be regarded as "opcode" and I'll add more description about
>> them in next revision.
>>
>> >> +};
>> >> +
>> >> +#define VFIO_EEH_PE_SET_OPTION _IO(VFIO_TYPE, VFIO_BASE + 21)
>> >> +
>> >> +/*
>> >> + * Each EEH PE should have unique address to be identified. PE's
>> >> + * sharing mode is also useful information as well.
>> >> + */
>> >> +#define VFIO_EEH_PE_GET_ADDRESS 0 /* Get address */
>> >> +#define VFIO_EEH_PE_GET_MODE 1 /* Query mode */
>> >> +#define VFIO_EEH_PE_MODE_NONE 0 /* Not a PE */
>> >> +#define VFIO_EEH_PE_MODE_NOT_SHARED 1 /* Exclusive */
>> >> +#define VFIO_EEH_PE_MODE_SHARED 2 /* Shared mode */
>> >> +
>> >> +/*
>> >> + * 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 flags;
>> >> + __u32 state;
>> >> +};
>> >
>> >Should state be a union to better describe the value returned? What
>> >exactly is the address and why does the user need to know it? Does this
>> >need user input or could we just return the address and mode regardless?
>> >
>>
>> Ok. I think you want enum (not union) for state. I'll have macros for the
>> state in next revision as I did that for other cases.
>>
>> Those macros defined for "address" just for ABI stuff as Alex.G mentioned.
>> There isn't corresponding ioctl command for host to get address any more
>> because QEMU (user) will have to figure it out by himself. The "address"
>> here means PE address and user has to figure it out according to PE
>> segmentation.
>>
>> >> +
>> >> +#define VFIO_EEH_PE_GET_STATE _IO(VFIO_TYPE, VFIO_BASE + 22)
>> >> +
>> >> +/*
>> >> + * Reset is the major step to recover problematic PE. The following
>> >> + * command helps on that.
>> >> + */
>> >> +struct vfio_eeh_pe_reset {
>> >> + __u32 argsz;
>> >> + __u32 flags;
>> >> + __u32 option;
>> >> +#define VFIO_EEH_PE_RESET_DEACTIVATE 0 /* Deactivate reset */
>> >> +#define VFIO_EEH_PE_RESET_HOT 1 /* Hot reset */
>> >> +#define VFIO_EEH_PE_RESET_FUNDAMENTAL 3 /* Fundamental reset */
>> >
>> >How does a user know which of these to use?
>> >
>>
>> I think Ben already helped explaining it for a lot in the subsequent
>> replies. Thanks to Ben :-)
>>
>> >> +};
>> >> +
>> >> +#define VFIO_EEH_PE_RESET _IO(VFIO_TYPE, VFIO_BASE + 23)
>> >> +
>> >> +/*
>> >> + * One of the steps for recovery after PE reset is to configure the
>> >> + * PCI bridges affected by the PE reset.
>> >> + */
>> >> +struct vfio_eeh_pe_configure {
>> >> + __u32 argsz;
>> >> + __u32 flags;
>> >> +};
>> >
>> >Parameters are probably not necessary here.
>> >
>>
>> Yep. I'll remove it in next revision. Thanks for your comments, Alex.
>>
>> >> +
>> >> +#define VFIO_EEH_PE_CONFIGURE _IO(VFIO_TYPE, VFIO_BASE + 24)
>> >> +
>> >> /* ***************************************************************** */
>> >>
>> >> #endif /* _UAPIVFIO_H */
>> >
>> >How does a user learn that a device supports EEH? Do they just start
>> >making ioctl calls and getting a failure? Shouldn't we make use of one
>> >of the flag bits on the device or add a capability on the container for
>> >the user to query?
>> >
>>
>> User needs to make some ioctl calls to make sure EEH can be supported on
>> one specific device:
>>
>> struct vfio_eeh_pe_set_option set_option;
>>
>> /* User have to make sure the device isn't a bridge and there
>> * has one PE for it, which means that case of VFIO_EEH_PE_MODE_NONE.
>> */
>>
>> set_option.argsz = sizeof(set_option);
>> set_option.option = VFIO_EEH_PE_SET_OPT_ENABLE;
>> ret = ioctl(container, VFIO_EEH_PE_SET_OPTION, &set_option);
>> if (ret < 0) {
>> /* EEH can't supported */
>> }
>
>No, the user should be able to query whether it's available with
>VFIO_CHECK_EXTENSION.
>
Sounds like a good idea. If you agree, I'll have one more extention option
and use it in tce_iommu_ioctl():
#define VFIO_EEH_SUPPORT 5
>I think I'm also still looking for an explanation of how the user learns
>about the error that occurred, not just that it occurred, but what it
>was. The description of the recovery process indicates a lot retrieval
>step, but there's no interface here for the user to get logs. Is that a
>TBD? Thanks,
>
The log stuff is TBD and I'll figure it out later.
About to what are the errors, there are a lot. Most of them are related
to hardware level, for example unstable PCI link. Usually, those error
bits defined in AER fatal error state register contribute to EEH errors.
It could be software related, e.g. violating IOMMU protection (read/write
permission etc), or even one PCI device isn't capable of DMAing. Hopefully,
it's the explaination you're looking for? :-)
Thanks,
Gavin
^ permalink raw reply
* Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device
From: Benjamin Herrenschmidt @ 2014-05-29 0:44 UTC (permalink / raw)
To: Gavin Shan; +Cc: aik, agraf, kvm-ppc, Alex Williamson, qiudayu, linuxppc-dev
In-Reply-To: <20140529000543.GA9645@shangw>
On Thu, 2014-05-29 at 10:05 +1000, Gavin Shan wrote:
> The log stuff is TBD and I'll figure it out later.
>
> About to what are the errors, there are a lot. Most of them are related
> to hardware level, for example unstable PCI link. Usually, those error
> bits defined in AER fatal error state register contribute to EEH errors.
> It could be software related, e.g. violating IOMMU protection (read/write
> permission etc), or even one PCI device isn't capable of DMAing. Hopefully,
> it's the explaination you're looking for? :-)
Note to Alex('s) ...
The log we get from FW at the moment in the host is:
- In the case of pHyp / RTAS host, opaque. Basically it's a blob that we store
and that can be sent to IBM service people :-) Not terribly practical.
- On PowerNV, it's a IODA specific data structure (basically a dump of a
bunch of register state and tables). IODA is our IO architecture (sadly the
document itself isn't public at this point) and we have two versions, IODA1
and IODA2. You can consider the structure as chipset specific basically.
What I want to do in the long run is:
- In the case of pHyp/RTAS host, there's not much we can do, so basically
forward that log as-is.
- In the case of PowerNV, forward the log *and* add a well-defined blob to
it that does some basic interpretation of it. In fact I want to do the latter
more generally in the host kernel for host kernel errors as well, but we
can forward it to the guest via VFIO too. What I mean by interpretation is
something along the lines of an error type "DMA IOMMU fault, MMIO error,
Link loss, PCIe UE, ..." among a list of well known error types that
represent the most common ones, with a little bit of added info when
available (for most DMA errors we can provide the DMA address that faulted
for example).
So Gavin and I need to work a bit on that, both in the context of the host
kernel to improve the reporting there, and in the context of what we pass to
user space.
However, no driver today cares about that information. The PCIe error recovery
system doesn't carry it and it has no impact on the EEH recovery procedures,
so EEH in that sense is useful without that reporting. It's useful for the
programmer (or user/admin) to identify what went wrong but it's not used by
the automated recovery process.
One last thing to look at is in the case of a VFIO device, we might want to
silence the host kernel printf's once we support guest EEH since otherwise
the guest has a path to flood the host kernel log by triggering a lot of
EEH errors purposefully.
Cheers,
Ben.
^ permalink raw reply
* linux-next: manual merge of the powerpc tree with Linus' tree
From: Stephen Rothwell @ 2014-05-29 2:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev, Alexander Graf
Cc: linux-next, linux-kernel, Anton Blanchard
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
Hi all,
Today's linux-next merge of the powerpc tree got a conflict in
arch/powerpc/include/asm/sections.h between commit b18db0b80867 ("KVM
guest: Make pv trampoline code executable") from the tree and commit
07de8377f748 ("powerpc: Fix ABIv2 issue with
dereference_function_descriptor") from the powerpc tree.
I fixed it up (see below) and can carry the fix as necessary (no action
is required).
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
diff --cc arch/powerpc/include/asm/sections.h
index 521790330672,d1bb96d5a298..000000000000
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@@ -39,17 -39,7 +39,18 @@@ static inline int overlaps_kernel_text(
(unsigned long)_stext < end;
}
+static inline int overlaps_kvm_tmp(unsigned long start, unsigned long end)
+{
+#ifdef CONFIG_KVM_GUEST
+ extern char kvm_tmp[];
+ return start < (unsigned long)kvm_tmp &&
+ (unsigned long)&kvm_tmp[1024 * 1024] < end;
+#else
+ return 0;
+#endif
+}
+
+ #if !defined(_CALL_ELF) || _CALL_ELF != 2
#undef dereference_function_descriptor
static inline void *dereference_function_descriptor(void *ptr)
{
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: Kernel 3.15: Boot problems with a PA6T board
From: Michael Ellerman @ 2014-05-29 2:48 UTC (permalink / raw)
To: Christian Zigotzky; +Cc: linuxppc-dev
In-Reply-To: <5385C7A3.1080508@xenosoft.de>
On Wed, 2014-05-28 at 13:25 +0200, Christian Zigotzky wrote:
> On 28.05.2014 10:53, Christian Zigotzky wrote:
> > Hi Michael,
> >
> > Thank you for your answer and thank you for your help. :-)
> >
> > On 28.05.2014 06:23, Michael Ellerman wrote:
> >> On Wed, 2014-05-28 at 01:08 +0200, Christian Zigotzky wrote:
> >>
> >> I'm going to guess that cd427485357c0c4b99f69719251baacf25946e11 is
> >> BAD. Can
> >> you please confirm or deny that?
>
> > It's not BAD. It boots.
Hmm, interesting.
> >> If cd42748 is *good*, then you'll need to do a bigger bisect from
> >> d8ff9cd to
> >> 18a1a7a.
> OK :-)
>
> -> git clone
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git linux2-git
> -> git bisect start
> -> git bisect good d8ff9cdf68fd119d491f3de90e1a612afc2f3b2b
> -> git bisect bad 18a1a7a1d862ae0794a0179473d08a414dd49234
>
> Output:
> Bisecting: 5900 revisions left to test after this (roughly 13 steps)
> [cb1595563880a81dab6eab9a5ecb4520d2e76077] Merge tag 'tty-3.15-rc1' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
>
> Unfortunately it doesn't boot. :-(
OK. So you do:
$ git bisect bad
And it will pick a new commit for you to test. Repeat that ~13 times and you
should have identified the bad commit.
cheers
^ permalink raw reply
* powerpc/pseries: Use new defines when calling h_set_mode
From: Michael Neuling @ 2014-05-29 7:45 UTC (permalink / raw)
To: Alexander Graf
Cc: linuxppc-dev, Paul Mackerras, kvm@vger.kernel.org,
kvm-ppc@vger.kernel.org
In-Reply-To: <BD1CF085-FEBC-4208-AD3F-C55DB002097F@suse.de>
> > +/* Values for 2nd argument to H_SET_MODE */
> > +#define H_SET_MODE_RESOURCE_SET_CIABR 1
> > +#define H_SET_MODE_RESOURCE_SET_DAWR 2
> > +#define H_SET_MODE_RESOURCE_ADDR_TRANS_MODE 3
> > +#define H_SET_MODE_RESOURCE_LE 4
>=20
>=20
> Much better, but I think you want to make use of these in non-kvm code to=
o,
> no? At least the LE one is definitely already implemented as call :)
Sure but that's a different patch.... below.
Mikey
powerpc/pseries: Use new defines when calling h_set_mode
Now that we define these in the KVM code, use these defines when we call
h_set_mode. No functional change.
Signed-off-by: Michael Neuling <mikey@neuling.org>
--
This depends on the KVM h_set_mode patches.
diff --git a/arch/powerpc/include/asm/plpar_wrappers.h b/arch/powerpc/inclu=
de/asm/plpar_wrappers.h
index 12c32c5..67859ed 100644
--- a/arch/powerpc/include/asm/plpar_wrappers.h
+++ b/arch/powerpc/include/asm/plpar_wrappers.h
@@ -273,7 +273,7 @@ static inline long plpar_set_mode(unsigned long mflags,=
unsigned long resource,
static inline long enable_reloc_on_exceptions(void)
{
/* mflags =3D 3: Exceptions at 0xC000000000004000 */
- return plpar_set_mode(3, 3, 0, 0);
+ return plpar_set_mode(3, H_SET_MODE_RESOURCE_ADDR_TRANS_MODE, 0, 0);
}
=20
/*
@@ -284,7 +284,7 @@ static inline long enable_reloc_on_exceptions(void)
* returns H_SUCCESS.
*/
static inline long disable_reloc_on_exceptions(void) {
- return plpar_set_mode(0, 3, 0, 0);
+ return plpar_set_mode(0, H_SET_MODE_RESOURCE_ADDR_TRANS_MODE, 0, 0);
}
=20
/*
@@ -297,7 +297,7 @@ static inline long disable_reloc_on_exceptions(void) {
static inline long enable_big_endian_exceptions(void)
{
/* mflags =3D 0: big endian exceptions */
- return plpar_set_mode(0, 4, 0, 0);
+ return plpar_set_mode(0, H_SET_MODE_RESOURCE_LE, 0, 0);
}
=20
/*
@@ -310,17 +310,17 @@ static inline long enable_big_endian_exceptions(void)
static inline long enable_little_endian_exceptions(void)
{
/* mflags =3D 1: little endian exceptions */
- return plpar_set_mode(1, 4, 0, 0);
+ return plpar_set_mode(1, H_SET_MODE_RESOURCE_LE, 0, 0);
}
=20
static inline long plapr_set_ciabr(unsigned long ciabr)
{
- return plpar_set_mode(0, 1, ciabr, 0);
+ return plpar_set_mode(0, H_SET_MODE_RESOURCE_SET_CIABR, ciabr, 0);
}
=20
static inline long plapr_set_watchpoint0(unsigned long dawr0, unsigned lon=
g dawrx0)
{
- return plpar_set_mode(0, 2, dawr0, dawrx0);
+ return plpar_set_mode(0, H_SET_MODE_RESOURCE_SET_DAWR, dawr0, dawrx0);
}
=20
#endif /* _ASM_POWERPC_PLPAR_WRAPPERS_H */
^ permalink raw reply related
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