From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49405) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abRWJ-00016w-TS for qemu-devel@nongnu.org; Thu, 03 Mar 2016 06:34:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abRWI-0003fx-Ty for qemu-devel@nongnu.org; Thu, 03 Mar 2016 06:34:15 -0500 Date: Thu, 3 Mar 2016 13:33:58 +0200 From: "Michael S. Tsirkin" Message-ID: <20160303133126-mutt-send-email-mst@redhat.com> References: <1450176195-9066-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1450176195-9066-2-git-send-email-caoj.fnst@cn.fujitsu.com> <87wpplqm3i.fsf@blackfin.pond.sub.org> <56D80E0B.8070902@redhat.com> <20160303123221-mutt-send-email-mst@redhat.com> <56D81DAD.8040101@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56D81DAD.8040101@redhat.com> Subject: Re: [Qemu-devel] [PATCH RFC v2 1/2] Add param Error** to msi_init() & modify the callers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: kwolf@redhat.com, alex.williamson@redhat.com, jiri@resnulli.us, qemu-block@nongnu.org, jasowang@redhat.com, Markus Armbruster , qemu-devel@nongnu.org, keith.busch@intel.com, Cao jin , sfeldma@gmail.com, kraxel@redhat.com, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, Jan Kiszka , hare@suse.de On Thu, Mar 03, 2016 at 01:19:09PM +0200, Marcel Apfelbaum wrote: > On 03/03/2016 12:45 PM, Michael S. Tsirkin wrote: > >On Thu, Mar 03, 2016 at 12:12:27PM +0200, Marcel Apfelbaum wrote: > >>>>+int msi_init(struct PCIDevice *dev, uint8_t offset, unsigned int nr_vectors, > >>>>+ bool msi64bit, bool msi_per_vector_mask, Error **errp) > >>>> { > >>>> unsigned int vectors_order; > >>>>- uint16_t flags; > >>>>+ uint16_t flags; /* Message Control register value */ > >>>> uint8_t cap_size; > >>>> int config_offset; > >>>> > >>>> if (!msi_supported) { > >>>>+ error_setg(errp, "MSI is not supported by interrupt controller"); > >>>> return -ENOTSUP; > >>> > >>>First failure mode: board does not support MSI (-ENOTSUP). > >>> > >>>Question to the PCI guys: why is this even an error? A device with > >>>capability MSI should work just fine in such a board. > >> > >>Hi Markus, > >> > >>Adding Jan Kiszka, maybe he can help. > >> > >>That's a fair question. Is there any history for this decision? > >>The board not supporting MSI has nothing to do with the capability being there. > >>The HW should not change because the board doe not support it. > >> > >>The capability should be present but not active. > > > >Digging in git log will tell you why we have the msi_supported flag: > > > >commit 02eb84d0ec97f183ac23ee939403a139e8849b1d ("qemu/pci: MSI-X support functions") > > > > This is a safety measure to avoid breaking platforms which should support > > MSI-X but currently lack this in the interrupt controller emulation. > > > >in other words, on some platforms we must hide msi support from devices > >because otherwise guests will try to use it, and our emulation is > >incomplete. > > > OK, thanks. So the flag should be "msi_broken" or "msi_present_but_not_implemented" and not > "msi_supported" that leads (at least me) to the assumption that some platform *does not support msi* > rather than it supports it, but we don't emulate it. > > > > > >And the conclusion from that is that for msi_init to fail silently is > >at the moment the right thing to do. > > But this is not the only thing we do, we are modifying the PCI devices. We could fail starting the VM > if a device supporting MSI is added on a platform with broken msi, but this will prevent us to use > assigned devices. Emulated devices should be created with a specific "msi=off" flag. > > Thanks, > Marcel That will just break a bunch of valid configurations, for no real benefit to users. > > > >The only other reason for it to fail is pci config space corruption, > >this probably never happens in practice. > >