Re: [PATCH v2] xen/pci: detect when BARs are not suitably positioned

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.