RE: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Tuesday, January 29, 2013 5:25 AM
> To: Pandarathil, Vijaymohan R
> Cc: Gleb Natapov; Bjorn Helgaas; Blue Swirl; Ortiz, Lance E;
> k...@vger.kernel.org; qemu-de...@nongnu.org; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for
> supporting AER
>
> On Mon, 2013-01-28 at 12:31 -0700, Alex Williamson wrote:
> > On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
> > > - New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
> when
> > > an error occurs in the vfio_pci_device
> > >
> > > - Register pci_error_handler for the vfio_pci driver
> > >
> > > - When the device encounters an error, the error handler registered
> by
> > > the vfio_pci driver gets invoked by the AER infrastructure
> > >
> > > - In the error handler, signal the eventfd registered for the device.
> > >
> > > - This results in the qemu eventfd handler getting invoked and
> > > appropriate action taken for the guest.
> > >
> > > Signed-off-by: Vijay Mohan Pandarathil
> > > ---
> > > drivers/vfio/pci/vfio_pci.c | 44
> -
> > > drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
> > > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > > include/uapi/linux/vfio.h | 3 +++
> > > 4 files changed, 79 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index b28e66c..ff2a078 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct
> vfio_pci_device *vdev, int irq_type)
> > >
> > > return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> > > }
> > > - }
> > > + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
> > > + if (pci_is_pcie(vdev->pdev))
> > > + return 1;
> > >
> > > return 0;
> > > }
> > > @@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
> > > if (vdev->reset_works)
> > > info.flags |= VFIO_DEVICE_FLAGS_RESET;
> > >
> > > + if (pci_is_pcie(vdev->pdev)) {
> > > + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
> > > + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
> >
> > Not sure this second flag should be AER specific or if it's even needed,
> > see below for more comments on this.
> >
> > > + }
> > > +
> > > info.num_regions = VFIO_PCI_NUM_REGIONS;
> > > info.num_irqs = VFIO_PCI_NUM_IRQS;
> > >
> > > + /* Expose only implemented IRQs */
> > > + if (!(info.flags & VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
> > > + info.num_irqs--;
> >
> > I'm having second thoughts on this, see further below.
> >
> > > +
> > > return copy_to_user((void __user *)arg, , minsz);
> > >
> > > } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > > @@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
> > > if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
> > > return -EINVAL;
> > >
> > > + if ((info.index == VFIO_PCI_ERR_IRQ_INDEX) &&
> > > + !pci_is_pcie(vdev->pdev))
> > > + return -EINVAL;
> > > +
> >
> > Perhaps we could incorporate the index test above this too?
> >
> > switch (info.index) {
> > case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
> > break;
> > case VFIO_PCI_ERR_IRQ_INDEX:
> > if (pci_is_pcie(vdev->pdev))
> > break;
> > default:
> > return -EINVAL;
> > }
> >
> > This is more similar to how I've re-written the same for the proposed
> > VGA/legacy I/O support.
> >
> > > info.flags = VFIO_IRQ_INFO_EVENTFD;
> > >
> > > info.count = vfio_pci_get_irq_count(vdev, info.index);
> > > @@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> > > kfree(vdev);
> > > }
> > >
> > > +static pci_ers_
RE: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
-Original Message-
From: Alex Williamson [mailto:alex.william...@redhat.com]
Sent: Tuesday, January 29, 2013 5:25 AM
To: Pandarathil, Vijaymohan R
Cc: Gleb Natapov; Bjorn Helgaas; Blue Swirl; Ortiz, Lance E;
k...@vger.kernel.org; qemu-de...@nongnu.org; linux-...@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for
supporting AER
On Mon, 2013-01-28 at 12:31 -0700, Alex Williamson wrote:
On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
- New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
when
an error occurs in the vfio_pci_device
- Register pci_error_handler for the vfio_pci driver
- When the device encounters an error, the error handler registered
by
the vfio_pci driver gets invoked by the AER infrastructure
- In the error handler, signal the eventfd registered for the device.
- This results in the qemu eventfd handler getting invoked and
appropriate action taken for the guest.
Signed-off-by: Vijay Mohan Pandarathil vijaymohan.pandarat...@hp.com
---
drivers/vfio/pci/vfio_pci.c | 44
-
drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
drivers/vfio/pci/vfio_pci_private.h | 1 +
include/uapi/linux/vfio.h | 3 +++
4 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index b28e66c..ff2a078 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct
vfio_pci_device *vdev, int irq_type)
return (flags PCI_MSIX_FLAGS_QSIZE) + 1;
}
- }
+ } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
+ if (pci_is_pcie(vdev-pdev))
+ return 1;
return 0;
}
@@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
if (vdev-reset_works)
info.flags |= VFIO_DEVICE_FLAGS_RESET;
+ if (pci_is_pcie(vdev-pdev)) {
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
Not sure this second flag should be AER specific or if it's even needed,
see below for more comments on this.
+ }
+
info.num_regions = VFIO_PCI_NUM_REGIONS;
info.num_irqs = VFIO_PCI_NUM_IRQS;
+ /* Expose only implemented IRQs */
+ if (!(info.flags VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
+ info.num_irqs--;
I'm having second thoughts on this, see further below.
+
return copy_to_user((void __user *)arg, info, minsz);
} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
@@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
if (info.argsz minsz || info.index = VFIO_PCI_NUM_IRQS)
return -EINVAL;
+ if ((info.index == VFIO_PCI_ERR_IRQ_INDEX)
+ !pci_is_pcie(vdev-pdev))
+ return -EINVAL;
+
Perhaps we could incorporate the index test above this too?
switch (info.index) {
case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
break;
case VFIO_PCI_ERR_IRQ_INDEX:
if (pci_is_pcie(vdev-pdev))
break;
default:
return -EINVAL;
}
This is more similar to how I've re-written the same for the proposed
VGA/legacy I/O support.
info.flags = VFIO_IRQ_INFO_EVENTFD;
info.count = vfio_pci_get_irq_count(vdev, info.index);
@@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
kfree(vdev);
}
+static pci_ers_result_t vfio_err_detected(struct pci_dev *pdev,
+ pci_channel_state_t state)
This is actually AER specific, right? So perhaps it should be
vfio_pci_aer_err_detected?
Also, please follow existing whitespace usage throughout, tabs followed
by spaces to align function parameter wrap.
+{
+ struct vfio_pci_device *vpdev;
+ void *vdev;
struct vfio_device *vdev;
+
+ vdev = vfio_device_get_from_dev(pdev-dev);
+ if (vdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ vpdev = vfio_device_data(vdev);
+ if (vpdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ if (vpdev-err_trigger)
+ eventfd_signal(vpdev-err_trigger, 1);
+
+ vfio_device_put_vdev(vdev);
+
+ return PCI_ERS_RESULT_CAN_RECOVER;
+}
+
+static const struct pci_error_handlers vfio_err_handlers = {
+ .error_detected = vfio_err_detected,
+};
+
static struct pci_driver vfio_pci_driver = {
.name = vfio-pci,
.id_table = NULL, /* only dynamic ids */
.probe = vfio_pci_probe
Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
On Mon, 2013-01-28 at 12:31 -0700, Alex Williamson wrote:
> On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
> > - New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
> > when
> > an error occurs in the vfio_pci_device
> >
> > - Register pci_error_handler for the vfio_pci driver
> >
> > - When the device encounters an error, the error handler registered by
> > the vfio_pci driver gets invoked by the AER infrastructure
> >
> > - In the error handler, signal the eventfd registered for the device.
> >
> > - This results in the qemu eventfd handler getting invoked and
> > appropriate action taken for the guest.
> >
> > Signed-off-by: Vijay Mohan Pandarathil
> > ---
> > drivers/vfio/pci/vfio_pci.c | 44
> > -
> > drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
> > drivers/vfio/pci/vfio_pci_private.h | 1 +
> > include/uapi/linux/vfio.h | 3 +++
> > 4 files changed, 79 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > index b28e66c..ff2a078 100644
> > --- a/drivers/vfio/pci/vfio_pci.c
> > +++ b/drivers/vfio/pci/vfio_pci.c
> > @@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct
> > vfio_pci_device *vdev, int irq_type)
> >
> > return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> > }
> > - }
> > + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
> > + if (pci_is_pcie(vdev->pdev))
> > + return 1;
> >
> > return 0;
> > }
> > @@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
> > if (vdev->reset_works)
> > info.flags |= VFIO_DEVICE_FLAGS_RESET;
> >
> > + if (pci_is_pcie(vdev->pdev)) {
> > + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
> > + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
>
> Not sure this second flag should be AER specific or if it's even needed,
> see below for more comments on this.
>
> > + }
> > +
> > info.num_regions = VFIO_PCI_NUM_REGIONS;
> > info.num_irqs = VFIO_PCI_NUM_IRQS;
> >
> > + /* Expose only implemented IRQs */
> > + if (!(info.flags & VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
> > + info.num_irqs--;
>
> I'm having second thoughts on this, see further below.
>
> > +
> > return copy_to_user((void __user *)arg, , minsz);
> >
> > } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> > @@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
> > if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
> > return -EINVAL;
> >
> > + if ((info.index == VFIO_PCI_ERR_IRQ_INDEX) &&
> > +!pci_is_pcie(vdev->pdev))
> > + return -EINVAL;
> > +
>
> Perhaps we could incorporate the index test above this too?
>
> switch (info.index) {
> case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
> break;
> case VFIO_PCI_ERR_IRQ_INDEX:
> if (pci_is_pcie(vdev->pdev))
> break;
> default:
> return -EINVAL;
> }
>
> This is more similar to how I've re-written the same for the proposed
> VGA/legacy I/O support.
>
> > info.flags = VFIO_IRQ_INFO_EVENTFD;
> >
> > info.count = vfio_pci_get_irq_count(vdev, info.index);
> > @@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> > kfree(vdev);
> > }
> >
> > +static pci_ers_result_t vfio_err_detected(struct pci_dev *pdev,
> > + pci_channel_state_t state)
>
> This is actually AER specific, right? So perhaps it should be
> vfio_pci_aer_err_detected?
>
> Also, please follow existing whitespace usage throughout, tabs followed
> by spaces to align function parameter wrap.
>
> > +{
> > + struct vfio_pci_device *vpdev;
> > + void *vdev;
>
> struct vfio_device *vdev;
>
> > +
> > + vdev = vfio_device_get_from_dev(>dev);
> > + if (vdev == NULL)
> > + return PCI_ERS_RESULT_DISCONNECT;
> > +
> > + vpdev = vfio_device_data(vdev);
> > + if (vpdev == NULL)
> > + return PCI_ERS_RESULT_DISCONNECT;
> > +
> > + if (vpdev->err_trigger)
> > + eventfd_signal(vpdev->err_trigger, 1);
> > +
> > + vfio_device_put_vdev(vdev);
> > +
> > + return PCI_ERS_RESULT_CAN_RECOVER;
> > +}
> > +
> > +static const struct pci_error_handlers vfio_err_handlers = {
> > + .error_detected = vfio_err_detected,
> > +};
> > +
> > static struct pci_driver vfio_pci_driver = {
> > .name = "vfio-pci",
> > .id_table = NULL, /* only dynamic ids */
> > .probe = vfio_pci_probe,
> > .remove = vfio_pci_remove,
> > + .err_handler= _err_handlers,
> > };
> >
> > static void __exit vfio_pci_cleanup(void)
> > diff --git
Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
On Mon, 2013-01-28 at 12:31 -0700, Alex Williamson wrote:
On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
- New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
when
an error occurs in the vfio_pci_device
- Register pci_error_handler for the vfio_pci driver
- When the device encounters an error, the error handler registered by
the vfio_pci driver gets invoked by the AER infrastructure
- In the error handler, signal the eventfd registered for the device.
- This results in the qemu eventfd handler getting invoked and
appropriate action taken for the guest.
Signed-off-by: Vijay Mohan Pandarathil vijaymohan.pandarat...@hp.com
---
drivers/vfio/pci/vfio_pci.c | 44
-
drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
drivers/vfio/pci/vfio_pci_private.h | 1 +
include/uapi/linux/vfio.h | 3 +++
4 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index b28e66c..ff2a078 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct
vfio_pci_device *vdev, int irq_type)
return (flags PCI_MSIX_FLAGS_QSIZE) + 1;
}
- }
+ } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
+ if (pci_is_pcie(vdev-pdev))
+ return 1;
return 0;
}
@@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
if (vdev-reset_works)
info.flags |= VFIO_DEVICE_FLAGS_RESET;
+ if (pci_is_pcie(vdev-pdev)) {
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
Not sure this second flag should be AER specific or if it's even needed,
see below for more comments on this.
+ }
+
info.num_regions = VFIO_PCI_NUM_REGIONS;
info.num_irqs = VFIO_PCI_NUM_IRQS;
+ /* Expose only implemented IRQs */
+ if (!(info.flags VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
+ info.num_irqs--;
I'm having second thoughts on this, see further below.
+
return copy_to_user((void __user *)arg, info, minsz);
} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
@@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
if (info.argsz minsz || info.index = VFIO_PCI_NUM_IRQS)
return -EINVAL;
+ if ((info.index == VFIO_PCI_ERR_IRQ_INDEX)
+!pci_is_pcie(vdev-pdev))
+ return -EINVAL;
+
Perhaps we could incorporate the index test above this too?
switch (info.index) {
case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
break;
case VFIO_PCI_ERR_IRQ_INDEX:
if (pci_is_pcie(vdev-pdev))
break;
default:
return -EINVAL;
}
This is more similar to how I've re-written the same for the proposed
VGA/legacy I/O support.
info.flags = VFIO_IRQ_INFO_EVENTFD;
info.count = vfio_pci_get_irq_count(vdev, info.index);
@@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
kfree(vdev);
}
+static pci_ers_result_t vfio_err_detected(struct pci_dev *pdev,
+ pci_channel_state_t state)
This is actually AER specific, right? So perhaps it should be
vfio_pci_aer_err_detected?
Also, please follow existing whitespace usage throughout, tabs followed
by spaces to align function parameter wrap.
+{
+ struct vfio_pci_device *vpdev;
+ void *vdev;
struct vfio_device *vdev;
+
+ vdev = vfio_device_get_from_dev(pdev-dev);
+ if (vdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ vpdev = vfio_device_data(vdev);
+ if (vpdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ if (vpdev-err_trigger)
+ eventfd_signal(vpdev-err_trigger, 1);
+
+ vfio_device_put_vdev(vdev);
+
+ return PCI_ERS_RESULT_CAN_RECOVER;
+}
+
+static const struct pci_error_handlers vfio_err_handlers = {
+ .error_detected = vfio_err_detected,
+};
+
static struct pci_driver vfio_pci_driver = {
.name = vfio-pci,
.id_table = NULL, /* only dynamic ids */
.probe = vfio_pci_probe,
.remove = vfio_pci_remove,
+ .err_handler= vfio_err_handlers,
};
static void __exit vfio_pci_cleanup(void)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
b/drivers/vfio/pci/vfio_pci_intrs.c
index 3639371..f003e08 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -745,6 +745,31 @@ static int
Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
> - New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
> when
> an error occurs in the vfio_pci_device
>
> - Register pci_error_handler for the vfio_pci driver
>
> - When the device encounters an error, the error handler registered by
> the vfio_pci driver gets invoked by the AER infrastructure
>
> - In the error handler, signal the eventfd registered for the device.
>
> - This results in the qemu eventfd handler getting invoked and
> appropriate action taken for the guest.
>
> Signed-off-by: Vijay Mohan Pandarathil
> ---
> drivers/vfio/pci/vfio_pci.c | 44
> -
> drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
> drivers/vfio/pci/vfio_pci_private.h | 1 +
> include/uapi/linux/vfio.h | 3 +++
> 4 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index b28e66c..ff2a078 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device
> *vdev, int irq_type)
>
> return (flags & PCI_MSIX_FLAGS_QSIZE) + 1;
> }
> - }
> + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
> + if (pci_is_pcie(vdev->pdev))
> + return 1;
>
> return 0;
> }
> @@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
> if (vdev->reset_works)
> info.flags |= VFIO_DEVICE_FLAGS_RESET;
>
> + if (pci_is_pcie(vdev->pdev)) {
> + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
> + info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
Not sure this second flag should be AER specific or if it's even needed,
see below for more comments on this.
> + }
> +
> info.num_regions = VFIO_PCI_NUM_REGIONS;
> info.num_irqs = VFIO_PCI_NUM_IRQS;
>
> + /* Expose only implemented IRQs */
> + if (!(info.flags & VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
> + info.num_irqs--;
I'm having second thoughts on this, see further below.
> +
> return copy_to_user((void __user *)arg, , minsz);
>
> } else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
> @@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
> if (info.argsz < minsz || info.index >= VFIO_PCI_NUM_IRQS)
> return -EINVAL;
>
> + if ((info.index == VFIO_PCI_ERR_IRQ_INDEX) &&
> + !pci_is_pcie(vdev->pdev))
> + return -EINVAL;
> +
Perhaps we could incorporate the index test above this too?
switch (info.index) {
case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
break;
case VFIO_PCI_ERR_IRQ_INDEX:
if (pci_is_pcie(vdev->pdev))
break;
default:
return -EINVAL;
}
This is more similar to how I've re-written the same for the proposed
VGA/legacy I/O support.
> info.flags = VFIO_IRQ_INFO_EVENTFD;
>
> info.count = vfio_pci_get_irq_count(vdev, info.index);
> @@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> kfree(vdev);
> }
>
> +static pci_ers_result_t vfio_err_detected(struct pci_dev *pdev,
> + pci_channel_state_t state)
This is actually AER specific, right? So perhaps it should be
vfio_pci_aer_err_detected?
Also, please follow existing whitespace usage throughout, tabs followed
by spaces to align function parameter wrap.
> +{
> + struct vfio_pci_device *vpdev;
> + void *vdev;
struct vfio_device *vdev;
> +
> + vdev = vfio_device_get_from_dev(>dev);
> + if (vdev == NULL)
> + return PCI_ERS_RESULT_DISCONNECT;
> +
> + vpdev = vfio_device_data(vdev);
> + if (vpdev == NULL)
> + return PCI_ERS_RESULT_DISCONNECT;
> +
> + if (vpdev->err_trigger)
> + eventfd_signal(vpdev->err_trigger, 1);
> +
> + vfio_device_put_vdev(vdev);
> +
> + return PCI_ERS_RESULT_CAN_RECOVER;
> +}
> +
> +static const struct pci_error_handlers vfio_err_handlers = {
> + .error_detected = vfio_err_detected,
> +};
> +
> static struct pci_driver vfio_pci_driver = {
> .name = "vfio-pci",
> .id_table = NULL, /* only dynamic ids */
> .probe = vfio_pci_probe,
> .remove = vfio_pci_remove,
> + .err_handler= _err_handlers,
> };
>
> static void __exit vfio_pci_cleanup(void)
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
> b/drivers/vfio/pci/vfio_pci_intrs.c
> index 3639371..f003e08 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -745,6 +745,31 @@ static int
Re: [PATCH v2 2/3] VFIO-AER: Vfio-pci driver changes for supporting AER
On Mon, 2013-01-28 at 09:54 +, Pandarathil, Vijaymohan R wrote:
- New VFIO_SET_IRQ ioctl option to pass the eventfd that is signalled
when
an error occurs in the vfio_pci_device
- Register pci_error_handler for the vfio_pci driver
- When the device encounters an error, the error handler registered by
the vfio_pci driver gets invoked by the AER infrastructure
- In the error handler, signal the eventfd registered for the device.
- This results in the qemu eventfd handler getting invoked and
appropriate action taken for the guest.
Signed-off-by: Vijay Mohan Pandarathil vijaymohan.pandarat...@hp.com
---
drivers/vfio/pci/vfio_pci.c | 44
-
drivers/vfio/pci/vfio_pci_intrs.c | 32 +++
drivers/vfio/pci/vfio_pci_private.h | 1 +
include/uapi/linux/vfio.h | 3 +++
4 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index b28e66c..ff2a078 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -196,7 +196,9 @@ static int vfio_pci_get_irq_count(struct vfio_pci_device
*vdev, int irq_type)
return (flags PCI_MSIX_FLAGS_QSIZE) + 1;
}
- }
+ } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX)
+ if (pci_is_pcie(vdev-pdev))
+ return 1;
return 0;
}
@@ -223,9 +225,18 @@ static long vfio_pci_ioctl(void *device_data,
if (vdev-reset_works)
info.flags |= VFIO_DEVICE_FLAGS_RESET;
+ if (pci_is_pcie(vdev-pdev)) {
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER;
+ info.flags |= VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY;
Not sure this second flag should be AER specific or if it's even needed,
see below for more comments on this.
+ }
+
info.num_regions = VFIO_PCI_NUM_REGIONS;
info.num_irqs = VFIO_PCI_NUM_IRQS;
+ /* Expose only implemented IRQs */
+ if (!(info.flags VFIO_DEVICE_FLAGS_PCI_AER_NOTIFY))
+ info.num_irqs--;
I'm having second thoughts on this, see further below.
+
return copy_to_user((void __user *)arg, info, minsz);
} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
@@ -302,6 +313,10 @@ static long vfio_pci_ioctl(void *device_data,
if (info.argsz minsz || info.index = VFIO_PCI_NUM_IRQS)
return -EINVAL;
+ if ((info.index == VFIO_PCI_ERR_IRQ_INDEX)
+ !pci_is_pcie(vdev-pdev))
+ return -EINVAL;
+
Perhaps we could incorporate the index test above this too?
switch (info.index) {
case VFIO_PCI_INTX_IRQ_INDEX: ... VFIO_PCI_MSIX_IRQ_INDEX:
break;
case VFIO_PCI_ERR_IRQ_INDEX:
if (pci_is_pcie(vdev-pdev))
break;
default:
return -EINVAL;
}
This is more similar to how I've re-written the same for the proposed
VGA/legacy I/O support.
info.flags = VFIO_IRQ_INFO_EVENTFD;
info.count = vfio_pci_get_irq_count(vdev, info.index);
@@ -538,11 +553,38 @@ static void vfio_pci_remove(struct pci_dev *pdev)
kfree(vdev);
}
+static pci_ers_result_t vfio_err_detected(struct pci_dev *pdev,
+ pci_channel_state_t state)
This is actually AER specific, right? So perhaps it should be
vfio_pci_aer_err_detected?
Also, please follow existing whitespace usage throughout, tabs followed
by spaces to align function parameter wrap.
+{
+ struct vfio_pci_device *vpdev;
+ void *vdev;
struct vfio_device *vdev;
+
+ vdev = vfio_device_get_from_dev(pdev-dev);
+ if (vdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ vpdev = vfio_device_data(vdev);
+ if (vpdev == NULL)
+ return PCI_ERS_RESULT_DISCONNECT;
+
+ if (vpdev-err_trigger)
+ eventfd_signal(vpdev-err_trigger, 1);
+
+ vfio_device_put_vdev(vdev);
+
+ return PCI_ERS_RESULT_CAN_RECOVER;
+}
+
+static const struct pci_error_handlers vfio_err_handlers = {
+ .error_detected = vfio_err_detected,
+};
+
static struct pci_driver vfio_pci_driver = {
.name = vfio-pci,
.id_table = NULL, /* only dynamic ids */
.probe = vfio_pci_probe,
.remove = vfio_pci_remove,
+ .err_handler= vfio_err_handlers,
};
static void __exit vfio_pci_cleanup(void)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
b/drivers/vfio/pci/vfio_pci_intrs.c
index 3639371..f003e08 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -745,6 +745,31 @@ static int vfio_pci_set_msi_trigger(struct
vfio_pci_device *vdev,
return 0;
}
+static int
