From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=50559 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PAHbb-0003GU-KU
	for qemu-devel@nongnu.org; Mon, 25 Oct 2010 03:36:32 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1PAHba-0006XD-6f
	for qemu-devel@nongnu.org; Mon, 25 Oct 2010 03:36:31 -0400
Received: from mx1.redhat.com ([209.132.183.28]:6292)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <mst@redhat.com>) id 1PAHba-0006X9-0S
	for qemu-devel@nongnu.org; Mon, 25 Oct 2010 03:36:30 -0400
Date: Mon, 25 Oct 2010 09:29:50 +0200
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20101025072950.GB4679@redhat.com>
References: <20101025060725.GA3533@redhat.com>
	<20101025070713.GX31309@valinux.co.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20101025070713.GX31309@valinux.co.jp>
Subject: [Qemu-devel] Re: [PATCH RFC] msi: remove range checks
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: qemu-devel@nongnu.org

Hmm, does this simply check that write is within the capability?
Then I think I misunderstood what this does.
Please replace hard-coded length logic with existing function call
that gets the lengths, the range check can stay then.


On Mon, Oct 25, 2010 at 04:07:13PM +0900, Isaku Yamahata wrote:
> Seems good.
> For consistency, the range check in msix_write_config() should
> be also removed.
> 
> On Mon, Oct 25, 2010 at 08:07:25AM +0200, Michael S. Tsirkin wrote:
> > config write handlers should be idempotent.
> > So no need for range checks.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/msi.c |   29 -----------------------------
> >  1 files changed, 0 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/msi.c b/hw/msi.c
> > index a949d82..af8ea44 100644
> > --- a/hw/msi.c
> > +++ b/hw/msi.c
> > @@ -257,35 +257,6 @@ void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len)
> >      uint32_t pending;
> >      int i;
> >  
> > -#ifdef MSI_DEBUG
> > -    if (ranges_overlap(addr, len, dev->msi_cap, msi_cap_sizeof(flags))) {
> > -        MSI_DEV_PRINTF(dev, "addr 0x%"PRIx32" val 0x%"PRIx32" len %d\n",
> > -                       addr, val, len);
> > -        MSI_DEV_PRINTF(dev, "ctrl: 0x%"PRIx16" address: 0x%"PRIx32,
> > -                       flags,
> > -                       pci_get_long(dev->config + msi_address_lo_off(dev)));
> > -        if (msi64bit) {
> > -            fprintf(stderr, " addrss-hi: 0x%"PRIx32,
> > -                    pci_get_long(dev->config + msi_address_hi_off(dev)));
> > -        }
> > -        fprintf(stderr, " data: 0x%"PRIx16,
> > -                pci_get_word(dev->config + msi_data_off(dev, msi64bit)));
> > -        if (flags & PCI_MSI_FLAGS_MASKBIT) {
> > -            fprintf(stderr, " mask 0x%"PRIx32" pending 0x%"PRIx32,
> > -                    pci_get_long(dev->config + msi_mask_off(dev, msi64bit)),
> > -                    pci_get_long(dev->config + msi_pending_off(dev, msi64bit)));
> > -        }
> > -        fprintf(stderr, "\n");
> > -    }
> > -#endif
> > -
> > -    /* Are we modified? */
> > -    if (!(ranges_overlap(addr, len, msi_flags_off(dev), 2) ||
> > -          (msi_per_vector_mask &&
> > -           ranges_overlap(addr, len, msi_mask_off(dev, msi64bit), 4)))) {
> > -        return;
> > -    }
> > -
> >      if (!(flags & PCI_MSI_FLAGS_ENABLE)) {
> >          return;
> >      }
> > -- 
> > 1.7.3-rc1
> > 
> 
> -- 
> yamahata