Skip to content

Commit 293fbc2

Browse files
awilliamawilliam
committed
vfio/mtty: Overhaul mtty interrupt handling
The mtty driver does not currently conform to the vfio SET_IRQS uAPI. For example, it claims to support mask and unmask of INTx, but actually does nothing. It claims to support AUTOMASK for INTx, but doesn't. It fails to teardown eventfds under the full semantics specified by the SET_IRQS ioctl. It also fails to teardown eventfds when the device is closed, leading to memory leaks. It claims to support the request IRQ, but doesn't. Fix all these. A side effect of this is that QEMU will now report a warning: vfio <uuid>: Failed to set up UNMASK eventfd signaling for interrupt \ INTX-0: VFIO_DEVICE_SET_IRQS failure: Inappropriate ioctl for device The fact is that the unmask eventfd was never supported but quietly failed. mtty never honored the AUTOMASK behavior, therefore there was nothing to unmask. QEMU is verbose about the failure, but properly falls back to userspace unmasking. Fixes: 9d1a546 ("docs: Sample driver to demonstrate how to use Mediated device framework.") Reviewed-by: Cédric Le Goater <clg@redhat.com> Link: https://lore.kernel.org/r/20231016224736.2575718-2-alex.williamson@redhat.com Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
1 parent bd885fc commit 293fbc2

File tree

1 file changed

+166
-73
lines changed

1 file changed

+166
-73
lines changed

‎samples/vfio-mdev/mtty.c

Lines changed: 166 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@ struct serial_port {
127127
/* State of each mdev device */
128128
struct mdev_state {
129129
struct vfio_device vdev;
130-
int irq_fd;
131130
struct eventfd_ctx *intx_evtfd;
132131
struct eventfd_ctx *msi_evtfd;
133132
int irq_index;
@@ -141,6 +140,7 @@ struct mdev_state {
141140
struct mutex rxtx_lock;
142141
struct vfio_device_info dev_info;
143142
int nr_ports;
143+
u8 intx_mask:1;
144144
};
145145

146146
static struct mtty_type {
@@ -166,10 +166,6 @@ static const struct file_operations vd_fops = {
166166

167167
static const struct vfio_device_ops mtty_dev_ops;
168168

169-
/* function prototypes */
170-
171-
static int mtty_trigger_interrupt(struct mdev_state *mdev_state);
172-
173169
/* Helper functions */
174170

175171
static void dump_buffer(u8 *buf, uint32_t count)
@@ -186,6 +182,36 @@ static void dump_buffer(u8 *buf, uint32_t count)
186182
#endif
187183
}
188184

185+
static bool is_intx(struct mdev_state *mdev_state)
186+
{
187+
return mdev_state->irq_index == VFIO_PCI_INTX_IRQ_INDEX;
188+
}
189+
190+
static bool is_msi(struct mdev_state *mdev_state)
191+
{
192+
return mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX;
193+
}
194+
195+
static bool is_noirq(struct mdev_state *mdev_state)
196+
{
197+
return !is_intx(mdev_state) && !is_msi(mdev_state);
198+
}
199+
200+
static void mtty_trigger_interrupt(struct mdev_state *mdev_state)
201+
{
202+
lockdep_assert_held(&mdev_state->ops_lock);
203+
204+
if (is_msi(mdev_state)) {
205+
if (mdev_state->msi_evtfd)
206+
eventfd_signal(mdev_state->msi_evtfd, 1);
207+
} else if (is_intx(mdev_state)) {
208+
if (mdev_state->intx_evtfd && !mdev_state->intx_mask) {
209+
eventfd_signal(mdev_state->intx_evtfd, 1);
210+
mdev_state->intx_mask = true;
211+
}
212+
}
213+
}
214+
189215
static void mtty_create_config_space(struct mdev_state *mdev_state)
190216
{
191217
/* PCI dev ID */
@@ -921,6 +947,25 @@ static ssize_t mtty_write(struct vfio_device *vdev, const char __user *buf,
921947
return -EFAULT;
922948
}
923949

950+
static void mtty_disable_intx(struct mdev_state *mdev_state)
951+
{
952+
if (mdev_state->intx_evtfd) {
953+
eventfd_ctx_put(mdev_state->intx_evtfd);
954+
mdev_state->intx_evtfd = NULL;
955+
mdev_state->intx_mask = false;
956+
mdev_state->irq_index = -1;
957+
}
958+
}
959+
960+
static void mtty_disable_msi(struct mdev_state *mdev_state)
961+
{
962+
if (mdev_state->msi_evtfd) {
963+
eventfd_ctx_put(mdev_state->msi_evtfd);
964+
mdev_state->msi_evtfd = NULL;
965+
mdev_state->irq_index = -1;
966+
}
967+
}
968+
924969
static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
925970
unsigned int index, unsigned int start,
926971
unsigned int count, void *data)
@@ -932,59 +977,113 @@ static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
932977
case VFIO_PCI_INTX_IRQ_INDEX:
933978
switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
934979
case VFIO_IRQ_SET_ACTION_MASK:
980+
if (!is_intx(mdev_state) || start != 0 || count != 1) {
981+
ret = -EINVAL;
982+
break;
983+
}
984+
985+
if (flags & VFIO_IRQ_SET_DATA_NONE) {
986+
mdev_state->intx_mask = true;
987+
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
988+
uint8_t mask = *(uint8_t *)data;
989+
990+
if (mask)
991+
mdev_state->intx_mask = true;
992+
} else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
993+
ret = -ENOTTY; /* No support for mask fd */
994+
}
995+
break;
935996
case VFIO_IRQ_SET_ACTION_UNMASK:
997+
if (!is_intx(mdev_state) || start != 0 || count != 1) {
998+
ret = -EINVAL;
999+
break;
1000+
}
1001+
1002+
if (flags & VFIO_IRQ_SET_DATA_NONE) {
1003+
mdev_state->intx_mask = false;
1004+
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
1005+
uint8_t mask = *(uint8_t *)data;
1006+
1007+
if (mask)
1008+
mdev_state->intx_mask = false;
1009+
} else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
1010+
ret = -ENOTTY; /* No support for unmask fd */
1011+
}
9361012
break;
9371013
case VFIO_IRQ_SET_ACTION_TRIGGER:
938-
{
939-
if (flags & VFIO_IRQ_SET_DATA_NONE) {
940-
pr_info("%s: disable INTx\n", __func__);
941-
if (mdev_state->intx_evtfd)
942-
eventfd_ctx_put(mdev_state->intx_evtfd);
1014+
if (is_intx(mdev_state) && !count &&
1015+
(flags & VFIO_IRQ_SET_DATA_NONE)) {
1016+
mtty_disable_intx(mdev_state);
1017+
break;
1018+
}
1019+
1020+
if (!(is_intx(mdev_state) || is_noirq(mdev_state)) ||
1021+
start != 0 || count != 1) {
1022+
ret = -EINVAL;
9431023
break;
9441024
}
9451025

9461026
if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
9471027
int fd = *(int *)data;
1028+
struct eventfd_ctx *evt;
1029+
1030+
mtty_disable_intx(mdev_state);
1031+
1032+
if (fd < 0)
1033+
break;
9481034

949-
if (fd > 0) {
950-
struct eventfd_ctx *evt;
951-
952-
evt = eventfd_ctx_fdget(fd);
953-
if (IS_ERR(evt)) {
954-
ret = PTR_ERR(evt);
955-
break;
956-
}
957-
mdev_state->intx_evtfd = evt;
958-
mdev_state->irq_fd = fd;
959-
mdev_state->irq_index = index;
1035+
evt = eventfd_ctx_fdget(fd);
1036+
if (IS_ERR(evt)) {
1037+
ret = PTR_ERR(evt);
9601038
break;
9611039
}
1040+
mdev_state->intx_evtfd = evt;
1041+
mdev_state->irq_index = index;
1042+
break;
1043+
}
1044+
1045+
if (!is_intx(mdev_state)) {
1046+
ret = -EINVAL;
1047+
break;
1048+
}
1049+
1050+
if (flags & VFIO_IRQ_SET_DATA_NONE) {
1051+
mtty_trigger_interrupt(mdev_state);
1052+
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
1053+
uint8_t trigger = *(uint8_t *)data;
1054+
1055+
if (trigger)
1056+
mtty_trigger_interrupt(mdev_state);
9621057
}
9631058
break;
9641059
}
965-
}
9661060
break;
9671061
case VFIO_PCI_MSI_IRQ_INDEX:
9681062
switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
9691063
case VFIO_IRQ_SET_ACTION_MASK:
9701064
case VFIO_IRQ_SET_ACTION_UNMASK:
1065+
ret = -ENOTTY;
9711066
break;
9721067
case VFIO_IRQ_SET_ACTION_TRIGGER:
973-
if (flags & VFIO_IRQ_SET_DATA_NONE) {
974-
if (mdev_state->msi_evtfd)
975-
eventfd_ctx_put(mdev_state->msi_evtfd);
976-
pr_info("%s: disable MSI\n", __func__);
977-
mdev_state->irq_index = VFIO_PCI_INTX_IRQ_INDEX;
1068+
if (is_msi(mdev_state) && !count &&
1069+
(flags & VFIO_IRQ_SET_DATA_NONE)) {
1070+
mtty_disable_msi(mdev_state);
9781071
break;
9791072
}
1073+
1074+
if (!(is_msi(mdev_state) || is_noirq(mdev_state)) ||
1075+
start != 0 || count != 1) {
1076+
ret = -EINVAL;
1077+
break;
1078+
}
1079+
9801080
if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
9811081
int fd = *(int *)data;
9821082
struct eventfd_ctx *evt;
9831083

984-
if (fd <= 0)
985-
break;
1084+
mtty_disable_msi(mdev_state);
9861085

987-
if (mdev_state->msi_evtfd)
1086+
if (fd < 0)
9881087
break;
9891088

9901089
evt = eventfd_ctx_fdget(fd);
@@ -993,54 +1092,44 @@ static int mtty_set_irqs(struct mdev_state *mdev_state, uint32_t flags,
9931092
break;
9941093
}
9951094
mdev_state->msi_evtfd = evt;
996-
mdev_state->irq_fd = fd;
9971095
mdev_state->irq_index = index;
1096+
break;
1097+
}
1098+
1099+
if (!is_msi(mdev_state)) {
1100+
ret = -EINVAL;
1101+
break;
1102+
}
1103+
1104+
if (flags & VFIO_IRQ_SET_DATA_NONE) {
1105+
mtty_trigger_interrupt(mdev_state);
1106+
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
1107+
uint8_t trigger = *(uint8_t *)data;
1108+
1109+
if (trigger)
1110+
mtty_trigger_interrupt(mdev_state);
9981111
}
9991112
break;
1000-
}
1001-
break;
1113+
}
1114+
break;
10021115
case VFIO_PCI_MSIX_IRQ_INDEX:
1003-
pr_info("%s: MSIX_IRQ\n", __func__);
1116+
dev_dbg(mdev_state->vdev.dev, "%s: MSIX_IRQ\n", __func__);
1117+
ret = -ENOTTY;
10041118
break;
10051119
case VFIO_PCI_ERR_IRQ_INDEX:
1006-
pr_info("%s: ERR_IRQ\n", __func__);
1120+
dev_dbg(mdev_state->vdev.dev, "%s: ERR_IRQ\n", __func__);
1121+
ret = -ENOTTY;
10071122
break;
10081123
case VFIO_PCI_REQ_IRQ_INDEX:
1009-
pr_info("%s: REQ_IRQ\n", __func__);
1124+
dev_dbg(mdev_state->vdev.dev, "%s: REQ_IRQ\n", __func__);
1125+
ret = -ENOTTY;
10101126
break;
10111127
}
10121128

10131129
mutex_unlock(&mdev_state->ops_lock);
10141130
return ret;
10151131
}
10161132

1017-
static int mtty_trigger_interrupt(struct mdev_state *mdev_state)
1018-
{
1019-
int ret = -1;
1020-
1021-
if ((mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX) &&
1022-
(!mdev_state->msi_evtfd))
1023-
return -EINVAL;
1024-
else if ((mdev_state->irq_index == VFIO_PCI_INTX_IRQ_INDEX) &&
1025-
(!mdev_state->intx_evtfd)) {
1026-
pr_info("%s: Intr eventfd not found\n", __func__);
1027-
return -EINVAL;
1028-
}
1029-
1030-
if (mdev_state->irq_index == VFIO_PCI_MSI_IRQ_INDEX)
1031-
ret = eventfd_signal(mdev_state->msi_evtfd, 1);
1032-
else
1033-
ret = eventfd_signal(mdev_state->intx_evtfd, 1);
1034-
1035-
#if defined(DEBUG_INTR)
1036-
pr_info("Intx triggered\n");
1037-
#endif
1038-
if (ret != 1)
1039-
pr_err("%s: eventfd signal failed (%d)\n", __func__, ret);
1040-
1041-
return ret;
1042-
}
1043-
10441133
static int mtty_get_region_info(struct mdev_state *mdev_state,
10451134
struct vfio_region_info *region_info,
10461135
u16 *cap_type_id, void **cap_type)
@@ -1084,22 +1173,16 @@ static int mtty_get_region_info(struct mdev_state *mdev_state,
10841173

10851174
static int mtty_get_irq_info(struct vfio_irq_info *irq_info)
10861175
{
1087-
switch (irq_info->index) {
1088-
case VFIO_PCI_INTX_IRQ_INDEX:
1089-
case VFIO_PCI_MSI_IRQ_INDEX:
1090-
case VFIO_PCI_REQ_IRQ_INDEX:
1091-
break;
1092-
1093-
default:
1176+
if (irq_info->index != VFIO_PCI_INTX_IRQ_INDEX &&
1177+
irq_info->index != VFIO_PCI_MSI_IRQ_INDEX)
10941178
return -EINVAL;
1095-
}
10961179

10971180
irq_info->flags = VFIO_IRQ_INFO_EVENTFD;
10981181
irq_info->count = 1;
10991182

11001183
if (irq_info->index == VFIO_PCI_INTX_IRQ_INDEX)
1101-
irq_info->flags |= (VFIO_IRQ_INFO_MASKABLE |
1102-
VFIO_IRQ_INFO_AUTOMASKED);
1184+
irq_info->flags |= VFIO_IRQ_INFO_MASKABLE |
1185+
VFIO_IRQ_INFO_AUTOMASKED;
11031186
else
11041187
irq_info->flags |= VFIO_IRQ_INFO_NORESIZE;
11051188

@@ -1262,6 +1345,15 @@ static unsigned int mtty_get_available(struct mdev_type *mtype)
12621345
return atomic_read(&mdev_avail_ports) / type->nr_ports;
12631346
}
12641347

1348+
static void mtty_close(struct vfio_device *vdev)
1349+
{
1350+
struct mdev_state *mdev_state =
1351+
container_of(vdev, struct mdev_state, vdev);
1352+
1353+
mtty_disable_intx(mdev_state);
1354+
mtty_disable_msi(mdev_state);
1355+
}
1356+
12651357
static const struct vfio_device_ops mtty_dev_ops = {
12661358
.name = "vfio-mtty",
12671359
.init = mtty_init_dev,
@@ -1273,6 +1365,7 @@ static const struct vfio_device_ops mtty_dev_ops = {
12731365
.unbind_iommufd = vfio_iommufd_emulated_unbind,
12741366
.attach_ioas = vfio_iommufd_emulated_attach_ioas,
12751367
.detach_ioas = vfio_iommufd_emulated_detach_ioas,
1368+
.close_device = mtty_close,
12761369
};
12771370

12781371
static struct mdev_driver mtty_driver = {

0 commit comments

Comments
 (0)