* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 16:07 [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs Thomas Huth
@ 2019-02-19 17:23 ` Philippe Mathieu-Daudé
2019-02-19 18:24 ` Paolo Bonzini
2019-02-19 20:19 ` Michael S. Tsirkin
2 siblings, 0 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-02-19 17:23 UTC (permalink / raw)
To: Thomas Huth, Marcel Apfelbaum, Michael S. Tsirkin
Cc: qemu-trivial, yang.zhong, qemu-devel, pbonzini
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();
> +}
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 16:07 [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs Thomas Huth
2019-02-19 17:23 ` Philippe Mathieu-Daudé
@ 2019-02-19 18:24 ` Paolo Bonzini
2019-02-19 20:23 ` Michael S. Tsirkin
2019-02-19 20:19 ` Michael S. Tsirkin
2 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-02-19 18:24 UTC (permalink / raw)
To: Thomas Huth, Marcel Apfelbaum, Michael S. Tsirkin
Cc: qemu-devel, qemu-trivial, yang.zhong
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 18:24 ` Paolo Bonzini
@ 2019-02-19 20:23 ` Michael S. Tsirkin
2019-02-19 23:19 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-19 20:23 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Thomas Huth, Marcel Apfelbaum, qemu-devel, qemu-trivial,
yang.zhong
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 20:23 ` Michael S. Tsirkin
@ 2019-02-19 23:19 ` Paolo Bonzini
2019-02-20 3:35 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-02-19 23:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Thomas Huth, Marcel Apfelbaum, qemu-devel, qemu-trivial,
yang zhong
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 23:19 ` Paolo Bonzini
@ 2019-02-20 3:35 ` Michael S. Tsirkin
0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-20 3:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Thomas Huth, Marcel Apfelbaum, qemu-devel, qemu-trivial,
yang zhong, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 16:07 [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs Thomas Huth
2019-02-19 17:23 ` Philippe Mathieu-Daudé
2019-02-19 18:24 ` Paolo Bonzini
@ 2019-02-19 20:19 ` Michael S. Tsirkin
2019-02-20 6:24 ` Thomas Huth
2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-19 20:19 UTC (permalink / raw)
To: Thomas Huth
Cc: Marcel Apfelbaum, qemu-devel, qemu-trivial, yang.zhong, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-19 20:19 ` Michael S. Tsirkin
@ 2019-02-20 6:24 ` Thomas Huth
2019-02-21 16:55 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2019-02-20 6:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Marcel Apfelbaum, qemu-devel, qemu-trivial, yang.zhong, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-20 6:24 ` Thomas Huth
@ 2019-02-21 16:55 ` Michael S. Tsirkin
2019-02-21 18:00 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2019-02-21 16:55 UTC (permalink / raw)
To: Thomas Huth
Cc: Marcel Apfelbaum, qemu-devel, qemu-trivial, yang.zhong, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/pci/pci-stub: Add msi_enabled() and msi_notify() to the pci stubs
2019-02-21 16:55 ` Michael S. Tsirkin
@ 2019-02-21 18:00 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2019-02-21 18:00 UTC (permalink / raw)
To: Michael S. Tsirkin, Thomas Huth
Cc: Marcel Apfelbaum, qemu-devel, qemu-trivial, yang.zhong
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
^ permalink raw reply [flat|nested] 10+ messages in thread