| Message ID | 20170427143546.14662-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -588,6 +588,51 @@ static void pci_enable_acs(struct pci_dev *pdev) > pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); > } > > +int pci_size_bar(unsigned int seg, unsigned int bus, unsigned int slot, > + unsigned int func, unsigned int base, unsigned int max_bars, > + unsigned int *index, uint64_t *addr, uint64_t *size) > +{ > + unsigned int idx = base + *index * 4; The parameter and variable naming looks confusing to me. Generally we call what we pass to pci_conf_read*() "pos". I think it would be better to have the caller pass pos and a boolean indicator whether another BAR is following, reducing the (base,max_bars,index) triplet to a pair, and the function returning a negative error or the (positive) number of BARs to increment by (the more that you leave half of the incrementing to the caller anyway). > + u32 bar = pci_conf_read32(seg, bus, slot, func, idx); > + u32 hi = 0; > + > + *addr = *size = 0; With addr not needed by the current only caller, please allow passing NULL there. I'm also unconvinced these initializations are actually needed. > + ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY); With this, the function name should be more like pci_size_mem_bar(). > + pci_conf_write32(seg, bus, slot, func, idx, ~0); > + if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == > + PCI_BASE_ADDRESS_MEM_TYPE_64 ) > + { > + if ( *index >= max_bars ) > + { > + dprintk(XENLOG_WARNING, > + "device %04x:%02x:%02x.%u with 64-bit BAR in last slot\n", > + seg, bus, slot, func); This was a normal printk() originally. > @@ -663,38 +708,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_bar(seg, bus, slot, func, pos + PCI_SRIOV_BAR, > + PCI_SRIOV_NUM_BARS, &i, &addr, > + &pdev->vf_rlen[i]); > + if ( ret ) > + dprintk(XENLOG_WARNING, > + "%04x:%02x:%02x.%u: failed to size SR-IOV BAR%u\n", > + seg, bus, slot, func, i); You shouldn't log two messages for the same problem (the called function already logs one). A final more general remark: With you intending to call this function from other than pci_add_device() context, some further care may / will be needed. For example, are all to be added callers such that you playing with config space won't interfere with what Dom0 does? Are you sure you can get away without disabling memory decode while fiddling with the BARs? Jan
On Fri, May 19, 2017 at 07:56:55AM -0600, Jan Beulich wrote: > >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote: > > @@ -663,38 +708,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_bar(seg, bus, slot, func, pos + PCI_SRIOV_BAR, > > + PCI_SRIOV_NUM_BARS, &i, &addr, > > + &pdev->vf_rlen[i]); > > + if ( ret ) > > + dprintk(XENLOG_WARNING, > > + "%04x:%02x:%02x.%u: failed to size SR-IOV BAR%u\n", > > + seg, bus, slot, func, i); > > You shouldn't log two messages for the same problem (the called > function already logs one). > > A final more general remark: With you intending to call this function > from other than pci_add_device() context, some further care may / > will be needed. For example, are all to be added callers such that > you playing with config space won't interfere with what Dom0 does? > Are you sure you can get away without disabling memory decode > while fiddling with the BARs? So far I've been able to get away, but you are right that callers should disable memory decode before trying to size the BARs. I will do this in the callers however. Roger.
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 2288cf8814..7710c41533 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -588,6 +588,51 @@ static void pci_enable_acs(struct pci_dev *pdev) pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); } +int pci_size_bar(unsigned int seg, unsigned int bus, unsigned int slot, + unsigned int func, unsigned int base, unsigned int max_bars, + unsigned int *index, uint64_t *addr, uint64_t *size) +{ + unsigned int idx = base + *index * 4; + u32 bar = pci_conf_read32(seg, bus, slot, func, idx); + u32 hi = 0; + + *addr = *size = 0; + + ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY); + pci_conf_write32(seg, bus, slot, func, idx, ~0); + if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == + PCI_BASE_ADDRESS_MEM_TYPE_64 ) + { + if ( *index >= max_bars ) + { + dprintk(XENLOG_WARNING, + "device %04x:%02x:%02x.%u with 64-bit BAR in last slot\n", + seg, bus, slot, func); + return -EINVAL; + } + hi = pci_conf_read32(seg, bus, slot, func, idx + 4); + pci_conf_write32(seg, bus, slot, func, idx + 4, ~0); + } + *size = 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 ) + { + *size |= (u64)pci_conf_read32(seg, bus, slot, func, idx + 4) << 32; + pci_conf_write32(seg, bus, slot, func, idx + 4, hi); + } + else if ( *size ) + *size |= (u64)~0 << 32; + pci_conf_write32(seg, bus, slot, func, idx, bar); + *size = -(*size); + *addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32); + if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == + PCI_BASE_ADDRESS_MEM_TYPE_64 ) + ++*index; + + return 0; +} + int pci_add_device(u16 seg, u8 bus, u8 devfn, const struct pci_dev_info *info, nodeid_t node) { @@ -652,7 +697,7 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, { unsigned int idx = pos + PCI_SRIOV_BAR + i * 4; u32 bar = pci_conf_read32(seg, bus, slot, func, idx); - u32 hi = 0; + uint64_t addr; if ( (bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO ) @@ -663,38 +708,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_bar(seg, bus, slot, func, pos + PCI_SRIOV_BAR, + PCI_SRIOV_NUM_BARS, &i, &addr, + &pdev->vf_rlen[i]); + if ( ret ) + dprintk(XENLOG_WARNING, + "%04x:%02x:%02x.%u: failed to size SR-IOV BAR%u\n", + seg, bus, slot, func, i); } } else diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index a83c4a1276..3d3853fd6f 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -165,6 +165,9 @@ const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, unsigned int *dev, unsigned int *func); 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_bar(unsigned int seg, unsigned int bus, unsigned int slot, + unsigned int func, unsigned int base, unsigned int max_bars, + unsigned int *index, uint64_t *addr, uint64_t *size); bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
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> --- Cc: Jan Beulich <jbeulich@suse.com> --- xen/drivers/passthrough/pci.c | 86 ++++++++++++++++++++++++++----------------- xen/include/xen/pci.h | 3 ++ 2 files changed, 56 insertions(+), 33 deletions(-)