[Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Alistair Francis]

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)
-- 
2.21.0

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Paolo Bonzini]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Markus Armbruster]

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.

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Andrea Bolognani]

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.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Palmer Dabbelt]

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!

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Andrea Bolognani]

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>

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[David Abdurachmanov]

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
>

Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Peter Maydell]

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

Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Markus Armbruster]

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

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Andrea Bolognani]

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 :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-riscv] [PATCH v1 1/1] riscv: plic: Set msi_nonbroken as true

[Paolo Bonzini]

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