| Message ID | 20170630150117.88489-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 30.06.17 at 17:01, <roger.pau@citrix.com> wrote: > 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> > > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -588,6 +588,54 @@ static void pci_enable_acs(struct pci_dev *pdev) > pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); > } > > +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) > +{ > + uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos); > + uint64_t addr, size; > + > + ASSERT((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 ( last ) > + { > + printk(XENLOG_WARNING > + "device %04x:%02x:%02x.%u with 64-bit BAR in last slot\n", This message needs to tell what kind of slot is being processed (just like the original did). > + seg, bus, slot, func); > + return -EINVAL; > + } > + hi = pci_conf_read32(seg, bus, slot, func, pos + 4); > + 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 ) > + { > + size |= (u64)pci_conf_read32(seg, bus, slot, func, pos + 4) << 32; uint64_t > + pci_conf_write32(seg, bus, slot, func, pos + 4, hi); > + } > + else if ( size ) > + size |= (u64)~0 << 32; Again (and more below). > + pci_conf_write32(seg, bus, slot, func, pos, bar); > + size = -(size); Stray parentheses. > + addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32); > + > + if ( paddr ) > + *paddr = addr; > + if ( psize ) > + *psize = size; Is it reasonable to expect the caller to not care about the size? > @@ -663,38 +710,12 @@ 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(seg, bus, slot, func, idx, > + i == PCI_SRIOV_NUM_BARS - 1, NULL, > + &pdev->vf_rlen[i]); > + if ( ret < 0 ) > + break; ASSERT(ret) ? > + i += ret; Jan
On Fri, Jul 14, 2017 at 04:33:20AM -0600, Jan Beulich wrote: > >>> On 30.06.17 at 17:01, <roger.pau@citrix.com> wrote: > > 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> > > > > --- a/xen/drivers/passthrough/pci.c > > +++ b/xen/drivers/passthrough/pci.c > > @@ -588,6 +588,54 @@ static void pci_enable_acs(struct pci_dev *pdev) > > pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); > > } > > > > +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) > > +{ > > + uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos); > > + uint64_t addr, size; > > + > > + ASSERT((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 ( last ) > > + { > > + printk(XENLOG_WARNING > > + "device %04x:%02x:%02x.%u with 64-bit BAR in last slot\n", > > This message needs to tell what kind of slot is being processed (just > like the original did). The original message is: "SR-IOV device %04x:%02x:%02x.%u with 64-bit vf BAR in last slot" I guess you would like to have the "vf" again, in which case I will add a bool vf parameter to the function that's only going to be used here. IMHO I'm not really sure it's worth it because I don't find it that informative. I though that just knowing the device sbdf is enough. > > + seg, bus, slot, func); > > + return -EINVAL; > > + } > > + hi = pci_conf_read32(seg, bus, slot, func, pos + 4); > > + 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 ) > > + { > > + size |= (u64)pci_conf_read32(seg, bus, slot, func, pos + 4) << 32; > > uint64_t > > > + pci_conf_write32(seg, bus, slot, func, pos + 4, hi); > > + } > > + else if ( size ) > > + size |= (u64)~0 << 32; > > Again (and more below). Yes, I think I've fixed all of them. > > + pci_conf_write32(seg, bus, slot, func, pos, bar); > > + size = -(size); > > Stray parentheses. > > > + addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32); > > + > > + if ( paddr ) > > + *paddr = addr; > > + if ( psize ) > > + *psize = size; > > Is it reasonable to expect the caller to not care about the size? Not at the moment, so I guess ASSERT(psize) would be better. > > @@ -663,38 +710,12 @@ 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(seg, bus, slot, func, idx, > > + i == PCI_SRIOV_NUM_BARS - 1, NULL, > > + &pdev->vf_rlen[i]); > > + if ( ret < 0 ) > > + break; > > ASSERT(ret) ? Really? This is different from the previous behavior, that would just break out of the loop in this situation. And on non-debug builds we would end up decreasing i, which is not good. Thanks for the review, Roger.
On Thu, Jul 20, 2017 at 03:00:40PM +0100, Roger Pau Monne wrote: > On Fri, Jul 14, 2017 at 04:33:20AM -0600, Jan Beulich wrote: > > >>> On 30.06.17 at 17:01, <roger.pau@citrix.com> wrote: > > > + if ( ret < 0 ) > > > + break; > > > > ASSERT(ret) ? > > Really? This is different from the previous behavior, that would just > break out of the loop in this situation. And on non-debug builds we > would end up decreasing i, which is not good. Figured that out, you wanted me to just add the ASSERT to make sure ret != 0. Thanks, Roger.
>>> Roger Pau Monne <roger.pau@citrix.com> 07/20/17 4:00 PM >>> >On Fri, Jul 14, 2017 at 04:33:20AM -0600, Jan Beulich wrote: > >>> On 30.06.17 at 17:01, <roger.pau@citrix.com> wrote: >> > --- a/xen/drivers/passthrough/pci.c >> > +++ b/xen/drivers/passthrough/pci.c >> > @@ -588,6 +588,54 @@ static void pci_enable_acs(struct pci_dev *pdev) >> > pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); >> > } >> > >> > +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) >> > +{ >> > + uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos); >> > + uint64_t addr, size; >> > + >> > + ASSERT((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 ( last ) >> > + { >> > + printk(XENLOG_WARNING >> > + "device %04x:%02x:%02x.%u with 64-bit BAR in last slot\n", >> >> This message needs to tell what kind of slot is being processed (just >> like the original did). > >The original message is: > >"SR-IOV device %04x:%02x:%02x.%u with 64-bit vf BAR in last slot" > >I guess you would like to have the "vf" again, in which case I will >add a bool vf parameter to the function that's only going to be used >here. Note also the "SR-IOV" at the beginning. But either part would be sufficient. > IMHO I'm not really sure it's worth it because I don't find it >that informative. I though that just knowing the device sbdf is >enough. It allows deducing the situation in which this function is being called. >> > + addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32); >> > + >> > + if ( paddr ) >> > + *paddr = addr; >> > + if ( psize ) >> > + *psize = size; >> >> Is it reasonable to expect the caller to not care about the size? > >Not at the moment, so I guess ASSERT(psize) would be better. I don't even see a need for such an ASSERT(). Jan
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 2d38a5a297..656a2a316b 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -588,6 +588,54 @@ static void pci_enable_acs(struct pci_dev *pdev) pci_conf_write16(seg, bus, dev, func, pos + PCI_ACS_CTRL, ctrl); } +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) +{ + uint32_t hi = 0, bar = pci_conf_read32(seg, bus, slot, func, pos); + uint64_t addr, size; + + ASSERT((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 ( last ) + { + printk(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, pos + 4); + 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 ) + { + size |= (u64)pci_conf_read32(seg, bus, slot, func, pos + 4) << 32; + pci_conf_write32(seg, bus, slot, func, pos + 4, hi); + } + else if ( size ) + size |= (u64)~0 << 32; + pci_conf_write32(seg, bus, slot, func, pos, bar); + size = -(size); + addr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((u64)hi << 32); + + if ( paddr ) + *paddr = addr; + if ( psize ) + *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) { @@ -648,11 +696,10 @@ 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; if ( (bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO ) @@ -663,38 +710,12 @@ 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(seg, bus, slot, func, idx, + i == PCI_SRIOV_NUM_BARS - 1, NULL, + &pdev->vf_rlen[i]); + if ( ret < 0 ) + break; + i += ret; } } else diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index e550effcc9..11ad185cec 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_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_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> --- 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 | 89 ++++++++++++++++++++++++++----------------- xen/include/xen/pci.h | 3 ++ 2 files changed, 58 insertions(+), 34 deletions(-)