From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51123) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RX8yP-0003VF-SC for qemu-devel@nongnu.org; Sun, 04 Dec 2011 05:07:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RX8yO-0006Q3-Pk for qemu-devel@nongnu.org; Sun, 04 Dec 2011 05:07:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33419) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RX8yO-0006Pp-JD for qemu-devel@nongnu.org; Sun, 04 Dec 2011 05:07:04 -0500 Date: Sun, 4 Dec 2011 12:08:42 +0200 From: "Michael S. Tsirkin" Message-ID: <20111204100841.GA15464@redhat.com> References: <27ef57671106d9214757df8a1d4cc28287dae07c.1321893802.git.mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for v1.0 1/3] msix: track function masked in pci device state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cam Macdonell Cc: Blue Swirl , Jan Kiszka , Anthony Liguori , qemu-devel@nongnu.org, Alexander Graf On Fri, Dec 02, 2011 at 04:34:21PM -0700, Cam Macdonell wrote: > Based on a git bisect, this patch breaks msi-x interrupt delivery in > the ivshmem device. >=20 > On Mon, Nov 21, 2011 at 9:57 AM, Michael S. Tsirkin wr= ote: > > Only go over the table when function is masked. > > This is not really important for qemu.git but helps > > fix a bug in qemu-kvm.git. > > > > Signed-off-by: Michael S. Tsirkin > > --- > > =A0hw/msix.c | =A0 21 ++++++++++++++------- > > =A0hw/pci.h =A0| =A0 =A02 ++ > > =A02 files changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/hw/msix.c b/hw/msix.c > > index b15bafc..63b41b9 100644 > > --- a/hw/msix.c > > +++ b/hw/msix.c > > @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, = unsigned short nentries, > > =A0 =A0 /* Make flags bit writable. */ > > =A0 =A0 pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |=3D MSIX_EN= ABLE_MASK | > > =A0 =A0 =A0 =A0 =A0 =A0MSIX_MASKALL_MASK; > > + =A0 =A0pdev->msix_function_masked =3D true; > > =A0 =A0 return 0; >=20 > iiuc, this masks the msix by default. Yes, because msi-x is disabled by default, that's in the pci spec. > > =A0} > > > > @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, in= t vector) > > =A0 =A0 *msix_pending_byte(dev, vector) &=3D ~msix_pending_mask(vecto= r); > > =A0} > > > > -static int msix_function_masked(PCIDevice *dev) > > -{ > > - =A0 =A0return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MS= IX_MASKALL_MASK; > > -} > > - > > =A0static int msix_is_masked(PCIDevice *dev, int vector) > > =A0{ > > =A0 =A0 unsigned offset =3D > > =A0 =A0 =A0 =A0 vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_= CTRL; > > - =A0 =A0return msix_function_masked(dev) || > > + =A0 =A0return dev->msix_function_masked || > > =A0 =A0 =A0 =A0 =A0 dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTR= L_MASKBIT; > > =A0} > > > > @@ -138,24 +134,34 @@ static void msix_handle_mask_update(PCIDevice *= dev, int vector) > > =A0 =A0 } > > =A0} > > > > +static void msix_update_function_masked(PCIDevice *dev) > > +{ > > + =A0 =A0dev->msix_function_masked =3D !msix_enabled(dev) || > > + =A0 =A0 =A0 =A0(dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & = MSIX_MASKALL_MASK); > > +} > > + > > =A0/* Handle MSI-X capability config write. */ > > =A0void msix_write_config(PCIDevice *dev, uint32_t addr, > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0uint32_t val, int len) > > =A0{ > > =A0 =A0 unsigned enable_pos =3D dev->msix_cap + MSIX_CONTROL_OFFSET; > > =A0 =A0 int vector; > > + =A0 =A0bool was_masked; > > > > =A0 =A0 if (!range_covers_byte(addr, len, enable_pos)) { > > =A0 =A0 =A0 =A0 return; > > =A0 =A0 } > > > > + =A0 =A0was_masked =3D dev->msix_function_masked; > > + =A0 =A0msix_update_function_masked(dev); > > + > > =A0 =A0 if (!msix_enabled(dev)) { > > =A0 =A0 =A0 =A0 return; > > =A0 =A0 } > > > > =A0 =A0 pci_device_deassert_intx(dev); > > > > - =A0 =A0if (msix_function_masked(dev)) { > > + =A0 =A0if (dev->msix_function_masked =3D=3D was_masked) { > > =A0 =A0 =A0 =A0 return; > > =A0 =A0 } >=20 > So I believe my bug is due to the fact the new logic included in this > patch requires msix_write_config() to be called to unmask the vectors. Not exactly, to enable msi-x really. > Virtio-pci calls msix_write_config(), but ivshmem does not (nor does > PCIe so I'm not sure if it's also affected). At this point PCIe is a stub. > I haven't been able to fix the bug yet, but I wanted to make sure I > was looking in the correct place. Any help of further explanation of > this patch would be greatly appreciated. >=20 > Sincerely, > Cam So I think you just need to call msix_write_config, otherwise msix is not getting enabled. BTW looking at the ivshmem code, this bit looks wrong: pci_conf[PCI_COMMAND] =3D PCI_COMMAND_IO | PCI_COMMAND_MEMORY; I think the spec says IO/MEMORY must be disabled at init time since BARs are not yet set to anything reasonable. > > > > @@ -300,6 +306,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f) > > =A0 =A0 msix_free_irq_entries(dev); > > =A0 =A0 qemu_get_buffer(f, dev->msix_table_page, n * PCI_MSIX_ENTRY_S= IZE); > > =A0 =A0 qemu_get_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, = (n + 7) / 8); > > + =A0 =A0msix_update_function_masked(dev); > > =A0} > > > > =A0/* Does device support MSI-X? */ > > diff --git a/hw/pci.h b/hw/pci.h > > index 4b2e785..625e717 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -178,6 +178,8 @@ struct PCIDevice { > > =A0 =A0 unsigned *msix_entry_used; > > =A0 =A0 /* Region including the MSI-X table */ > > =A0 =A0 uint32_t msix_bar_size; > > + =A0 =A0/* MSIX function mask set or MSIX disabled */ > > + =A0 =A0bool msix_function_masked; > > =A0 =A0 /* Version id needed for VMState */ > > =A0 =A0 int32_t version_id; > > > > -- > > 1.7.5.53.gc233e > > > >