[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init() |
Date: |
Mon, 29 May 2017 14:16:30 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Peter Xu <address@hidden> writes:
> On Mon, May 29, 2017 at 11:42:35AM +0200, Markus Armbruster wrote:
>> Peter Xu <address@hidden> writes:
>>
>> > On Mon, May 15, 2017 at 09:14:33PM +0800, Peter Xu wrote:
>> >> MSI should be supported by all interrupt controllers. Switching the old
>> >> check for msi_nonbroken into assertion. Do similar thing to
>> >> pci_add_capability2() below that. Then time to remove *errp.
>> >>
>> >> Since msi_init() won't fail now, touch up all the callers to avoid
>> >> checks against it. One side effect is that we fixed a possible leak in
>> >> current edu device.
>> >>
>> >> Reported-by: Markus Armbruster <address@hidden>
>> >> Suggested-by: Paolo Bonzini <address@hidden>
>> >> Signed-off-by: Peter Xu <address@hidden>
>> >> ---
>> >> hw/audio/intel-hda.c | 18 +-----------------
>> >> hw/i386/amd_iommu.c | 2 +-
>> >> hw/ide/ich.c | 6 +-----
>> >> hw/misc/edu.c | 4 +---
>> >> hw/net/e1000e.c | 6 +-----
>> >> hw/net/trace-events | 1 -
>> >> hw/net/vmxnet3.c | 8 ++------
>> >> hw/pci-bridge/ioh3420.c | 17 ++++-------------
>> >> hw/pci-bridge/pci_bridge_dev.c | 19 +------------------
>> >> hw/pci-bridge/xio3130_downstream.c | 11 +++--------
>> >> hw/pci-bridge/xio3130_upstream.c | 11 +++--------
>> >> hw/pci/msi.c | 25 ++++++-------------------
>> >> hw/scsi/megasas.c | 18 +-----------------
>> >> hw/scsi/mptsas.c | 20 ++------------------
>> >> hw/scsi/trace-events | 1 -
>> >> hw/scsi/vmw_pvscsi.c | 12 +++---------
>> >> hw/usb/hcd-xhci.c | 16 +---------------
>> >> hw/vfio/pci.c | 13 ++-----------
>> >> include/hw/pci/msi.h | 6 +++---
>> >> 19 files changed, 36 insertions(+), 178 deletions(-)
>> >
>> > Ping?
>> >
>> > Just to mention in case missed - this is also a bug fix for edu
>> > device.
>> >
>> > Also CC Markus since he's the reporter and I forgot to CC him in
>> > previous post. Sorry.
>>
>> The patch indeed fixes the leak in the edu device. It might fix similar
>> cleanup errors in other devices; I didn't check.
>>
>> The interesting part is of course having devices assert the interrrupt
>> controller isn't broken. The commit message claims "MSI should be
>> supported by all interrupt controllers". Does that mean "you think it
>> is supported", or does it mean "it really should be supported"?
>>
>> If the former, shouldn't we drop @msi_nonbroken entirely?
>>
>> If the latter, why is it okay to assert?
>>
>> The Suggested-by makes me suspect this has been explained elsewhere
>> already; feel free to send me just a pointer.
>
> I cannot provide a pointer... It was a discussion on IRC.
>
> For me, I cannot guarantee the latter is the truth. So I guess I am
> the former case.
>
> IIUC Paolo has the same suggestion there (remove msi_nonbroken
> entirely), but the reason is slightly different - device MSI init
> should not really depend on the irq controller at all. Or say, even
> the irq controller does not have MSI supported, the device should also
> be able to declare that capability, while the guest should just skip
> that capability since the board/controller does not support MSI at
> all.
>
> However here I still added an assertion() and didn't really remove
> msi_nonbroken intentionally, since I didn't know whether that'll be
> safe enough. This patch kept that variable, and the assertion makes
> sure that the behavior would be merely the same (from an
> error_report() into an assertion though).
>
> If we really want to totally remove that variable, I can repost a v2,
> as long as no one on the list would disagree.
>
> Paolo, please feel free to comment as well if I got anything wrong.
>
> Thanks,
The name msi_nonbroken and its comment (commit 226419d) came out of a
discussion I had with Michael Tsirkin:
https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg00712.html
The extra-condensed summary from memory (Michael, please correct
misrepresentations): the flag exists for the benefit of boards that
nominally support MSI, but the MSI emulation is actually broken. As a
work-around, we remove MSI capability from *devices*, so the broken MSI
emulation can't be reached.
Note that a board that doesn't support MSI can take MSI-capable devices
just fine. Only the broken boards can't.
Obviously, broken boards should be fixed. Once they all are, we can
(and should!) remove msi_nonbroken.
Ideally, the broken boards would mark themselves by clearing
msi_nonbroken, with a comment explaining why.
Sadly, that's not what they do. Instead, the *non-broken* boards mark
themselves by setting msi_nonbroken. Which ones of the boards that
don't are actually broken is anyone's guess. So is what exactly needs
fixing.
I guess the first step towards removing msi_nonbroken would be
addressing that particular sadness. Patches welcome :)
- [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/15
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/29
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Markus Armbruster, 2017/05/29
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/29
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(),
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Paolo Bonzini, 2017/05/30
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Markus Armbruster, 2017/05/30
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Paolo Bonzini, 2017/05/30
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/31
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Paolo Bonzini, 2017/05/31
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/31
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Paolo Bonzini, 2017/05/31
- Re: [Qemu-devel] [PATCH] msi: remove return code for msi_init(), Peter Xu, 2017/05/31