| Message ID | 20220412021009.582424-2-atishp@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce a new Qemu machine for RISC-V | expand |
On 2022-04-12 03:10, Atish Patra wrote: > The seria-pci device doesn't support MSI. Enable the device to provide > MSI so that any platform with MSI support only can also use > this serial device. MSI can be enabled by enabling the newly introduced > device property. This will be disabled by default preserving the > current > behavior of the seria-pci device. This seems really odd. Switching to MSI implies that you now have an edge signalling. This means that the guest will not be interrupted again if it acks the MSI and doesn't service the device, as you'd expect for a level interrupt (which is what the device generates today). From what I understand of the patch, you signal a MSI on each transition of the device state, which is equally odd (you get an interrupt even where the device goes idle?). While this may work for some guests, this completely changes the semantics of the device. You may want to at least document the new behaviour. Thanks, M. > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > hw/char/serial-pci.c | 36 +++++++++++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c > index 93d6f9924425..ca93c2ce2776 100644 > --- a/hw/char/serial-pci.c > +++ b/hw/char/serial-pci.c > @@ -31,6 +31,7 @@ > #include "hw/char/serial.h" > #include "hw/irq.h" > #include "hw/pci/pci.h" > +#include "hw/pci/msi.h" > #include "hw/qdev-properties.h" > #include "migration/vmstate.h" > #include "qom/object.h" > @@ -39,26 +40,54 @@ struct PCISerialState { > PCIDevice dev; > SerialState state; > uint8_t prog_if; > + bool msi_enabled; > }; > > #define TYPE_PCI_SERIAL "pci-serial" > OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL) > > + > +static void msi_irq_handler(void *opaque, int irq_num, int level) > +{ > + PCIDevice *pci_dev = opaque; > + > + assert(level == 0 || level == 1); > + > + if (msi_enabled(pci_dev)) { > + msi_notify(pci_dev, 0); > + } > +} > + > static void serial_pci_realize(PCIDevice *dev, Error **errp) > { > PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); > SerialState *s = &pci->state; > + Error *err = NULL; > + int ret; > > if (!qdev_realize(DEVICE(s), NULL, errp)) { > return; > } > > pci->dev.config[PCI_CLASS_PROG] = pci->prog_if; > - pci->dev.config[PCI_INTERRUPT_PIN] = 0x01; > - s->irq = pci_allocate_irq(&pci->dev); > - > + if (pci->msi_enabled) { > + pci->dev.config[PCI_INTERRUPT_PIN] = 0x00; > + s->irq = qemu_allocate_irq(msi_irq_handler, &pci->dev, 100); > + } else { > + pci->dev.config[PCI_INTERRUPT_PIN] = 0x01; > + s->irq = pci_allocate_irq(&pci->dev); > + } > memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, > "serial", 8); > pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io); > + > + if (!pci->msi_enabled) { > + return; > + } > + > + ret = msi_init(&pci->dev, 0, 1, true, false, &err); > + if (ret == -ENOTSUP) { > + fprintf(stdout, "MSIX INIT FAILED\n"); > + } > } > > static void serial_pci_exit(PCIDevice *dev) > @@ -83,6 +112,7 @@ static const VMStateDescription vmstate_pci_serial = > { > > static Property serial_pci_properties[] = { > DEFINE_PROP_UINT8("prog_if", PCISerialState, prog_if, 0x02), > + DEFINE_PROP_BOOL("msi", PCISerialState, msi_enabled, false), > DEFINE_PROP_END_OF_LIST(), > };
diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c index 93d6f9924425..ca93c2ce2776 100644 --- a/hw/char/serial-pci.c +++ b/hw/char/serial-pci.c @@ -31,6 +31,7 @@ #include "hw/char/serial.h" #include "hw/irq.h" #include "hw/pci/pci.h" +#include "hw/pci/msi.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" #include "qom/object.h" @@ -39,26 +40,54 @@ struct PCISerialState { PCIDevice dev; SerialState state; uint8_t prog_if; + bool msi_enabled; }; #define TYPE_PCI_SERIAL "pci-serial" OBJECT_DECLARE_SIMPLE_TYPE(PCISerialState, PCI_SERIAL) + +static void msi_irq_handler(void *opaque, int irq_num, int level) +{ + PCIDevice *pci_dev = opaque; + + assert(level == 0 || level == 1); + + if (msi_enabled(pci_dev)) { + msi_notify(pci_dev, 0); + } +} + static void serial_pci_realize(PCIDevice *dev, Error **errp) { PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev); SerialState *s = &pci->state; + Error *err = NULL; + int ret; if (!qdev_realize(DEVICE(s), NULL, errp)) { return; } pci->dev.config[PCI_CLASS_PROG] = pci->prog_if; - pci->dev.config[PCI_INTERRUPT_PIN] = 0x01; - s->irq = pci_allocate_irq(&pci->dev); - + if (pci->msi_enabled) { + pci->dev.config[PCI_INTERRUPT_PIN] = 0x00; + s->irq = qemu_allocate_irq(msi_irq_handler, &pci->dev, 100); + } else { + pci->dev.config[PCI_INTERRUPT_PIN] = 0x01; + s->irq = pci_allocate_irq(&pci->dev); + } memory_region_init_io(&s->io, OBJECT(pci), &serial_io_ops, s, "serial", 8); pci_register_bar(&pci->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io); + + if (!pci->msi_enabled) { + return; + } + + ret = msi_init(&pci->dev, 0, 1, true, false, &err); + if (ret == -ENOTSUP) { + fprintf(stdout, "MSIX INIT FAILED\n"); + } } static void serial_pci_exit(PCIDevice *dev) @@ -83,6 +112,7 @@ static const VMStateDescription vmstate_pci_serial = { static Property serial_pci_properties[] = { DEFINE_PROP_UINT8("prog_if", PCISerialState, prog_if, 0x02), + DEFINE_PROP_BOOL("msi", PCISerialState, msi_enabled, false), DEFINE_PROP_END_OF_LIST(), };
The seria-pci device doesn't support MSI. Enable the device to provide MSI so that any platform with MSI support only can also use this serial device. MSI can be enabled by enabling the newly introduced device property. This will be disabled by default preserving the current behavior of the seria-pci device. Signed-off-by: Atish Patra <atishp@rivosinc.com> --- hw/char/serial-pci.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-)