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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> --- 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.

> @@ -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.

Jan