Re: [Qemu-devel] [PATCH 02/34] pci: handle BAR mapping at PCI level

[Isaku Yamahata]

On Fri, Jul 23, 2010 at 08:15:44PM +0000, Blue Swirl wrote:
> On Fri, Jul 23, 2010 at 10:40 AM, Isaku Yamahata <address@hidden> wrote:
> > On Thu, Jul 22, 2010 at 09:54:46PM +0000, Blue Swirl wrote:
> >> diff --git a/hw/pci.c b/hw/pci.c
> >> index a98d6f3..49f03fb 100644
> >> --- a/hw/pci.c
> >> +++ b/hw/pci.c
> > ...
> >> @@ -817,6 +825,25 @@ void pci_register_bar(PCIDevice *pci_dev, int 
> >> region_num,
> >>               pci_set_long(pci_dev->wmask + addr, wmask & 0xffffffff);
> >>               pci_set_long(pci_dev->cmask + addr, 0xffffffff);
> >>         }
> >> +      pci_bar_map(pci_dev, region_num, 0, 0, size, -1);
> >> +}
> >> +
> >> +void pci_bar_map(PCIDevice *pci_dev, int region_num, int subregion_num,
> >> +                         pcibus_t offset, pcibus_t size, int ix)
> >> +{
> >> +      PCIIOSubRegion *s;
> >> +
> >> +      if ((unsigned int)region_num >= PCI_NUM_REGIONS ||
> >> +            (unsigned int)subregion_num >= PCI_NUM_SUBREGIONS) {
> >> +            return;
> >> +      }
> >
> > abort() or assert()? It's caller's bug.
> 
> Yes. I copied this from pci_register_bar(), which should also have
> assert() or abort().

Here is the patch.
It would make sense to include this patch into the patch series.

>From 24ffedc781227e5f05ceb94f2690b207591b8929 Mon Sep 17 00:00:00 2001
Message-Id: <address@hidden>
In-Reply-To: <address@hidden>
References: <address@hidden>
From: Isaku Yamahata <address@hidden>
Date: Wed, 28 Jul 2010 11:15:45 +0900
Subject: [PATCH] pci: Improve pci_register_bar().

Improve pci_register_bar().
- Don't return silently when invalid region_num
  is passed to pci_register_bar().
  It's caller's bug, so use assert().
- make region_num argument unsigned int.
  There is no point that it's signed int.
- make type argument uint8_t to match PCIIORegion::type
PCIMapIORegionFunc signature should be changed, but didn't change it
because it will be eliminated later.

Signed-off-by: Isaku Yamahata <address@hidden>
---
 hw/pci.c |    9 ++++-----
 hw/pci.h |    6 +++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index a98d6f3..8d581c4 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -778,16 +778,15 @@ static int pci_unregister_device(DeviceState *dev)
     return 0;
 }
 
-void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            pcibus_t size, int type,
-                            PCIMapIORegionFunc *map_func)
+void pci_register_bar(PCIDevice *pci_dev, unsigned int region_num,
+                      pcibus_t size, uint8_t type,
+                      PCIMapIORegionFunc *map_func)
 {
     PCIIORegion *r;
     uint32_t addr;
     pcibus_t wmask;
 
-    if ((unsigned int)region_num >= PCI_NUM_REGIONS)
-        return;
+    assert(region_num < PCI_NUM_REGIONS);
 
     if (size & (size-1)) {
         fprintf(stderr, "ERROR: PCI region size must be pow2 "
diff --git a/hw/pci.h b/hw/pci.h
index 1eab7e7..df8fedd 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -179,9 +179,9 @@ PCIDevice *pci_register_device(PCIBus *bus, const char 
*name,
                                PCIConfigReadFunc *config_read,
                                PCIConfigWriteFunc *config_write);
 
-void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            pcibus_t size, int type,
-                            PCIMapIORegionFunc *map_func);
+void pci_register_bar(PCIDevice *pci_dev, unsigned int region_num,
+                      pcibus_t size, uint8_t type,
+                      PCIMapIORegionFunc *map_func);
 
 int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
 int pci_add_capability_at_offset(PCIDevice *pci_dev, uint8_t cap_id,
-- 
1.7.1.1