[Qemu-devel] [PATCH 0/3] pci: pci_register_bar() clean up

[Qemu-devel] [PATCH 0/3] pci: pci_register_bar() clean up

[Isaku Yamahata]

This patch set cleans up pci_register_bar() a bit.

Isaku Yamahata (3):
  pci: sorting out type confusion in pci_register_bar().
  pci: don't ignore invalid parameter for pci_register_bar().
  pci: improve signature of pci_register_bar().

 hw/pci.c |   11 +++++------
 hw/pci.h |    2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

[Qemu-devel] [PATCH 1/3] pci: sorting out type confusion in pci_register_bar().

[Isaku Yamahata]

This patch sorts out invalid use of pcibus_t.

In pci_register_bar(), pcibus_t wmask is used.
It should, However, be uint64_t because it is used to set
pci configuration space value(PCIDevice::wmask) by pci_set_quad()
or pci_set_long().
So make its type uint64_t and cast from pcibus_t to int64_t where
necessary.

Maybe for strict type safety conversion functions would
be desirable. Something like pcibus_to_uintN(), uintN_to_pcibus().
But conversions are done only a few place, so it wouldn't be
worthwhile.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index f03b83e..8d6b299 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -763,7 +763,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
 {
     PCIIORegion *r;
     uint32_t addr;
-    pcibus_t wmask;
+    uint64_t wmask;
 
     if ((unsigned int)region_num >= PCI_NUM_REGIONS)
         return;
@@ -781,7 +781,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     r->type = type;
     r->map_func = map_func;
 
-    wmask = ~(size - 1);
+    wmask = (uint64_t)(~(size - 1));
     addr = pci_bar(pci_dev, region_num);
     if (region_num == PCI_ROM_SLOT) {
         /* ROM enable bit is writeable */
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 1/3] pci: sorting out type confusion in pci_register_bar().

[Michael S. Tsirkin]

On Thu, Sep 09, 2010 at 11:48:55AM +0900, Isaku Yamahata wrote:
> This patch sorts out invalid use of pcibus_t.
> 
> In pci_register_bar(), pcibus_t wmask is used.
> It should, However, be uint64_t because it is used to set
> pci configuration space value(PCIDevice::wmask) by pci_set_quad()
> or pci_set_long().
> So make its type uint64_t and cast from pcibus_t to int64_t where
> necessary.


I don't think we need the cast. Applied without the cast.

> Maybe for strict type safety conversion functions would
> be desirable. Something like pcibus_to_uintN(), uintN_to_pcibus().
> But conversions are done only a few place, so it wouldn't be
> worthwhile.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index f03b83e..8d6b299 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -763,7 +763,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>  {
>      PCIIORegion *r;
>      uint32_t addr;
> -    pcibus_t wmask;
> +    uint64_t wmask;
>  
>      if ((unsigned int)region_num >= PCI_NUM_REGIONS)
>          return;
> @@ -781,7 +781,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      r->type = type;
>      r->map_func = map_func;
>  
> -    wmask = ~(size - 1);
> +    wmask = (uint64_t)(~(size - 1));
>      addr = pci_bar(pci_dev, region_num);
>      if (region_num == PCI_ROM_SLOT) {
>          /* ROM enable bit is writeable */
> -- 
> 1.7.1.1

[Qemu-devel] [PATCH 2/3] pci: don't ignore invalid parameter for pci_register_bar().

[Isaku Yamahata]

Abort when invalid value for region_num is passed to pci_register_bar.
That is caller's bug. Abort instead of silently ignoring invalid value.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 8d6b299..31eba9a 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -765,9 +765,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     uint32_t addr;
     uint64_t wmask;
 
-    if ((unsigned int)region_num >= PCI_NUM_REGIONS)
-        return;
-
+    assert(region_num >= 0);
+    assert(region_num < PCI_NUM_REGIONS);
     if (size & (size-1)) {
         fprintf(stderr, "ERROR: PCI region size must be pow2 "
                     "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 2/3] pci: don't ignore invalid parameter for pci_register_bar().

[Michael S. Tsirkin]

On Thu, Sep 09, 2010 at 11:48:56AM +0900, Isaku Yamahata wrote:
> Abort when invalid value for region_num is passed to pci_register_bar.
> That is caller's bug. Abort instead of silently ignoring invalid value.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)

Applied.

> diff --git a/hw/pci.c b/hw/pci.c
> index 8d6b299..31eba9a 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -765,9 +765,8 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>      uint32_t addr;
>      uint64_t wmask;
>  
> -    if ((unsigned int)region_num >= PCI_NUM_REGIONS)
> -        return;
> -
> +    assert(region_num >= 0);
> +    assert(region_num < PCI_NUM_REGIONS);
>      if (size & (size-1)) {
>          fprintf(stderr, "ERROR: PCI region size must be pow2 "
>                      "type=0x%x, size=0x%"FMT_PCIBUS"\n", type, size);
> -- 
> 1.7.1.1

[Qemu-devel] [PATCH 3/3] pci: improve signature of pci_register_bar().

[Isaku Yamahata]

Make type uint8_t from int because PCIIORegion::type is uint8_t.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |    2 +-
 hw/pci.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 31eba9a..ceee291 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -758,7 +758,7 @@ static int pci_unregister_device(DeviceState *dev)
 }
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            pcibus_t size, int type,
+                            pcibus_t size, uint8_t type,
                             PCIMapIORegionFunc *map_func)
 {
     PCIIORegion *r;
diff --git a/hw/pci.h b/hw/pci.h
index bccab3a..25179eb 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -192,7 +192,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                PCIConfigWriteFunc *config_write);
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
-                            pcibus_t size, int type,
+                            pcibus_t size, uint8_t type,
                             PCIMapIORegionFunc *map_func);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
-- 
1.7.1.1

[Qemu-devel] Re: [PATCH 3/3] pci: improve signature of pci_register_bar().

[Michael S. Tsirkin]

On Thu, Sep 09, 2010 at 11:48:57AM +0900, Isaku Yamahata wrote:
> Make type uint8_t from int because PCIIORegion::type is uint8_t.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |    2 +-
>  hw/pci.h |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)


Applied, thanks.

> diff --git a/hw/pci.c b/hw/pci.c
> index 31eba9a..ceee291 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -758,7 +758,7 @@ static int pci_unregister_device(DeviceState *dev)
>  }
>  
>  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> -                            pcibus_t size, int type,
> +                            pcibus_t size, uint8_t type,
>                              PCIMapIORegionFunc *map_func)
>  {
>      PCIIORegion *r;
> diff --git a/hw/pci.h b/hw/pci.h
> index bccab3a..25179eb 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -192,7 +192,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
>                                 PCIConfigWriteFunc *config_write);
>  
>  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> -                            pcibus_t size, int type,
> +                            pcibus_t size, uint8_t type,
>                              PCIMapIORegionFunc *map_func);
>  
>  int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> -- 
> 1.7.1.1