| Message ID | 256afbb2da005dc62c159b0f4a4fc0d95c050660.1552679970.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] riscv: plic: Set msi_nonbroken as true | expand |
On 15/03/19 21:05, Alistair Francis wrote: > Set msi_nonbroken as true for the PLIC. > > According to the comment located here: > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > the msi_nonbroken variable should be set to true even if they don't > support MSI. In this case that is what we are doing as we don't support > MSI. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reported-by: Andrea Bolognani <abologna@redhat.com> > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > --- > This should allow working pcie-root-ports in QEMU and allow libvirt > to start using PCIe by default for RISC-V guests. > > hw/riscv/sifive_plic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c > index d12ec3fc9a..4b0537c912 100644 > --- a/hw/riscv/sifive_plic.c > +++ b/hw/riscv/sifive_plic.c > @@ -22,6 +22,7 @@ > #include "qemu/log.h" > #include "qemu/error-report.h" > #include "hw/sysbus.h" > +#include "hw/pci/msi.h" > #include "target/riscv/cpu.h" > #include "hw/riscv/sifive_plic.h" > > @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) > plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); > qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); > + > + msi_nonbroken = true; > } > > static void sifive_plic_class_init(ObjectClass *klass, void *data) > I can queue this patch, and add the "select MSI" to CONFIG_SIFIVE. Paolo
Alistair Francis <Alistair.Francis@wdc.com> writes: > Set msi_nonbroken as true for the PLIC. > > According to the comment located here: > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > the msi_nonbroken variable should be set to true even if they don't > support MSI. In this case that is what we are doing as we don't support > MSI. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reported-by: Andrea Bolognani <abologna@redhat.com> > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > --- > This should allow working pcie-root-ports in QEMU and allow libvirt > to start using PCIe by default for RISC-V guests. Lovely! If more people reviewed and updated their interrupt controllers this way, we'd be in better shape.
On Mon, 2019-03-18 at 09:39 +0100, Paolo Bonzini wrote: > On 15/03/19 21:05, Alistair Francis wrote: > > Set msi_nonbroken as true for the PLIC. > > > > According to the comment located here: > > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > > the msi_nonbroken variable should be set to true even if they don't > > support MSI. In this case that is what we are doing as we don't support > > MSI. > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > Reported-by: Andrea Bolognani <abologna@redhat.com> > > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > > --- > > This should allow working pcie-root-ports in QEMU and allow libvirt > > to start using PCIe by default for RISC-V guests. > > > > hw/riscv/sifive_plic.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c > > index d12ec3fc9a..4b0537c912 100644 > > --- a/hw/riscv/sifive_plic.c > > +++ b/hw/riscv/sifive_plic.c > > @@ -22,6 +22,7 @@ > > #include "qemu/log.h" > > #include "qemu/error-report.h" > > #include "hw/sysbus.h" > > +#include "hw/pci/msi.h" > > #include "target/riscv/cpu.h" > > #include "hw/riscv/sifive_plic.h" > > > > @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) > > plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); > > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); > > qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); > > + > > + msi_nonbroken = true; > > } > > > > static void sifive_plic_class_init(ObjectClass *klass, void *data) > > I can queue this patch, and add the "select MSI" to CONFIG_SIFIVE. The interrupt controller is used by the virt machine type too IIUC, so the same should be added to CONFIG_RISCV_VIRT I think.
On Mon, 18 Mar 2019 01:39:46 PDT (-0700), pbonzini@redhat.com wrote: > On 15/03/19 21:05, Alistair Francis wrote: >> Set msi_nonbroken as true for the PLIC. >> >> According to the comment located here: >> https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 >> the msi_nonbroken variable should be set to true even if they don't >> support MSI. In this case that is what we are doing as we don't support >> MSI. >> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >> Reported-by: Andrea Bolognani <abologna@redhat.com> >> Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> >> --- >> This should allow working pcie-root-ports in QEMU and allow libvirt >> to start using PCIe by default for RISC-V guests. >> >> hw/riscv/sifive_plic.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c >> index d12ec3fc9a..4b0537c912 100644 >> --- a/hw/riscv/sifive_plic.c >> +++ b/hw/riscv/sifive_plic.c >> @@ -22,6 +22,7 @@ >> #include "qemu/log.h" >> #include "qemu/error-report.h" >> #include "hw/sysbus.h" >> +#include "hw/pci/msi.h" >> #include "target/riscv/cpu.h" >> #include "hw/riscv/sifive_plic.h" >> >> @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) >> plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); >> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); >> qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); >> + >> + msi_nonbroken = true; >> } >> >> static void sifive_plic_class_init(ObjectClass *klass, void *data) >> > > I can queue this patch, and add the "select MSI" to CONFIG_SIFIVE. Works for me. Thanks!
On Fri, 2019-03-15 at 20:05 +0000, Alistair Francis wrote: > Set msi_nonbroken as true for the PLIC. > > According to the comment located here: > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > the msi_nonbroken variable should be set to true even if they don't > support MSI. In this case that is what we are doing as we don't support > MSI. > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reported-by: Andrea Bolognani <abologna@redhat.com> > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > --- > This should allow working pcie-root-ports in QEMU and allow libvirt > to start using PCIe by default for RISC-V guests. > > hw/riscv/sifive_plic.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c > index d12ec3fc9a..4b0537c912 100644 > --- a/hw/riscv/sifive_plic.c > +++ b/hw/riscv/sifive_plic.c > @@ -22,6 +22,7 @@ > #include "qemu/log.h" > #include "qemu/error-report.h" > #include "hw/sysbus.h" > +#include "hw/pci/msi.h" > #include "target/riscv/cpu.h" > #include "hw/riscv/sifive_plic.h" > > @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) > plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); > qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); > + > + msi_nonbroken = true; > } > > static void sifive_plic_class_init(ObjectClass *klass, void *data) With this patch applied, I was able to bring up a riscv64/virt guest with graphics, using PCIe devices only: https://imgur.com/a/taN06hE Tested-by: Andrea Bolognani <abologna@redhat.com>
On Mon, Mar 18, 2019 at 10:22 AM Andrea Bolognani <abologna@redhat.com> wrote: > > On Mon, 2019-03-18 at 09:39 +0100, Paolo Bonzini wrote: > > On 15/03/19 21:05, Alistair Francis wrote: > > > Set msi_nonbroken as true for the PLIC. > > > > > > According to the comment located here: > > > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > > > the msi_nonbroken variable should be set to true even if they don't > > > support MSI. In this case that is what we are doing as we don't support > > > MSI. > > > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > > Reported-by: Andrea Bolognani <abologna@redhat.com> > > > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > > > --- > > > This should allow working pcie-root-ports in QEMU and allow libvirt > > > to start using PCIe by default for RISC-V guests. > > > > > > hw/riscv/sifive_plic.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c > > > index d12ec3fc9a..4b0537c912 100644 > > > --- a/hw/riscv/sifive_plic.c > > > +++ b/hw/riscv/sifive_plic.c > > > @@ -22,6 +22,7 @@ > > > #include "qemu/log.h" > > > #include "qemu/error-report.h" > > > #include "hw/sysbus.h" > > > +#include "hw/pci/msi.h" > > > #include "target/riscv/cpu.h" > > > #include "hw/riscv/sifive_plic.h" > > > > > > @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) > > > plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); > > > sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); > > > qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); > > > + > > > + msi_nonbroken = true; > > > } > > > > > > static void sifive_plic_class_init(ObjectClass *klass, void *data) > > > > I can queue this patch, and add the "select MSI" to CONFIG_SIFIVE. > > The interrupt controller is used by the virt machine type too IIUC, > so the same should be added to CONFIG_RISCV_VIRT I think. CONFIG_SIFIVE is selected by CONFIG_RISCV_VIRT thus we should be good. > > -- > Andrea Bolognani / Red Hat / Virtualization >
On Mon, 18 Mar 2019 at 08:59, Markus Armbruster <armbru@redhat.com> wrote: > > Alistair Francis <Alistair.Francis@wdc.com> writes: > > > Set msi_nonbroken as true for the PLIC. > > > > According to the comment located here: > > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > > the msi_nonbroken variable should be set to true even if they don't > > support MSI. In this case that is what we are doing as we don't support > > MSI. > > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > > Reported-by: Andrea Bolognani <abologna@redhat.com> > > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> > > --- > > This should allow working pcie-root-ports in QEMU and allow libvirt > > to start using PCIe by default for RISC-V guests. > > Lovely! If more people reviewed and updated their interrupt controllers > this way, we'd be in better shape. Why do we have a flag which each interrupt controller has to set rather than just making the right thing the default (and having the one or two interrupt controllers that need the wrong thing for backwards compatibility reasons be the ones that have to set the flag) ? This way round makes it way to easy to add a new interrupt controller with this bug without noticing it, I think. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 18 Mar 2019 at 08:59, Markus Armbruster <armbru@redhat.com> wrote: >> >> Alistair Francis <Alistair.Francis@wdc.com> writes: >> >> > Set msi_nonbroken as true for the PLIC. >> > >> > According to the comment located here: >> > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 >> > the msi_nonbroken variable should be set to true even if they don't >> > support MSI. In this case that is what we are doing as we don't support >> > MSI. >> > >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >> > Reported-by: Andrea Bolognani <abologna@redhat.com> >> > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> >> > --- >> > This should allow working pcie-root-ports in QEMU and allow libvirt >> > to start using PCIe by default for RISC-V guests. >> >> Lovely! If more people reviewed and updated their interrupt controllers >> this way, we'd be in better shape. > > Why do we have a flag which each interrupt controller > has to set rather than just making the right thing the > default (and having the one or two interrupt controllers > that need the wrong thing for backwards compatibility reasons > be the ones that have to set the flag) ? This way round makes > it way to easy to add a new interrupt controller with this > bug without noticing it, I think. This is ultimately a question for Michael (cc'ed). However, I can provide a bit of context right away. The problem is virtual interrupt controllers that claim to support MSI when they don't. The OS's probe returns "go ahead and use MSI", and the system falls apart. So we put in a lame work-around to masks these interrupt controller bugs: if MSI is broken, mangle all PCI devices to make them deny MSI capability. The next problem is that we don't even know which of our interrupt controllers have MSI working. So we summarily declare them all broken, then have the few we actually know declare themselves non-broken. The next problem is having multiple interrupt controllers, some broken, some not. The work-around falls apart there. We currently use the ostrich algorithm to deal with that. More information in the thread around Message-ID: <87wppi1vol.fsf@blackfin.pond.sub.org> https://lists.nongnu.org/archive/html/qemu-devel/2016-03/msg00983.html
On Mon, 2019-03-18 at 02:31 -0700, Palmer Dabbelt wrote: > On Mon, 18 Mar 2019 01:39:46 PDT (-0700), pbonzini@redhat.com wrote: > > On 15/03/19 21:05, Alistair Francis wrote: > > > Set msi_nonbroken as true for the PLIC. > > > > > > According to the comment located here: > > > https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 > > > the msi_nonbroken variable should be set to true even if they don't > > > support MSI. In this case that is what we are doing as we don't support > > > MSI. > > > > I can queue this patch, and add the "select MSI" to CONFIG_SIFIVE. > > Works for me. Thanks! Just so we're on the same page, are you targeting this at 4.0.0? If it gets merged in the next few days I can probably get the corresponding libvirt patches in before our own freeze starts. It would be great if we could make it so guests created with QEMU 4.0.0 + libvirt 5.2.0 get PCI by default :)
On 21/03/19 12:56, Andrea Bolognani wrote: > Just so we're on the same page, are you targeting this at 4.0.0? > If it gets merged in the next few days I can probably get the > corresponding libvirt patches in before our own freeze starts. > > It would be great if we could make it so guests created with > QEMU 4.0.0 + libvirt 5.2.0 get PCI by default :) Yes, I'll send the pull request tomorrow. Paolo
diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c index d12ec3fc9a..4b0537c912 100644 --- a/hw/riscv/sifive_plic.c +++ b/hw/riscv/sifive_plic.c @@ -22,6 +22,7 @@ #include "qemu/log.h" #include "qemu/error-report.h" #include "hw/sysbus.h" +#include "hw/pci/msi.h" #include "target/riscv/cpu.h" #include "hw/riscv/sifive_plic.h" @@ -443,6 +444,8 @@ static void sifive_plic_realize(DeviceState *dev, Error **errp) plic->enable = g_new0(uint32_t, plic->bitfield_words * plic->num_addrs); sysbus_init_mmio(SYS_BUS_DEVICE(dev), &plic->mmio); qdev_init_gpio_in(dev, sifive_plic_irq_request, plic->num_sources); + + msi_nonbroken = true; } static void sifive_plic_class_init(ObjectClass *klass, void *data)
Set msi_nonbroken as true for the PLIC. According to the comment located here: https://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci/msi.c;h=47d2b0f33c664533b8dbd5cb17faa8e6a01afe1f;hb=HEAD#l38 the msi_nonbroken variable should be set to true even if they don't support MSI. In this case that is what we are doing as we don't support MSI. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> Reported-by: Andrea Bolognani <abologna@redhat.com> Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com> --- This should allow working pcie-root-ports in QEMU and allow libvirt to start using PCIe by default for RISC-V guests. hw/riscv/sifive_plic.c | 3 +++ 1 file changed, 3 insertions(+)