* [Qemu-devel] [PATCH v2 0/2] vfio: add aer process @ 2016-07-19 7:32 Zhou Jie 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier Zhou Jie 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 2/2] vfio : add aer process Zhou Jie 0 siblings, 2 replies; 8+ messages in thread From: Zhou Jie @ 2016-07-19 7:32 UTC (permalink / raw) To: alex.williamson; +Cc: linux-kernel, qemu-devel, izumi.taku, fan.chen, Chen Fan From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> v1-v2: 1. Add aer process to vfio driver. Chen Fan (1): vfio : add aer process root (1): vfio : resume notifier drivers/vfio/pci/vfio_pci.c | 58 ++++++++++++++++++++++++++++++++++++- drivers/vfio/pci/vfio_pci_intrs.c | 18 ++++++++++++ drivers/vfio/pci/vfio_pci_private.h | 3 ++ include/uapi/linux/vfio.h | 3 ++ 4 files changed, 81 insertions(+), 1 deletion(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier 2016-07-19 7:32 [Qemu-devel] [PATCH v2 0/2] vfio: add aer process Zhou Jie @ 2016-07-19 7:32 ` Zhou Jie 2016-07-19 7:39 ` Chen Fan 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 2/2] vfio : add aer process Zhou Jie 1 sibling, 1 reply; 8+ messages in thread From: Zhou Jie @ 2016-07-19 7:32 UTC (permalink / raw) To: alex.williamson; +Cc: linux-kernel, qemu-devel, izumi.taku, fan.chen, root From: root <root@root.com> --- drivers/vfio/pci/vfio_pci.c | 28 +++++++++++++++++++++++++++- drivers/vfio/pci/vfio_pci_intrs.c | 18 ++++++++++++++++++ drivers/vfio/pci/vfio_pci_private.h | 1 + include/uapi/linux/vfio.h | 1 + 4 files changed, 47 insertions(+), 1 deletion(-) diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 188b1ff..2d12b03 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -363,7 +363,8 @@ 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) { + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX || + irq_type == VFIO_PCI_RESUME_IRQ_INDEX) { if (pci_is_pcie(vdev->pdev)) return 1; } else if (irq_type == VFIO_PCI_REQ_IRQ_INDEX) { @@ -731,6 +732,7 @@ static long vfio_pci_ioctl(void *device_data, case VFIO_PCI_REQ_IRQ_INDEX: break; case VFIO_PCI_ERR_IRQ_INDEX: + case VFIO_PCI_RESUME_IRQ_INDEX: if (pci_is_pcie(vdev->pdev)) break; /* pass thru to return error */ @@ -1234,8 +1236,32 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, return PCI_ERS_RESULT_CAN_RECOVER; } +static void vfio_pci_aer_resume(struct pci_dev *pdev) +{ + struct vfio_pci_device *vdev; + struct vfio_device *device; + + device = vfio_device_get_from_dev(&pdev->dev); + if (device == NULL) + return; + + vdev = vfio_device_data(device); + if (vdev == NULL) { + vfio_device_put(device); + return; + } + + mutex_lock(&vdev->igate); + if (vdev->resume_trigger) + eventfd_signal(vdev->resume_trigger, 1); + + mutex_unlock(&vdev->igate); + vfio_device_put(device); +} + static const struct pci_error_handlers vfio_err_handlers = { .error_detected = vfio_pci_aer_err_detected, + .resume = vfio_pci_aer_resume, }; static struct pci_driver vfio_pci_driver = { diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 15ecfc9..3a01a62 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -617,6 +617,16 @@ static int vfio_pci_set_err_trigger(struct vfio_pci_device *vdev, return vfio_pci_set_ctx_trigger_single(&vdev->err_trigger, flags, data); } +static int vfio_pci_set_resume_trigger(struct vfio_pci_device *vdev, + unsigned index, unsigned start, + unsigned count, uint32_t flags, void *data) +{ + if (index != VFIO_PCI_RESUME_IRQ_INDEX) + return -EINVAL; + + return vfio_pci_set_ctx_trigger_single(&vdev->resume_trigger, flags, data); +} + static int vfio_pci_set_req_trigger(struct vfio_pci_device *vdev, unsigned index, unsigned start, unsigned count, uint32_t flags, void *data) @@ -676,6 +686,14 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_device *vdev, uint32_t flags, break; } break; + case VFIO_PCI_RESUME_IRQ_INDEX: + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) { + case VFIO_IRQ_SET_ACTION_TRIGGER: + if (pci_is_pcie(vdev->pdev)) + func = vfio_pci_set_resume_trigger; + break; + } + break; } if (!func) diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h index 016c14a..80d4ddd 100644 --- a/drivers/vfio/pci/vfio_pci_private.h +++ b/drivers/vfio/pci/vfio_pci_private.h @@ -88,6 +88,7 @@ struct vfio_pci_device { int refcnt; struct eventfd_ctx *err_trigger; struct eventfd_ctx *req_trigger; + struct eventfd_ctx *resume_trigger; }; #define is_intx(vdev) (vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX) diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 255a211..34ab138 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -433,6 +433,7 @@ enum { VFIO_PCI_MSIX_IRQ_INDEX, VFIO_PCI_ERR_IRQ_INDEX, VFIO_PCI_REQ_IRQ_INDEX, + VFIO_PCI_RESUME_IRQ_INDEX, VFIO_PCI_NUM_IRQS }; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier Zhou Jie @ 2016-07-19 7:39 ` Chen Fan 0 siblings, 0 replies; 8+ messages in thread From: Chen Fan @ 2016-07-19 7:39 UTC (permalink / raw) To: Zhou Jie, alex.williamson; +Cc: linux-kernel, qemu-devel, izumi.taku, root On 2016年07月19日 15:32, Zhou Jie wrote: > From: root <root@root.com> Pls add your username and email by using git config --global user.name / user.email to update your git config, and git send-email again. Thanks, Chen > > --- > drivers/vfio/pci/vfio_pci.c | 28 +++++++++++++++++++++++++++- > drivers/vfio/pci/vfio_pci_intrs.c | 18 ++++++++++++++++++ > drivers/vfio/pci/vfio_pci_private.h | 1 + > include/uapi/linux/vfio.h | 1 + > 4 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 188b1ff..2d12b03 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -363,7 +363,8 @@ 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) { > + } else if (irq_type == VFIO_PCI_ERR_IRQ_INDEX || > + irq_type == VFIO_PCI_RESUME_IRQ_INDEX) { > if (pci_is_pcie(vdev->pdev)) > return 1; > } else if (irq_type == VFIO_PCI_REQ_IRQ_INDEX) { > @@ -731,6 +732,7 @@ static long vfio_pci_ioctl(void *device_data, > case VFIO_PCI_REQ_IRQ_INDEX: > break; > case VFIO_PCI_ERR_IRQ_INDEX: > + case VFIO_PCI_RESUME_IRQ_INDEX: > if (pci_is_pcie(vdev->pdev)) > break; > /* pass thru to return error */ > @@ -1234,8 +1236,32 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > return PCI_ERS_RESULT_CAN_RECOVER; > } > > +static void vfio_pci_aer_resume(struct pci_dev *pdev) > +{ > + struct vfio_pci_device *vdev; > + struct vfio_device *device; > + > + device = vfio_device_get_from_dev(&pdev->dev); > + if (device == NULL) > + return; > + > + vdev = vfio_device_data(device); > + if (vdev == NULL) { > + vfio_device_put(device); > + return; > + } > + > + mutex_lock(&vdev->igate); > + if (vdev->resume_trigger) > + eventfd_signal(vdev->resume_trigger, 1); > + > + mutex_unlock(&vdev->igate); > + vfio_device_put(device); > +} > + > static const struct pci_error_handlers vfio_err_handlers = { > .error_detected = vfio_pci_aer_err_detected, > + .resume = vfio_pci_aer_resume, > }; > > static struct pci_driver vfio_pci_driver = { > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 15ecfc9..3a01a62 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -617,6 +617,16 @@ static int vfio_pci_set_err_trigger(struct vfio_pci_device *vdev, > return vfio_pci_set_ctx_trigger_single(&vdev->err_trigger, flags, data); > } > > +static int vfio_pci_set_resume_trigger(struct vfio_pci_device *vdev, > + unsigned index, unsigned start, > + unsigned count, uint32_t flags, void *data) > +{ > + if (index != VFIO_PCI_RESUME_IRQ_INDEX) > + return -EINVAL; > + > + return vfio_pci_set_ctx_trigger_single(&vdev->resume_trigger, flags, data); > +} > + > static int vfio_pci_set_req_trigger(struct vfio_pci_device *vdev, > unsigned index, unsigned start, > unsigned count, uint32_t flags, void *data) > @@ -676,6 +686,14 @@ int vfio_pci_set_irqs_ioctl(struct vfio_pci_device *vdev, uint32_t flags, > break; > } > break; > + case VFIO_PCI_RESUME_IRQ_INDEX: > + switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) { > + case VFIO_IRQ_SET_ACTION_TRIGGER: > + if (pci_is_pcie(vdev->pdev)) > + func = vfio_pci_set_resume_trigger; > + break; > + } > + break; > } > > if (!func) > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h > index 016c14a..80d4ddd 100644 > --- a/drivers/vfio/pci/vfio_pci_private.h > +++ b/drivers/vfio/pci/vfio_pci_private.h > @@ -88,6 +88,7 @@ struct vfio_pci_device { > int refcnt; > struct eventfd_ctx *err_trigger; > struct eventfd_ctx *req_trigger; > + struct eventfd_ctx *resume_trigger; > }; > > #define is_intx(vdev) (vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX) > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 255a211..34ab138 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -433,6 +433,7 @@ enum { > VFIO_PCI_MSIX_IRQ_INDEX, > VFIO_PCI_ERR_IRQ_INDEX, > VFIO_PCI_REQ_IRQ_INDEX, > + VFIO_PCI_RESUME_IRQ_INDEX, > VFIO_PCI_NUM_IRQS > }; > -- Sincerely, Chen Fan ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] vfio : add aer process 2016-07-19 7:32 [Qemu-devel] [PATCH v2 0/2] vfio: add aer process Zhou Jie 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier Zhou Jie @ 2016-07-19 7:32 ` Zhou Jie 2016-07-29 17:12 ` Alex Williamson 1 sibling, 1 reply; 8+ messages in thread From: Zhou Jie @ 2016-07-19 7:32 UTC (permalink / raw) To: alex.williamson Cc: linux-kernel, qemu-devel, izumi.taku, fan.chen, Chen Fan, Zhou Jie From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> During aer err occurs and resume do following to protect device from being accessed. 1. Make config space read only. 2. Disable INTx/MSI Interrupt. 3. Do nothing for bar regions. Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com> --- drivers/vfio/pci/vfio_pci.c | 30 ++++++++++++++++++++++++++++++ drivers/vfio/pci/vfio_pci_private.h | 2 ++ include/uapi/linux/vfio.h | 2 ++ 3 files changed, 34 insertions(+) diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 2d12b03..dd96b60 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -318,6 +318,7 @@ static int vfio_pci_open(void *device_data) return -ENODEV; mutex_lock(&driver_lock); + init_completion(&vdev->aer_error_completion); if (!vdev->refcnt) { ret = vfio_pci_enable(vdev); @@ -571,6 +572,16 @@ static long vfio_pci_ioctl(void *device_data, struct vfio_pci_device *vdev = device_data; unsigned long minsz; + if (vdev->aer_error_in_progress && (cmd == VFIO_DEVICE_SET_IRQS || + cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) { + int ret; + ret = wait_for_completion_interruptible( + &vdev->aer_error_completion); + if (ret) { + return ret; + } + } + if (cmd == VFIO_DEVICE_GET_INFO) { struct vfio_device_info info; @@ -587,6 +598,10 @@ static long vfio_pci_ioctl(void *device_data, if (vdev->reset_works) info.flags |= VFIO_DEVICE_FLAGS_RESET; + info.flags |= VFIO_DEVICE_FLAGS_AERPROCESS; + if (vdev->aer_error_in_progress) + info.flags |= VFIO_DEVICE_FLAGS_INAERPROCESS; + info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions; info.num_irqs = VFIO_PCI_NUM_IRQS; @@ -996,6 +1011,14 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf, switch (index) { case VFIO_PCI_CONFIG_REGION_INDEX: + if (vdev->aer_error_in_progress && iswrite) { + int ret; + ret = wait_for_completion_interruptible( + &vdev->aer_error_completion); + if (ret) { + return ret; + } + } return vfio_pci_config_rw(vdev, buf, count, ppos, iswrite); case VFIO_PCI_ROM_REGION_INDEX: @@ -1226,6 +1249,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, mutex_lock(&vdev->igate); + vdev->aer_error_in_progress = true; + vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | + VFIO_IRQ_SET_ACTION_TRIGGER, + vdev->irq_type, 0, 0, NULL); if (vdev->err_trigger) eventfd_signal(vdev->err_trigger, 1); @@ -1252,6 +1279,9 @@ static void vfio_pci_aer_resume(struct pci_dev *pdev) } mutex_lock(&vdev->igate); + + vdev->aer_error_in_progress = false; + complete_all(&vdev->aer_error_completion); if (vdev->resume_trigger) eventfd_signal(vdev->resume_trigger, 1); diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h index 80d4ddd..2f151f5 100644 --- a/drivers/vfio/pci/vfio_pci_private.h +++ b/drivers/vfio/pci/vfio_pci_private.h @@ -84,6 +84,8 @@ struct vfio_pci_device { bool has_vga; bool needs_reset; bool nointx; + bool aer_error_in_progress; + struct completion aer_error_completion; struct pci_saved_state *pci_saved_state; int refcnt; struct eventfd_ctx *err_trigger; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index 34ab138..276ce50 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -198,6 +198,8 @@ struct vfio_device_info { #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */ #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ +#define VFIO_DEVICE_FLAGS_AERPROCESS (1 << 4) /* support aer error progress */ +#define VFIO_DEVICE_FLAGS_INAERPROCESS (1 << 5)/* status in aer error progress */ __u32 num_regions; /* Max region index + 1 */ __u32 num_irqs; /* Max IRQ index + 1 */ }; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] vfio : add aer process 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 2/2] vfio : add aer process Zhou Jie @ 2016-07-29 17:12 ` Alex Williamson 2016-08-01 2:14 ` Zhou Jie 0 siblings, 1 reply; 8+ messages in thread From: Alex Williamson @ 2016-07-29 17:12 UTC (permalink / raw) To: Zhou Jie; +Cc: fan.chen, linux-kernel, qemu-devel, Chen Fan, izumi.taku On Tue, 19 Jul 2016 15:32:43 +0800 Zhou Jie <zhoujie2011@cn.fujitsu.com> wrote: > From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > > During aer err occurs and resume do following to > protect device from being accessed. > 1. Make config space read only. > 2. Disable INTx/MSI Interrupt. > 3. Do nothing for bar regions. > > Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com> > --- > drivers/vfio/pci/vfio_pci.c | 30 ++++++++++++++++++++++++++++++ > drivers/vfio/pci/vfio_pci_private.h | 2 ++ > include/uapi/linux/vfio.h | 2 ++ > 3 files changed, 34 insertions(+) > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index 2d12b03..dd96b60 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -318,6 +318,7 @@ static int vfio_pci_open(void *device_data) > return -ENODEV; > > mutex_lock(&driver_lock); > + init_completion(&vdev->aer_error_completion); > > if (!vdev->refcnt) { > ret = vfio_pci_enable(vdev); > @@ -571,6 +572,16 @@ static long vfio_pci_ioctl(void *device_data, > struct vfio_pci_device *vdev = device_data; > unsigned long minsz; > > + if (vdev->aer_error_in_progress && (cmd == VFIO_DEVICE_SET_IRQS || > + cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) { > + int ret; > + ret = wait_for_completion_interruptible( > + &vdev->aer_error_completion); > + if (ret) { > + return ret; > + } No brackets necessary. > + } > + > if (cmd == VFIO_DEVICE_GET_INFO) { > struct vfio_device_info info; > > @@ -587,6 +598,10 @@ static long vfio_pci_ioctl(void *device_data, > if (vdev->reset_works) > info.flags |= VFIO_DEVICE_FLAGS_RESET; > > + info.flags |= VFIO_DEVICE_FLAGS_AERPROCESS; > + if (vdev->aer_error_in_progress) > + info.flags |= VFIO_DEVICE_FLAGS_INAERPROCESS; > + > info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions; > info.num_irqs = VFIO_PCI_NUM_IRQS; > > @@ -996,6 +1011,14 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf, > > switch (index) { > case VFIO_PCI_CONFIG_REGION_INDEX: > + if (vdev->aer_error_in_progress && iswrite) { > + int ret; > + ret = wait_for_completion_interruptible( > + &vdev->aer_error_completion); > + if (ret) { > + return ret; > + } > + } > return vfio_pci_config_rw(vdev, buf, count, ppos, iswrite); > > case VFIO_PCI_ROM_REGION_INDEX: > @@ -1226,6 +1249,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > > mutex_lock(&vdev->igate); > > + vdev->aer_error_in_progress = true; > + vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | > + VFIO_IRQ_SET_ACTION_TRIGGER, > + vdev->irq_type, 0, 0, NULL); > if (vdev->err_trigger) > eventfd_signal(vdev->err_trigger, 1); > > @@ -1252,6 +1279,9 @@ static void vfio_pci_aer_resume(struct pci_dev *pdev) > } > > mutex_lock(&vdev->igate); > + > + vdev->aer_error_in_progress = false; > + complete_all(&vdev->aer_error_completion); > if (vdev->resume_trigger) > eventfd_signal(vdev->resume_trigger, 1); > > diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h > index 80d4ddd..2f151f5 100644 > --- a/drivers/vfio/pci/vfio_pci_private.h > +++ b/drivers/vfio/pci/vfio_pci_private.h > @@ -84,6 +84,8 @@ struct vfio_pci_device { > bool has_vga; > bool needs_reset; > bool nointx; > + bool aer_error_in_progress; > + struct completion aer_error_completion; > struct pci_saved_state *pci_saved_state; > int refcnt; > struct eventfd_ctx *err_trigger; > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > index 34ab138..276ce50 100644 > --- a/include/uapi/linux/vfio.h > +++ b/include/uapi/linux/vfio.h > @@ -198,6 +198,8 @@ struct vfio_device_info { > #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */ > #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > +#define VFIO_DEVICE_FLAGS_AERPROCESS (1 << 4) /* support aer error progress */ > +#define VFIO_DEVICE_FLAGS_INAERPROCESS (1 << 5)/* status in aer error progress */ > __u32 num_regions; /* Max region index + 1 */ > __u32 num_irqs; /* Max IRQ index + 1 */ > }; Clearly this has only been tested for a single instance of an AER error event and resume per device. Are the things you're intending to block actually blocked for subsequent events? Note how complete_all() fills the done field to let all current and future waiters go through and nowhere is there a call to reinit_completion() to drain that path. Thanks, Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] vfio : add aer process 2016-07-29 17:12 ` Alex Williamson @ 2016-08-01 2:14 ` Zhou Jie 2016-08-01 15:36 ` Alex Williamson 0 siblings, 1 reply; 8+ messages in thread From: Zhou Jie @ 2016-08-01 2:14 UTC (permalink / raw) To: Alex Williamson; +Cc: fan.chen, linux-kernel, qemu-devel, Chen Fan, izumi.taku Hi, Alex On 2016/7/30 1:12, Alex Williamson wrote: > On Tue, 19 Jul 2016 15:32:43 +0800 > Zhou Jie <zhoujie2011@cn.fujitsu.com> wrote: > >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> >> >> During aer err occurs and resume do following to >> protect device from being accessed. >> 1. Make config space read only. >> 2. Disable INTx/MSI Interrupt. >> 3. Do nothing for bar regions. >> >> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com> >> --- >> drivers/vfio/pci/vfio_pci.c | 30 ++++++++++++++++++++++++++++++ >> drivers/vfio/pci/vfio_pci_private.h | 2 ++ >> include/uapi/linux/vfio.h | 2 ++ >> 3 files changed, 34 insertions(+) >> >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c >> index 2d12b03..dd96b60 100644 >> --- a/drivers/vfio/pci/vfio_pci.c >> +++ b/drivers/vfio/pci/vfio_pci.c >> @@ -318,6 +318,7 @@ static int vfio_pci_open(void *device_data) >> return -ENODEV; >> >> mutex_lock(&driver_lock); >> + init_completion(&vdev->aer_error_completion); >> >> if (!vdev->refcnt) { >> ret = vfio_pci_enable(vdev); >> @@ -571,6 +572,16 @@ static long vfio_pci_ioctl(void *device_data, >> struct vfio_pci_device *vdev = device_data; >> unsigned long minsz; >> >> + if (vdev->aer_error_in_progress && (cmd == VFIO_DEVICE_SET_IRQS || >> + cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) { >> + int ret; >> + ret = wait_for_completion_interruptible( >> + &vdev->aer_error_completion); >> + if (ret) { >> + return ret; >> + } > > No brackets necessary. > >> + } >> + >> if (cmd == VFIO_DEVICE_GET_INFO) { >> struct vfio_device_info info; >> >> @@ -587,6 +598,10 @@ static long vfio_pci_ioctl(void *device_data, >> if (vdev->reset_works) >> info.flags |= VFIO_DEVICE_FLAGS_RESET; >> >> + info.flags |= VFIO_DEVICE_FLAGS_AERPROCESS; >> + if (vdev->aer_error_in_progress) >> + info.flags |= VFIO_DEVICE_FLAGS_INAERPROCESS; >> + >> info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions; >> info.num_irqs = VFIO_PCI_NUM_IRQS; >> >> @@ -996,6 +1011,14 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf, >> >> switch (index) { >> case VFIO_PCI_CONFIG_REGION_INDEX: >> + if (vdev->aer_error_in_progress && iswrite) { >> + int ret; >> + ret = wait_for_completion_interruptible( >> + &vdev->aer_error_completion); >> + if (ret) { >> + return ret; >> + } >> + } >> return vfio_pci_config_rw(vdev, buf, count, ppos, iswrite); >> >> case VFIO_PCI_ROM_REGION_INDEX: >> @@ -1226,6 +1249,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, >> >> mutex_lock(&vdev->igate); >> >> + vdev->aer_error_in_progress = true; >> + vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | >> + VFIO_IRQ_SET_ACTION_TRIGGER, >> + vdev->irq_type, 0, 0, NULL); >> if (vdev->err_trigger) >> eventfd_signal(vdev->err_trigger, 1); >> >> @@ -1252,6 +1279,9 @@ static void vfio_pci_aer_resume(struct pci_dev *pdev) >> } >> >> mutex_lock(&vdev->igate); >> + >> + vdev->aer_error_in_progress = false; >> + complete_all(&vdev->aer_error_completion); >> if (vdev->resume_trigger) >> eventfd_signal(vdev->resume_trigger, 1); >> >> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h >> index 80d4ddd..2f151f5 100644 >> --- a/drivers/vfio/pci/vfio_pci_private.h >> +++ b/drivers/vfio/pci/vfio_pci_private.h >> @@ -84,6 +84,8 @@ struct vfio_pci_device { >> bool has_vga; >> bool needs_reset; >> bool nointx; >> + bool aer_error_in_progress; >> + struct completion aer_error_completion; >> struct pci_saved_state *pci_saved_state; >> int refcnt; >> struct eventfd_ctx *err_trigger; >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h >> index 34ab138..276ce50 100644 >> --- a/include/uapi/linux/vfio.h >> +++ b/include/uapi/linux/vfio.h >> @@ -198,6 +198,8 @@ struct vfio_device_info { >> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */ >> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ >> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ >> +#define VFIO_DEVICE_FLAGS_AERPROCESS (1 << 4) /* support aer error progress */ >> +#define VFIO_DEVICE_FLAGS_INAERPROCESS (1 << 5)/* status in aer error progress */ >> __u32 num_regions; /* Max region index + 1 */ >> __u32 num_irqs; /* Max IRQ index + 1 */ >> }; > > Clearly this has only been tested for a single instance of an AER error > event and resume per device. Are the things you're intending to block > actually blocked for subsequent events? Note how complete_all() fills > the done field to let all current and future waiters go through and > nowhere is there a call to reinit_completion() to drain that path. > Thanks, > > Alex Do you mean this condition? For device 1: error1 occurs ---- error1 resumes error2 occurs ---- error2 resumes error3 occurs ---- error3 resumes In current code, I do complete_all() when error1 resumes. And this will unblock the device when error2 and error3 are still be processed. Sincerely ZhouJie ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] vfio : add aer process 2016-08-01 2:14 ` Zhou Jie @ 2016-08-01 15:36 ` Alex Williamson 2016-08-02 1:22 ` Zhou Jie 0 siblings, 1 reply; 8+ messages in thread From: Alex Williamson @ 2016-08-01 15:36 UTC (permalink / raw) To: Zhou Jie; +Cc: fan.chen, linux-kernel, qemu-devel, Chen Fan, izumi.taku On Mon, 1 Aug 2016 10:14:06 +0800 Zhou Jie <zhoujie2011@cn.fujitsu.com> wrote: > Hi, Alex > > On 2016/7/30 1:12, Alex Williamson wrote: > > On Tue, 19 Jul 2016 15:32:43 +0800 > > Zhou Jie <zhoujie2011@cn.fujitsu.com> wrote: > > > >> From: Chen Fan <chen.fan.fnst@cn.fujitsu.com> > >> > >> During aer err occurs and resume do following to > >> protect device from being accessed. > >> 1. Make config space read only. > >> 2. Disable INTx/MSI Interrupt. > >> 3. Do nothing for bar regions. > >> > >> Signed-off-by: Zhou Jie <zhoujie2011@cn.fujitsu.com> > >> --- > >> drivers/vfio/pci/vfio_pci.c | 30 ++++++++++++++++++++++++++++++ > >> drivers/vfio/pci/vfio_pci_private.h | 2 ++ > >> include/uapi/linux/vfio.h | 2 ++ > >> 3 files changed, 34 insertions(+) > >> > >> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > >> index 2d12b03..dd96b60 100644 > >> --- a/drivers/vfio/pci/vfio_pci.c > >> +++ b/drivers/vfio/pci/vfio_pci.c > >> @@ -318,6 +318,7 @@ static int vfio_pci_open(void *device_data) > >> return -ENODEV; > >> > >> mutex_lock(&driver_lock); > >> + init_completion(&vdev->aer_error_completion); > >> > >> if (!vdev->refcnt) { > >> ret = vfio_pci_enable(vdev); > >> @@ -571,6 +572,16 @@ static long vfio_pci_ioctl(void *device_data, > >> struct vfio_pci_device *vdev = device_data; > >> unsigned long minsz; > >> > >> + if (vdev->aer_error_in_progress && (cmd == VFIO_DEVICE_SET_IRQS || > >> + cmd == VFIO_DEVICE_RESET || cmd == VFIO_DEVICE_PCI_HOT_RESET)) { > >> + int ret; > >> + ret = wait_for_completion_interruptible( > >> + &vdev->aer_error_completion); > >> + if (ret) { > >> + return ret; > >> + } > > > > No brackets necessary. > > > >> + } > >> + > >> if (cmd == VFIO_DEVICE_GET_INFO) { > >> struct vfio_device_info info; > >> > >> @@ -587,6 +598,10 @@ static long vfio_pci_ioctl(void *device_data, > >> if (vdev->reset_works) > >> info.flags |= VFIO_DEVICE_FLAGS_RESET; > >> > >> + info.flags |= VFIO_DEVICE_FLAGS_AERPROCESS; > >> + if (vdev->aer_error_in_progress) > >> + info.flags |= VFIO_DEVICE_FLAGS_INAERPROCESS; > >> + > >> info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions; > >> info.num_irqs = VFIO_PCI_NUM_IRQS; > >> > >> @@ -996,6 +1011,14 @@ static ssize_t vfio_pci_rw(void *device_data, char __user *buf, > >> > >> switch (index) { > >> case VFIO_PCI_CONFIG_REGION_INDEX: > >> + if (vdev->aer_error_in_progress && iswrite) { > >> + int ret; > >> + ret = wait_for_completion_interruptible( > >> + &vdev->aer_error_completion); > >> + if (ret) { > >> + return ret; > >> + } > >> + } > >> return vfio_pci_config_rw(vdev, buf, count, ppos, iswrite); > >> > >> case VFIO_PCI_ROM_REGION_INDEX: > >> @@ -1226,6 +1249,10 @@ static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, > >> > >> mutex_lock(&vdev->igate); > >> > >> + vdev->aer_error_in_progress = true; > >> + vfio_pci_set_irqs_ioctl(vdev, VFIO_IRQ_SET_DATA_NONE | > >> + VFIO_IRQ_SET_ACTION_TRIGGER, > >> + vdev->irq_type, 0, 0, NULL); > >> if (vdev->err_trigger) > >> eventfd_signal(vdev->err_trigger, 1); > >> > >> @@ -1252,6 +1279,9 @@ static void vfio_pci_aer_resume(struct pci_dev *pdev) > >> } > >> > >> mutex_lock(&vdev->igate); > >> + > >> + vdev->aer_error_in_progress = false; > >> + complete_all(&vdev->aer_error_completion); > >> if (vdev->resume_trigger) > >> eventfd_signal(vdev->resume_trigger, 1); > >> > >> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h > >> index 80d4ddd..2f151f5 100644 > >> --- a/drivers/vfio/pci/vfio_pci_private.h > >> +++ b/drivers/vfio/pci/vfio_pci_private.h > >> @@ -84,6 +84,8 @@ struct vfio_pci_device { > >> bool has_vga; > >> bool needs_reset; > >> bool nointx; > >> + bool aer_error_in_progress; > >> + struct completion aer_error_completion; > >> struct pci_saved_state *pci_saved_state; > >> int refcnt; > >> struct eventfd_ctx *err_trigger; > >> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h > >> index 34ab138..276ce50 100644 > >> --- a/include/uapi/linux/vfio.h > >> +++ b/include/uapi/linux/vfio.h > >> @@ -198,6 +198,8 @@ struct vfio_device_info { > >> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */ > >> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */ > >> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */ > >> +#define VFIO_DEVICE_FLAGS_AERPROCESS (1 << 4) /* support aer error progress */ > >> +#define VFIO_DEVICE_FLAGS_INAERPROCESS (1 << 5)/* status in aer error progress */ > >> __u32 num_regions; /* Max region index + 1 */ > >> __u32 num_irqs; /* Max IRQ index + 1 */ > >> }; > > > > Clearly this has only been tested for a single instance of an AER error > > event and resume per device. Are the things you're intending to block > > actually blocked for subsequent events? Note how complete_all() fills > > the done field to let all current and future waiters go through and > > nowhere is there a call to reinit_completion() to drain that path. > > Thanks, > > > > Alex > > Do you mean this condition? > > For device 1: > error1 occurs ---- error1 resumes > error2 occurs ---- error2 resumes > error3 occurs ---- error3 resumes > > In current code, I do complete_all() when error1 resumes. > And this will unblock the device > when error2 and error3 are still be processed. So walk me through how this works. On vfio_pci_open() we call init_completion(), which sets aer_error_completion.done equal to zero (BTW, a user can open the device file descriptor multiple times, so there's already a bug here). Let's assume that an error occurs and the user stalls a single access on wait_for_completion_interruptible(). The bulk of this function happens here: static inline long __sched do_wait_for_common(struct completion *x, long (*action)(long), long timeout, int state) { if (!x->done) { DECLARE_WAITQUEUE(wait, current); __add_wait_queue_tail_exclusive(&x->wait, &wait); do { if (signal_pending_state(state, current)) { timeout = -ERESTARTSYS; break; } __set_current_state(state); spin_unlock_irq(&x->wait.lock); timeout = action(timeout); spin_lock_irq(&x->wait.lock); } while (!x->done && timeout); __remove_wait_queue(&x->wait, &wait); if (!x->done) return timeout; } x->done--; return timeout ?: 1; } So it waits within that do{}while loop for a completion, interruption, or timeout. Then we have: void complete_all(struct completion *x) { unsigned long flags; spin_lock_irqsave(&x->wait.lock, flags); x->done += UINT_MAX/2; __wake_up_locked(&x->wait, TASK_NORMAL, 0); spin_unlock_irqrestore(&x->wait.lock, flags); } So aer_error_completion.done gets incremented to let a couple billion completion waiters through... Show me how another call to wait_for_completion_interruptible() will ever block again within our lifetime when the actual wait of do_wait_for_common() is only entered when 'done' count is equal to zero. This seems to be why reinit_completion() exists, but it's not used here. Thanks, Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] vfio : add aer process 2016-08-01 15:36 ` Alex Williamson @ 2016-08-02 1:22 ` Zhou Jie 0 siblings, 0 replies; 8+ messages in thread From: Zhou Jie @ 2016-08-02 1:22 UTC (permalink / raw) To: Alex Williamson; +Cc: fan.chen, linux-kernel, qemu-devel, Chen Fan, izumi.taku Hi, Alex >>> Clearly this has only been tested for a single instance of an AER error >>> event and resume per device. Are the things you're intending to block >>> actually blocked for subsequent events? Note how complete_all() fills >>> the done field to let all current and future waiters go through and >>> nowhere is there a call to reinit_completion() to drain that path. >>> Thanks, >>> >>> Alex >> >> Do you mean this condition? >> >> For device 1: >> error1 occurs ---- error1 resumes >> error2 occurs ---- error2 resumes >> error3 occurs ---- error3 resumes >> >> In current code, I do complete_all() when error1 resumes. >> And this will unblock the device >> when error2 and error3 are still be processed. > > So walk me through how this works. On vfio_pci_open() we call > init_completion(), which sets aer_error_completion.done equal to zero > (BTW, a user can open the device file descriptor multiple times, so > there's already a bug here). I will call init_completion() in vfio_pci_probe. > Let's assume that an error occurs and the > user stalls a single access on wait_for_completion_interruptible(). > The bulk of this function happens here: > > static inline long __sched > do_wait_for_common(struct completion *x, > long (*action)(long), long timeout, int state) > { > if (!x->done) { > DECLARE_WAITQUEUE(wait, current); > > __add_wait_queue_tail_exclusive(&x->wait, &wait); > do { > if (signal_pending_state(state, current)) { > timeout = -ERESTARTSYS; > break; > } > __set_current_state(state); > spin_unlock_irq(&x->wait.lock); > timeout = action(timeout); > spin_lock_irq(&x->wait.lock); > } while (!x->done && timeout); > __remove_wait_queue(&x->wait, &wait); > if (!x->done) > return timeout; > } > x->done--; > return timeout ?: 1; > } > > So it waits within that do{}while loop for a completion, interruption, > or timeout. Then we have: > > void complete_all(struct completion *x) > { > unsigned long flags; > > spin_lock_irqsave(&x->wait.lock, flags); > x->done += UINT_MAX/2; > __wake_up_locked(&x->wait, TASK_NORMAL, 0); > spin_unlock_irqrestore(&x->wait.lock, flags); > } > > So aer_error_completion.done gets incremented to let a couple billion > completion waiters through... Show me how another call to > wait_for_completion_interruptible() will ever block again within our > lifetime when the actual wait of do_wait_for_common() is only entered > when 'done' count is equal to zero. This seems to be why > reinit_completion() exists, but it's not used here. Thanks, > > Alex I will call reinit_completion() in vfio_pci_aer_err_detected when an aer error is detected. Thank you very much. Sincerely ZhouJie ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-02 1:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-19 7:32 [Qemu-devel] [PATCH v2 0/2] vfio: add aer process Zhou Jie 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 1/2] vfio : resume notifier Zhou Jie 2016-07-19 7:39 ` Chen Fan 2016-07-19 7:32 ` [Qemu-devel] [PATCH v2 2/2] vfio : add aer process Zhou Jie 2016-07-29 17:12 ` Alex Williamson 2016-08-01 2:14 ` Zhou Jie 2016-08-01 15:36 ` Alex Williamson 2016-08-02 1:22 ` Zhou Jie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).