On Thu, Jan 27, 2022 at 09:48:16AM +0100, Jan Beulich wrote:
> On 27.01.2022 09:22, Roger Pau Monne wrote:
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1625,6 +1625,17 @@ bool is_iomem_page(mfn_t mfn)
> > return !mfn_valid(mfn);
> > }
> >
> > +bool is_iomem_range(paddr_t start, uint64_t size)
> > +{
> > + unsigned int i;
> > +
> > + for ( i = 0; i < PFN_UP(size); i++ )
> > + if ( !is_iomem_page(_mfn(PFN_DOWN(start) + i)) )
> > + return false;
> > +
> > + return true;
> > +}
>
> I'm afraid you can't very well call this non-RFC as long as this doesn't
> scale. Or if you're building on this still being dead code on Arm, then
> some kind of "fixme" annotation would be needed here (or the function be
> omitted altogether, for Arm folks to sort out before they actually start
> selecting the HAS_PCI Kconfig setting).
I was expecting the lack of 'RFC' tag to attract the attention of the
maintainers for feedback. If not I'm happy to add a FIXME here or just
drop the chunk.
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -783,6 +783,23 @@ bool is_iomem_page(mfn_t mfn)
> > return (page_get_owner(page) == dom_io);
> > }
> >
> > +bool is_iomem_range(paddr_t start, uint64_t size)
> > +{
> > + unsigned int i;
> > +
> > + for ( i = 0; i < e820.nr_map; i++ )
> > + {
> > + const struct e820entry *entry = &e820.map[i];
> > +
> > + /* Do not allow overlaps with any memory range. */
> > + if ( start < entry->addr + entry->size &&
> > + entry->addr < start + size )
> > + return false;
> > + }
> > +
> > + return true;
> > +}
>
> I should have realized (and pointed out) earlier that with the type
> check dropped the function name no longer represents what the
> function does. It now really is unsuitable for about anything other
> that the checking of BARs.
is_devmem_range would be more suitable?
I will wrap this in an #ifdef HAS_PCI section now.
> > @@ -267,11 +270,75 @@ static void check_pdev(const struct pci_dev *pdev)
> > }
> > break;
> >
> > + case PCI_HEADER_TYPE_NORMAL:
> > + nbars = PCI_HEADER_NORMAL_NR_BARS;
> > + rom_pos = PCI_ROM_ADDRESS;
> > + break;
> > +
> > case PCI_HEADER_TYPE_CARDBUS:
> > /* TODO */
> > break;
> > }
> > #undef PCI_STATUS_CHECK
> > +
> > + /* Check if BARs overlap with other memory regions. */
> > + val = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> > + if ( !(val & PCI_COMMAND_MEMORY) || pdev->ignore_bars )
> > + return;
> > +
> > + pci_conf_write16(pdev->sbdf, PCI_COMMAND, val & ~PCI_COMMAND_MEMORY);
> > + for ( i = 0; i < nbars; )
> > + {
> > + uint64_t addr, size;
> > + unsigned int reg = PCI_BASE_ADDRESS_0 + i * 4;
> > + int rc = 1;
> > +
> > + if ( (pci_conf_read32(pdev->sbdf, reg) & PCI_BASE_ADDRESS_SPACE) !=
> > + PCI_BASE_ADDRESS_SPACE_MEMORY )
> > + goto next;
> > +
> > + rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
> > + (i == nbars - 1) ? PCI_BAR_LAST : 0);
> > + if ( rc < 0 )
> > + /* Unable to size, better leave memory decoding disabled. */
> > + return;
> > + if ( size && !is_iomem_range(addr, size) )
> > + {
> > + /*
> > + * Return without enabling memory decoding if BAR position is
> > not
> > + * in IO suitable memory. Let the hardware domain re-position
> > the
> > + * BAR.
> > + */
> > + printk(XENLOG_WARNING
> > +"%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlaps with memory
> > map\n",
>
> I guess despite its length this still wants indenting properly. Maybe
> you could fold this and ...
>
> > + &pdev->sbdf, i, addr, addr + size);
> > + return;
> > + }
> > +
> > + next:
> > + ASSERT(rc > 0);
> > + i += rc;
> > + }
> > +
> > + if ( rom_pos &&
> > + (pci_conf_read32(pdev->sbdf, rom_pos) & PCI_ROM_ADDRESS_ENABLE) )
> > + {
> > + uint64_t addr, size;
> > + int rc = pci_size_mem_bar(pdev->sbdf, rom_pos, &addr, &size,
> > + PCI_BAR_ROM);
> > +
> > + if ( rc < 0 )
> > + return;
> > + if ( size && !is_iomem_range(addr, size) )
> > + {
> > + printk(XENLOG_WARNING
> > +"%pp disabled: ROM BAR [%" PRIx64 ", %" PRIx64 ") overlaps with memory
> > map\n",
>
> .. this into
>
> static const char warn[] =
> "%pp disabled: %sBAR [%" PRIx64 ", %" PRIx64 ") overlaps with memory
> map\n";
>
> to limit indentation and redundancy at the same time? Could then also
> be re-used later for the SR-IOV BAR check.
Sure.
Thanks, Roger.
