diff mbox

[v6,05/11] pci: split code to size BARs from pci_add_device

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

Commit Message

Roger Pau Monné Sept. 19, 2017, 3:29 p.m. UTC 
So that it can be called from outside in order to get the size of regular PCI
BARs. This will be required in order to map the BARs from PCI devices into PVH
Dom0 p2m.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes since v5:
 - Introduce a flags field for pci_size_mem_bar.
 - Use pci_sbdf_t.

Changes since v4:
 - Restore printing whether the BAR is from a vf.
 - Make the psize pointer parameter not optional.
 - s/u64/uint64_t.
 - Remove some unneeded parentheses.
 - Assert the return value is never 0.
 - Use the newly introduced pci_sbdf_t type.

Changes since v3:
 - Rename function to size BARs to pci_size_mem_bar.
 - Change the parameters passed to the function. Pass the position and
   whether the BAR is the last one, instead of the (base, max_bars,
   *index) tuple.
 - Make the function return the number of BARs consumed (1 for 32b, 2
   for 64b BARs).
 - Change the dprintk back to printk.
 - Do not log another error message in pci_add_device in case
   pci_size_mem_bar fails.
---
 xen/drivers/passthrough/pci.c | 98 ++++++++++++++++++++++++++++---------------
 xen/include/xen/pci.h         |  4 ++
 2 files changed, 68 insertions(+), 34 deletions(-)

Comments

Jan Beulich Oct. 4, 2017, 8:32 a.m. UTC | #1 
>>> On 19.09.17 at 17:29, <roger.pau@citrix.com> wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -603,6 +603,56 @@ static int iommu_add_device(struct pci_dev *pdev);
>  static int iommu_enable_device(struct pci_dev *pdev);
>  static int iommu_remove_device(struct pci_dev *pdev);
>  
> +int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, bool last,
> +                     uint64_t *paddr, uint64_t *psize, unsigned int flags)
> +{
> +    uint32_t hi = 0, bar = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> +                                           sbdf.func, pos);
> +    uint64_t addr, size;
> +    bool vf = flags & PCI_BAR_VF;

Honestly I'm not convinced of the utility of this variable; same for the
"rom" one in the next patch.

> +    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
> +    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, ~0);
> +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +    {
> +        if ( last )
> +        {
> +            printk(XENLOG_WARNING
> +                   "%sdevice %04x:%02x:%02x.%u with 64-bit %sBAR in last slot\n",
> +                   vf ? "SR-IOV " : "", sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
> +                   vf ? "vf " : "");
> +            return -EINVAL;
> +        }
> +        hi = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4);
> +        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, ~0);
> +    }
> +    size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos) &
> +           PCI_BASE_ADDRESS_MEM_MASK;
> +    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +         PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +    {
> +        size |= (uint64_t)pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
> +                                          sbdf.func, pos + 4) << 32;
> +        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, hi);
> +    }
> +    else if ( size )
> +        size |= (uint64_t)~0 << 32;
> +    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, bar);
> +    size = -size;
> +    addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
> +
> +    if ( paddr )
> +        *paddr = addr;

You need addr only inside the if() - no need for the local variable,
and no need to calculate it unconditionally.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
>  const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>                            unsigned int *dev, unsigned int *func, bool *def_seg);
>  
> +#define _PCI_BAR_VF     0
> +#define PCI_BAR_VF      (1u << _PCI_BAR_VF)

Do you really need both? I know we have quite a few cases where flags
are being defined this way, but that's usually when bit operations
(test_bit() and alike) are intended on the flags fields.

Jan
Roger Pau Monné Oct. 4, 2017, 1:31 p.m. UTC | #2 
On Wed, Oct 04, 2017 at 08:32:06AM +0000, Jan Beulich wrote:
> >>> On 19.09.17 at 17:29, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/xen/pci.h
> > +++ b/xen/include/xen/pci.h
> > @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
> >  const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
> >                            unsigned int *dev, unsigned int *func, bool *def_seg);
> >  
> > +#define _PCI_BAR_VF     0
> > +#define PCI_BAR_VF      (1u << _PCI_BAR_VF)
> 
> Do you really need both? I know we have quite a few cases where flags
> are being defined this way, but that's usually when bit operations
> (test_bit() and alike) are intended on the flags fields.

Ack, would you then rather prefer to have 1, or (1u << 0)? (to keep it
in line with the other flag that will be added later).

Thanks, Roger.
Jan Beulich Oct. 4, 2017, 4 p.m. UTC | #3 
>>> On 04.10.17 at 15:31, <roger.pau@citrix.com> wrote:
> On Wed, Oct 04, 2017 at 08:32:06AM +0000, Jan Beulich wrote:
>> >>> On 19.09.17 at 17:29, <roger.pau@citrix.com> wrote:
>> > --- a/xen/include/xen/pci.h
>> > +++ b/xen/include/xen/pci.h
>> > @@ -189,6 +189,10 @@ const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
>> >  const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
>> >                            unsigned int *dev, unsigned int *func, bool *def_seg);
>> >  
>> > +#define _PCI_BAR_VF     0
>> > +#define PCI_BAR_VF      (1u << _PCI_BAR_VF)
>> 
>> Do you really need both? I know we have quite a few cases where flags
>> are being defined this way, but that's usually when bit operations
>> (test_bit() and alike) are intended on the flags fields.
> 
> Ack, would you then rather prefer to have 1, or (1u << 0)? (to keep it
> in line with the other flag that will be added later).

1u please, as that's going to be mandatory once someone adds
a definition for the 32nd flag bit. The other option would be to
use a plain hex constant without involving the shift operator.

Jan
diff mbox

Patch

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 975485fe05..ba58b4d0cc 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -603,6 +603,56 @@  static int iommu_add_device(struct pci_dev *pdev);
 static int iommu_enable_device(struct pci_dev *pdev);
 static int iommu_remove_device(struct pci_dev *pdev);
 
+int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, bool last,
+                     uint64_t *paddr, uint64_t *psize, unsigned int flags)
+{
+    uint32_t hi = 0, bar = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
+                                           sbdf.func, pos);
+    uint64_t addr, size;
+    bool vf = flags & PCI_BAR_VF;
+
+    ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
+    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, ~0);
+    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+         PCI_BASE_ADDRESS_MEM_TYPE_64 )
+    {
+        if ( last )
+        {
+            printk(XENLOG_WARNING
+                   "%sdevice %04x:%02x:%02x.%u with 64-bit %sBAR in last slot\n",
+                   vf ? "SR-IOV " : "", sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
+                   vf ? "vf " : "");
+            return -EINVAL;
+        }
+        hi = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4);
+        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, ~0);
+    }
+    size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos) &
+           PCI_BASE_ADDRESS_MEM_MASK;
+    if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+         PCI_BASE_ADDRESS_MEM_TYPE_64 )
+    {
+        size |= (uint64_t)pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
+                                          sbdf.func, pos + 4) << 32;
+        pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, hi);
+    }
+    else if ( size )
+        size |= (uint64_t)~0 << 32;
+    pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, bar);
+    size = -size;
+    addr = (bar & 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 )
+        return 2;
+
+    return 1;
+}
+
 int pci_add_device(u16 seg, u8 bus, u8 devfn,
                    const struct pci_dev_info *info, nodeid_t node)
 {
@@ -674,11 +724,16 @@  int pci_add_device(u16 seg, u8 bus, u8 devfn,
             unsigned int i;
 
             BUILD_BUG_ON(ARRAY_SIZE(pdev->vf_rlen) != PCI_SRIOV_NUM_BARS);
-            for ( i = 0; i < PCI_SRIOV_NUM_BARS; ++i )
+            for ( i = 0; i < PCI_SRIOV_NUM_BARS; )
             {
                 unsigned int idx = pos + PCI_SRIOV_BAR + i * 4;
                 u32 bar = pci_conf_read32(seg, bus, slot, func, idx);
-                u32 hi = 0;
+                pci_sbdf_t sbdf = {
+                    .seg = seg,
+                    .bus = bus,
+                    .dev = slot,
+                    .func = func,
+                };
 
                 if ( (bar & PCI_BASE_ADDRESS_SPACE) ==
                      PCI_BASE_ADDRESS_SPACE_IO )
@@ -689,38 +744,13 @@  int pci_add_device(u16 seg, u8 bus, u8 devfn,
                            seg, bus, slot, func, i);
                     continue;
                 }
-                pci_conf_write32(seg, bus, slot, func, idx, ~0);
-                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
-                {
-                    if ( i >= PCI_SRIOV_NUM_BARS )
-                    {
-                        printk(XENLOG_WARNING
-                               "SR-IOV device %04x:%02x:%02x.%u with 64-bit"
-                               " vf BAR in last slot\n",
-                               seg, bus, slot, func);
-                        break;
-                    }
-                    hi = pci_conf_read32(seg, bus, slot, func, idx + 4);
-                    pci_conf_write32(seg, bus, slot, func, idx + 4, ~0);
-                }
-                pdev->vf_rlen[i] = pci_conf_read32(seg, bus, slot, func, idx) &
-                                   PCI_BASE_ADDRESS_MEM_MASK;
-                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
-                {
-                    pdev->vf_rlen[i] |= (u64)pci_conf_read32(seg, bus,
-                                                             slot, func,
-                                                             idx + 4) << 32;
-                    pci_conf_write32(seg, bus, slot, func, idx + 4, hi);
-                }
-                else if ( pdev->vf_rlen[i] )
-                    pdev->vf_rlen[i] |= (u64)~0 << 32;
-                pci_conf_write32(seg, bus, slot, func, idx, bar);
-                pdev->vf_rlen[i] = -pdev->vf_rlen[i];
-                if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
-                     PCI_BASE_ADDRESS_MEM_TYPE_64 )
-                    ++i;
+                ret = pci_size_mem_bar(sbdf, idx, i == PCI_SRIOV_NUM_BARS - 1,
+                                       NULL, &pdev->vf_rlen[i], PCI_BAR_VF);
+                if ( ret < 0 )
+                    break;
+
+                ASSERT(ret);
+                i += ret;
             }
         }
         else
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index b7a6abfc53..2bee6a3247 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -189,6 +189,10 @@  const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus,
 const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus,
                           unsigned int *dev, unsigned int *func, bool *def_seg);
 
+#define _PCI_BAR_VF     0
+#define PCI_BAR_VF      (1u << _PCI_BAR_VF)
+int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos, bool last,
+                     uint64_t *addr, uint64_t *size, unsigned int flags);
 
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);