On 24/04/2020 09:41, Will Deacon wrote: Hi Will, thanks for having a look! > On Thu, Apr 23, 2020 at 06:38:40PM +0100, Andre Przywara wrote: >> At the moment the IRQ line for a virtio-mmio device is assigned in the >> generic device__register() routine in devices.c, by calling back into >> virtio-mmio.c. This does not only sound slightly convoluted, but also >> breaks when we try to register an MMIO device that is not a virtio-mmio >> device. In this case container_of will return a bogus pointer (as it >> assumes a struct virtio_mmio), and the IRQ allocation routine will >> corrupt some data in the device_header (for instance the first byte >> of the "data" pointer). >> >> Simply assign the IRQ directly in virtio_mmio_init(), before calling >> device__register(). This avoids the problem and looks actually much more >> straightforward. >> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> >> --- >> devices.c | 4 ---- >> include/kvm/virtio-mmio.h | 1 - >> virtio/mmio.c | 10 ++-------- >> 3 files changed, 2 insertions(+), 13 deletions(-) >> >> diff --git a/devices.c b/devices.c >> index a7c666a7..2c8b2665 100644 >> --- a/devices.c >> +++ b/devices.c >> @@ -1,7 +1,6 @@ >> #include "kvm/devices.h" >> #include "kvm/kvm.h" >> #include "kvm/pci.h" >> -#include "kvm/virtio-mmio.h" >> >> #include <linux/err.h> >> #include <linux/rbtree.h> >> @@ -33,9 +32,6 @@ int device__register(struct device_header *dev) >> case DEVICE_BUS_PCI: >> pci__assign_irq(dev); >> break; >> - case DEVICE_BUS_MMIO: >> - virtio_mmio_assign_irq(dev); >> - break; > > Hmm, but then it's a bit ugly to handle these differently to PCI. How > difficult is it to add a new bus type instead? e.g. stick the virtio mmio > devices on DEVICE_BUS_VIRTIO_MMIO and then add the non-virtio MMIO devices > to DEVICE_BUS_MMIO? I have another patch to also do the IRQ allocation for PCI devices in their callers. This avoids the allocation on an IRQ for vesa, for instance, but otherwise doesn't solve a real problem, so I didn't post it yet. By looking at devices.c, I feel like this should only be handling the administrative part of managing the device_header structs in the rbtree. Dealing with bus specific things looks out of scope for this file, IMHO. If you agree, I will send the patch shortly. Cheers, Andre
