Peter Xu <pet...@redhat.com> writes: > On Mon, May 29, 2017 at 11:42:35AM +0200, Markus Armbruster wrote: >> Peter Xu <pet...@redhat.com> 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 <arm...@redhat.com> >> >> Suggested-by: Paolo Bonzini <pbonz...@redhat.com> >> >> Signed-off-by: Peter Xu <pet...@redhat.com> >> >> --- >> >> 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 :)
