diff mbox

[v5,07/11] pci: add support to size ROM BARs to pci_size_mem_bar

Message ID 20170814142850.39133-8-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné Aug. 14, 2017, 2:28 p.m. UTC 
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes since v4:
 - New in this version.
---
 xen/drivers/passthrough/pci.c | 24 +++++++++++++-----------
 xen/include/xen/pci.h         |  2 +-
 2 files changed, 14 insertions(+), 12 deletions(-)

Comments

Jan Beulich Sept. 5, 2017, 3:12 p.m. UTC | #1 
>>> On 14.08.17 at 16:28, <roger.pau@citrix.com> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -594,15 +594,18 @@ static int iommu_remove_device(struct pci_dev *pdev);
>  
>  int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
>                       unsigned int func, unsigned int pos, bool last,
> -                     uint64_t *paddr, uint64_t *psize, bool vf)
> +                     uint64_t *paddr, uint64_t *psize, bool vf, bool rom)
>  {
>      uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos);
>      uint64_t addr, size;
> +    bool is64bits = !rom && (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +                    PCI_BASE_ADDRESS_MEM_TYPE_64;
>  
> -    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
> +    ASSERT(!(rom && vf));

Things like this as well as there now being three bools among the
function parameters is imo a good indication that you want an
"unsigned int flags" parameter instead. That'll also help seeing
what the true-s and false-s are representing at the call sites. And
perhaps that would then better already be done in the patch
adding "vf".

> @@ -616,9 +619,8 @@ int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
>          pci_conf_write32(seg, bus, slot, func, pos + 4, ~0);
>      }
>      size = pci_conf_read32(seg, bus, slot, func, pos) &
> -           PCI_BASE_ADDRESS_MEM_MASK;
> -    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> -         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +           (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK);

To aid readability and because it repeats ...

> @@ -627,14 +629,14 @@ int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
>          size |= (uint64_t)~0 << 32;
>      pci_conf_write32(seg, bus, slot, func, pos, bar);
>      size = -size;
> -    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
> +    addr = (bar & (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK)) |

... here, perhaps worth using a local variable just like you did e.g.
with is64bits?

> +    if ( is64bits )
>          return 2;
>  
>      return 1;

Mind folding these into a single return statement now that the
result is going to be reasonably short?

Jan
diff mbox

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 948c227e01..33cb774d29 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -594,15 +594,18 @@  static int iommu_remove_device(struct pci_dev *pdev);
 
 int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
                      unsigned int func, unsigned int pos, bool last,
-                     uint64_t *paddr, uint64_t *psize, bool vf)
+                     uint64_t *paddr, uint64_t *psize, bool vf, bool rom)
 {
     uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos);
     uint64_t addr, size;
+    bool is64bits = !rom && (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+                    PCI_BASE_ADDRESS_MEM_TYPE_64;
 
-    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
+    ASSERT(!(rom && vf));
+    ASSERT(rom ||
+           (bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
     pci_conf_write32(seg, bus, slot, func, pos, ~0);
-    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-         PCI_BASE_ADDRESS_MEM_TYPE_64 )
+    if ( is64bits )
     {
         if ( last )
         {
@@ -616,9 +619,8 @@  int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
         pci_conf_write32(seg, bus, slot, func, pos + 4, ~0);
     }
     size = pci_conf_read32(seg, bus, slot, func, pos) &
-           PCI_BASE_ADDRESS_MEM_MASK;
-    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-         PCI_BASE_ADDRESS_MEM_TYPE_64 )
+           (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK);
+    if ( is64bits )
     {
         size |= (uint64_t)pci_conf_read32(seg, bus, slot, func, pos + 4) << 32;
         pci_conf_write32(seg, bus, slot, func, pos + 4, hi);
@@ -627,14 +629,14 @@  int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
         size |= (uint64_t)~0 << 32;
     pci_conf_write32(seg, bus, slot, func, pos, bar);
     size = -size;
-    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
+    addr = (bar & (rom ? PCI_ROM_ADDRESS_MASK : PCI_BASE_ADDRESS_MEM_MASK)) |
+           ((uint64_t)hi << 32);
 
     if ( paddr )
         *paddr = addr;
     *psize = size;
 
-    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-         PCI_BASE_ADDRESS_MEM_TYPE_64 )
+    if ( is64bits )
         return 2;
 
     return 1;
@@ -716,7 +718,7 @@  int pci_add_device(u16 seg, u8 bus, u8 devfn,
                 }
                 ret = pci_size_mem_bar(seg, bus, slot, func, idx,
                                        i == PCI_SRIOV_NUM_BARS - 1, NULL,
-                                       &pdev->vf_rlen[i], true);
+                                       &pdev->vf_rlen[i], true, false);
                 if ( ret < 0 )
                     break;
 
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index b85e4fa8ad..72c901be66 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -166,7 +166,7 @@  const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
                           unsigned int *dev, unsigned int *func, bool *def_seg);
 int pci_size_mem_bar(unsigned int seg, unsigned int bus, unsigned int slot,
                      unsigned int func, unsigned int pos, bool last,
-                     uint64_t *addr, uint64_t *size, bool vf);
+                     uint64_t *addr, uint64_t *size, bool vf, bool rom);
 
 
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);