* [PATCH] vfio/pci: Separate INTx-enabled vfio_pci_device from unenabled to make the code logic clearer.
@ 2023-11-20 3:17 JianChunfu
2023-11-20 17:38 ` Alex Williamson
0 siblings, 1 reply; 2+ messages in thread
From: JianChunfu @ 2023-11-20 3:17 UTC (permalink / raw)
To: alex.williamson, cohuck; +Cc: kvm, linux-kernel, shenghui.qu, JianChunfu
It seems a little unclear when dealing with vfio_intx_set_signal()
because of vfio_pci_device which is irq_none,
so separate the two situations.
Signed-off-by: JianChunfu <chunfu.jian@shingroup.cn>
---
drivers/vfio/pci/vfio_pci_intrs.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 6069a11fb51a..b6d126c48393 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -468,6 +468,8 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
unsigned index, unsigned start,
unsigned count, uint32_t flags, void *data)
{
+ int32_t fd = *(int32_t *)data;
+
if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
vfio_intx_disable(vdev);
return 0;
@@ -476,28 +478,25 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
if (!(is_intx(vdev) || is_irq_none(vdev)) || start != 0 || count != 1)
return -EINVAL;
- if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
- int32_t fd = *(int32_t *)data;
+ if (!is_intx(vdev)) {
int ret;
+ if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+ ret = vfio_intx_enable(vdev);
+ if (ret)
+ return ret;
- if (is_intx(vdev))
- return vfio_intx_set_signal(vdev, fd);
+ ret = vfio_intx_set_signal(vdev, fd);
+ if (ret)
+ vfio_intx_disable(vdev);
- ret = vfio_intx_enable(vdev);
- if (ret)
return ret;
-
- ret = vfio_intx_set_signal(vdev, fd);
- if (ret)
- vfio_intx_disable(vdev);
-
- return ret;
+ } else
+ return -EINVAL;
}
- if (!is_intx(vdev))
- return -EINVAL;
-
- if (flags & VFIO_IRQ_SET_DATA_NONE) {
+ if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
+ return vfio_intx_set_signal(vdev, fd);
+ } else if (flags & VFIO_IRQ_SET_DATA_NONE) {
vfio_send_intx_eventfd(vdev, NULL);
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
uint8_t trigger = *(uint8_t *)data;
--
2.27.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] vfio/pci: Separate INTx-enabled vfio_pci_device from unenabled to make the code logic clearer.
2023-11-20 3:17 [PATCH] vfio/pci: Separate INTx-enabled vfio_pci_device from unenabled to make the code logic clearer JianChunfu
@ 2023-11-20 17:38 ` Alex Williamson
0 siblings, 0 replies; 2+ messages in thread
From: Alex Williamson @ 2023-11-20 17:38 UTC (permalink / raw)
To: JianChunfu; +Cc: cohuck, kvm, linux-kernel, shenghui.qu
On Mon, 20 Nov 2023 11:17:52 +0800
JianChunfu <chunfu.jian@shingroup.cn> wrote:
> It seems a little unclear when dealing with vfio_intx_set_signal()
> because of vfio_pci_device which is irq_none,
> so separate the two situations.
>
> Signed-off-by: JianChunfu <chunfu.jian@shingroup.cn>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 6069a11fb51a..b6d126c48393 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -468,6 +468,8 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> unsigned index, unsigned start,
> unsigned count, uint32_t flags, void *data)
> {
> + int32_t fd = *(int32_t *)data;
This is a null pointer dereference if anyone were to use
VFIO_IRQ_SET_DATA_NONE. Note this is also invalid for
VFIO_IRQ_SET_DATA_BOOL. I think this is largely why the function has
the current layout.
> +
> if (is_intx(vdev) && !count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
> vfio_intx_disable(vdev);
> return 0;
> @@ -476,28 +478,25 @@ static int vfio_pci_set_intx_trigger(struct vfio_pci_core_device *vdev,
> if (!(is_intx(vdev) || is_irq_none(vdev)) || start != 0 || count != 1)
> return -EINVAL;
>
> - if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> - int32_t fd = *(int32_t *)data;
> + if (!is_intx(vdev)) {
> int ret;
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
@ret should be scoped within this branch with this layout and there
should be a blank line after variable declaration.
> + ret = vfio_intx_enable(vdev);
> + if (ret)
> + return ret;
>
> - if (is_intx(vdev))
> - return vfio_intx_set_signal(vdev, fd);
> + ret = vfio_intx_set_signal(vdev, fd);
> + if (ret)
> + vfio_intx_disable(vdev);
>
> - ret = vfio_intx_enable(vdev);
> - if (ret)
> return ret;
> -
> - ret = vfio_intx_set_signal(vdev, fd);
> - if (ret)
> - vfio_intx_disable(vdev);
> -
> - return ret;
> + } else
> + return -EINVAL;
Single line branches also get braces if the previous branch required
braces. Thanks,
Alex
> }
>
> - if (!is_intx(vdev))
> - return -EINVAL;
> -
> - if (flags & VFIO_IRQ_SET_DATA_NONE) {
> + if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + return vfio_intx_set_signal(vdev, fd);
> + } else if (flags & VFIO_IRQ_SET_DATA_NONE) {
> vfio_send_intx_eventfd(vdev, NULL);
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> uint8_t trigger = *(uint8_t *)data;
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-11-20 17:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-20 3:17 [PATCH] vfio/pci: Separate INTx-enabled vfio_pci_device from unenabled to make the code logic clearer JianChunfu
2023-11-20 17:38 ` Alex Williamson
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).