From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 2/5] qemu-kvm: msix: Only invoke msix_handle_mask_update on changes Date: Tue, 18 Oct 2011 13:54:23 +0200 Message-ID: <4E9D68EF.7070703@siemens.com> References: <20111018114331.GE28776@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" To: "Michael S. Tsirkin" Return-path: Received: from thoth.sbs.de ([192.35.17.2]:30529 "EHLO thoth.sbs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752001Ab1JRLy1 (ORCPT ); Tue, 18 Oct 2011 07:54:27 -0400 In-Reply-To: <20111018114331.GE28776@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2011-10-18 13:43, Michael S. Tsirkin wrote: > On Tue, Oct 18, 2011 at 09:50:51AM +0200, Jan Kiszka wrote: >> Reorganize msix_mmio_writel so that msix_handle_mask_update is only >> called on mask changes. Pass previous config space value to >> msix_write_config so that it can check if a mask change took place. >> >> Signed-off-by: Jan Kiszka > > What we did in other cases is track the old value in device state. > This makes the API easier to use correctly. > I'm testing the following as a replacement - any comments? No concerns about caching the mask state, but... > > diff --git a/hw/msix.c b/hw/msix.c > index b15bafc..655a600 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -79,6 +79,7 @@ static int msix_add_config(struct PCIDevice *pdev, unsigned short nentries, > /* Make flags bit writable. */ > pdev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > MSIX_MASKALL_MASK; > + pdev->msix_function_masked = false; > return 0; > } > > @@ -117,16 +118,11 @@ static void msix_clr_pending(PCIDevice *dev, int vector) > *msix_pending_byte(dev, vector) &= ~msix_pending_mask(vector); > } > > -static int msix_function_masked(PCIDevice *dev) > -{ > - return dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; > -} > - > static int msix_is_masked(PCIDevice *dev, int vector) > { > unsigned offset = > vector * PCI_MSIX_ENTRY_SIZE + PCI_MSIX_ENTRY_VECTOR_CTRL; > - return msix_function_masked(dev) || > + return dev->msix_function_masked || > dev->msix_table_page[offset] & PCI_MSIX_ENTRY_CTRL_MASKBIT; > } > > @@ -144,6 +140,7 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, > { > unsigned enable_pos = dev->msix_cap + MSIX_CONTROL_OFFSET; > int vector; > + bool fmsk; > > if (!range_covers_byte(addr, len, enable_pos)) { > return; > @@ -155,10 +152,12 @@ void msix_write_config(PCIDevice *dev, uint32_t addr, > > pci_device_deassert_intx(dev); > > - if (msix_function_masked(dev)) { > + fmsk = dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] & MSIX_MASKALL_MASK; > + if (dev->msix_function_masked == fmsk) { ...this misses MSIX_MASKALL_MASK (but !MSIX_ENABLE_MASK) -> MSIX_ENABLE_MASK. OTOH, there will only be one user of msix_write_config remaining: pci_default_write_config. So there are not that many users to check for correct API use. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux