[Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Thomas Huth]

Some machines have an AHCI adapter, but no PCI. To be able to
compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
functions msi_enabled() and msi_notify() for linking.
This is required for the upcoming Kconfig-like build system, if
a user wants to compile a QEMU binary with just one machine that
has AHCI, but no PCI, like the ARM "cubieboard" for example.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/pci/pci-stub.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index b941a0e..c04a5df 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -53,3 +53,14 @@ uint16_t pci_requester_id(PCIDevice *dev)
     g_assert(false);
     return 0;
 }
+
+/* Required by ahci.c */
+bool msi_enabled(const PCIDevice *dev)
+{
+    return false;
+}
+
+void msi_notify(PCIDevice *dev, unsigned int vector)
+{
+    g_assert_not_reached();
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Philippe Mathieu-Daudé]

On 2/19/19 5:07 PM, Thomas Huth wrote:
> Some machines have an AHCI adapter, but no PCI. To be able to
> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
> functions msi_enabled() and msi_notify() for linking.
> This is required for the upcoming Kconfig-like build system, if
> a user wants to compile a QEMU binary with just one machine that
> has AHCI, but no PCI, like the ARM "cubieboard" for example.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/pci/pci-stub.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> index b941a0e..c04a5df 100644
> --- a/hw/pci/pci-stub.c
> +++ b/hw/pci/pci-stub.c
> @@ -53,3 +53,14 @@ uint16_t pci_requester_id(PCIDevice *dev)
>      g_assert(false);
>      return 0;
>  }
> +
> +/* Required by ahci.c */
> +bool msi_enabled(const PCIDevice *dev)
> +{
> +    return false;
> +}
> +
> +void msi_notify(PCIDevice *dev, unsigned int vector)
> +{
> +    g_assert_not_reached();
> +}
> 

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Paolo Bonzini]

On 19/02/19 17:07, Thomas Huth wrote:
> Some machines have an AHCI adapter, but no PCI. To be able to
> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
> functions msi_enabled() and msi_notify() for linking.
> This is required for the upcoming Kconfig-like build system, if
> a user wants to compile a QEMU binary with just one machine that
> has AHCI, but no PCI, like the ARM "cubieboard" for example.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/pci/pci-stub.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> index b941a0e..c04a5df 100644
> --- a/hw/pci/pci-stub.c
> +++ b/hw/pci/pci-stub.c
> @@ -53,3 +53,14 @@ uint16_t pci_requester_id(PCIDevice *dev)
>      g_assert(false);
>      return 0;
>  }
> +
> +/* Required by ahci.c */
> +bool msi_enabled(const PCIDevice *dev)
> +{
> +    return false;
> +}
> +
> +void msi_notify(PCIDevice *dev, unsigned int vector)
> +{
> +    g_assert_not_reached();
> +}
> 

Makes sense, but it is also abstraction time. :)  What if instead there
was a function

void msi_allocate_irqs(PCIDevice *pdev, int num, bool fallback_to_intx);

and then ich.c did

    irqs = msi_allocate_irqs(pdev, 1, true);
    s->irq = irqs[0];
    g_free(irqs);

?  "if msi_enabled raise MSI else raise INTX" is really a common idiom.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Michael S. Tsirkin]

On Tue, Feb 19, 2019 at 05:07:39PM +0100, Thomas Huth wrote:
> Some machines have an AHCI adapter, but no PCI. To be able to
> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
> functions msi_enabled() and msi_notify() for linking.
> This is required for the upcoming Kconfig-like build system, if
> a user wants to compile a QEMU binary with just one machine that
> has AHCI, but no PCI, like the ARM "cubieboard" for example.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Do you want me to merge this or do you prefer to
merge it with kconfig patches?

> ---
>  hw/pci/pci-stub.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> index b941a0e..c04a5df 100644
> --- a/hw/pci/pci-stub.c
> +++ b/hw/pci/pci-stub.c
> @@ -53,3 +53,14 @@ uint16_t pci_requester_id(PCIDevice *dev)
>      g_assert(false);
>      return 0;
>  }
> +
> +/* Required by ahci.c */
> +bool msi_enabled(const PCIDevice *dev)
> +{
> +    return false;
> +}
> +
> +void msi_notify(PCIDevice *dev, unsigned int vector)
> +{
> +    g_assert_not_reached();
> +}
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Michael S. Tsirkin]

On Tue, Feb 19, 2019 at 07:24:40PM +0100, Paolo Bonzini wrote:
> On 19/02/19 17:07, Thomas Huth wrote:
> > Some machines have an AHCI adapter, but no PCI. To be able to
> > compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
> > functions msi_enabled() and msi_notify() for linking.
> > This is required for the upcoming Kconfig-like build system, if
> > a user wants to compile a QEMU binary with just one machine that
> > has AHCI, but no PCI, like the ARM "cubieboard" for example.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  hw/pci/pci-stub.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
> > index b941a0e..c04a5df 100644
> > --- a/hw/pci/pci-stub.c
> > +++ b/hw/pci/pci-stub.c
> > @@ -53,3 +53,14 @@ uint16_t pci_requester_id(PCIDevice *dev)
> >      g_assert(false);
> >      return 0;
> >  }
> > +
> > +/* Required by ahci.c */
> > +bool msi_enabled(const PCIDevice *dev)
> > +{
> > +    return false;
> > +}
> > +
> > +void msi_notify(PCIDevice *dev, unsigned int vector)
> > +{
> > +    g_assert_not_reached();
> > +}
> > 
> 
> Makes sense, but it is also abstraction time. :)  What if instead there
> was a function
> 
> void msi_allocate_irqs(PCIDevice *pdev, int num, bool fallback_to_intx);
> 
> and then ich.c did
> 
>     irqs = msi_allocate_irqs(pdev, 1, true);
>     s->irq = irqs[0];
>     g_free(irqs);
> 
> ?  "if msi_enabled raise MSI else raise INTX" is really a common idiom.
> 
> Thanks,
> 
> Paolo

Maybe it is but the specific issue is not about fallback to INTX of PCI
(is the fallback broken for ahci? I don't know).
The trick is there's no pdev at all.

-- 
MST

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Paolo Bonzini]

> > Makes sense, but it is also abstraction time. :)  What if instead there
> > was a function
> > 
> > void msi_allocate_irqs(PCIDevice *pdev, int num, bool fallback_to_intx);
> > 
> > and then ich.c did
> > 
> >     irqs = msi_allocate_irqs(pdev, 1, true);
> >     s->irq = irqs[0];
> >     g_free(irqs);
> > 
> > ?  "if msi_enabled raise MSI else raise INTX" is really a common idiom.
> > 
> > Thanks,
> > 
> > Paolo
> 
> Maybe it is but the specific issue is not about fallback to INTX of PCI
> (is the fallback broken for ahci? I don't know).

It works, the above is just a new abstraction.

> The trick is there's no pdev at all.

The trick :) is that in ich.c there is a pdev.  Right now we are assigning to
s->irq either the INTX irq (if PCI) or a sysbus irq (if sysbus), but then
we need to know about MSI with a wrapper around s->irq.

Instead, my suggestion is to put the wrapper in the PCI core as a qemu_irq
callback---or perhaps in ich.c, but anyway ahci.c should not care that there
could be a PCI AHCI device and it would have two different interrupt modes.
In fact, doing this would also remove the need for s->container, I think.

Paolo

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Michael S. Tsirkin]

On Tue, Feb 19, 2019 at 06:19:30PM -0500, Paolo Bonzini wrote:
> 
> > > Makes sense, but it is also abstraction time. :)  What if instead there
> > > was a function
> > > 
> > > void msi_allocate_irqs(PCIDevice *pdev, int num, bool fallback_to_intx);
> > > 
> > > and then ich.c did
> > > 
> > >     irqs = msi_allocate_irqs(pdev, 1, true);
> > >     s->irq = irqs[0];
> > >     g_free(irqs);
> > > 
> > > ?  "if msi_enabled raise MSI else raise INTX" is really a common idiom.
> > > 
> > > Thanks,
> > > 
> > > Paolo
> > 
> > Maybe it is but the specific issue is not about fallback to INTX of PCI
> > (is the fallback broken for ahci? I don't know).
> 
> It works, the above is just a new abstraction.
> 
> > The trick is there's no pdev at all.
> 
> The trick :) is that in ich.c there is a pdev.  Right now we are assigning to
> s->irq either the INTX irq (if PCI) or a sysbus irq (if sysbus), but then
> we need to know about MSI with a wrapper around s->irq.

Oh you mean just for PCI.

> Instead, my suggestion is to put the wrapper in the PCI core as a qemu_irq
> callback---or perhaps in ich.c, but anyway ahci.c should not care that there
> could be a PCI AHCI device and it would have two different interrupt modes.

I like it very much that devices call pci_set_irq, I'd rather not
have callbacks.


I think the wrapper thay calls either pci_set_irq isn't a problem,
problem is MSI/X has multiple vectors, INTX doesn't.
So for many devices there's something extra that happens
just in one mode but not the other to deal with multiple vectors.

So I don't think it can be an abstraction that everyone
uses. But yes it can be a helper function.

In fact mptsas_update_interrupt seems not to be
PCI spec compliant: it sets both MSI and INTX.

CC original contributor with this question.



> In fact, doing this would also remove the need for s->container, I think.
> 
> Paolo

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Thomas Huth]

On 19/02/2019 21.19, Michael S. Tsirkin wrote:
> On Tue, Feb 19, 2019 at 05:07:39PM +0100, Thomas Huth wrote:
>> Some machines have an AHCI adapter, but no PCI. To be able to
>> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
>> functions msi_enabled() and msi_notify() for linking.
>> This is required for the upcoming Kconfig-like build system, if
>> a user wants to compile a QEMU binary with just one machine that
>> has AHCI, but no PCI, like the ARM "cubieboard" for example.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Thanks!

> Do you want me to merge this or do you prefer to
> merge it with kconfig patches?

If you plan a pci pull request soon, feel free to take it. Otherwise, I
can also add it to my Kconfig-for-arm patch series where I need it.

 Thomas

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Michael S. Tsirkin]

On Wed, Feb 20, 2019 at 07:24:00AM +0100, Thomas Huth wrote:
> On 19/02/2019 21.19, Michael S. Tsirkin wrote:
> > On Tue, Feb 19, 2019 at 05:07:39PM +0100, Thomas Huth wrote:
> >> Some machines have an AHCI adapter, but no PCI. To be able to
> >> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
> >> functions msi_enabled() and msi_notify() for linking.
> >> This is required for the upcoming Kconfig-like build system, if
> >> a user wants to compile a QEMU binary with just one machine that
> >> has AHCI, but no PCI, like the ARM "cubieboard" for example.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Thanks!
> 
> > Do you want me to merge this or do you prefer to
> > merge it with kconfig patches?
> 
> If you plan a pci pull request soon, feel free to take it. Otherwise, I
> can also add it to my Kconfig-for-arm patch series where I need it.
> 
>  Thomas

Feel free to include but please talk to Paolo about his
ideas for abstracting it out.

Thanks!

-- 
MST

Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs

[Paolo Bonzini]

On 21/02/19 17:55, Michael S. Tsirkin wrote:
> On Wed, Feb 20, 2019 at 07:24:00AM +0100, Thomas Huth wrote:
>> On 19/02/2019 21.19, Michael S. Tsirkin wrote:
>>> On Tue, Feb 19, 2019 at 05:07:39PM +0100, Thomas Huth wrote:
>>>> Some machines have an AHCI adapter, but no PCI. To be able to
>>>> compile hw/ide/ahci.c without CONFIG_PCI, we still need the two
>>>> functions msi_enabled() and msi_notify() for linking.
>>>> This is required for the upcoming Kconfig-like build system, if
>>>> a user wants to compile a QEMU binary with just one machine that
>>>> has AHCI, but no PCI, like the ARM "cubieboard" for example.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Thanks!
>>
>>> Do you want me to merge this or do you prefer to
>>> merge it with kconfig patches?
>>
>> If you plan a pci pull request soon, feel free to take it. Otherwise, I
>> can also add it to my Kconfig-for-arm patch series where I need it.
>>
>>  Thomas
> 
> Feel free to include but please talk to Paolo about his
> ideas for abstracting it out.

Not a blocker for me.

Paolo