[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 27 Jan 2022 09:48:16 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=XSK5J8ggiUsZRw8UOO8CzM47LF2LNC3PhGAh/1xyCS8=; b=P/wSXI0QaV52j8QFfNeM+JuYsVx2bO2/x0q07y9ilALmv3BbqTu7wTZlsL3LywIv7WMrcM8L2w4vDYbgi+ASFr3TS7fEgXzwLMB6dUZvRJGygRKR71rwoK6LVva4r5uVTffXTAiq/HiZVfL/yxQA3v0YF+Ywd9Ul0z5PI7hVn9NI2m8OxCALhjdMu1o0LB8qOY+UAUhpmDzW0wAvPTxrvAi1k4mBPNefLhhtH+xdmTSuzFWyrUDpDNIc4/59slgv1N+eUSWosvPyJRbmuSbygUazI1Do7rOfpjbOUryaX6yOp0gre5H7OAv6kbylf6XQ4MqYbsgvTCyGpMbVi+Oi8w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YzhJN9hcpQhXSa4lWGbLHrjZ7e7r+rkVu/scc12AOwNeVpBdJb6PvVJlmr4KQ/NbYK3dYODPcBFeWescjeUqHjWyuPDpyRZPPWuBdA/5Ri2OScCcIWp0KgKsw98mbmKg+ksqLcvBPg1hknV5lqzboqBJNHj/t1Yo2Hbfh+Y+JWG1eRQta20uZeVlzmr4PA7rlAHhBq+pSJ9+B2/3NkPo5dSRDak2I9k7PSMMImJa2cMlrWms59s0WNdY9HAd3/HqWsyDwddwuqlBORb+jaNFnnUas55cVy2YZQ+oEae9A4rbMOPMb3XPcjT+Y3AEeStbyezIVoEkP/rW1u/ffcQDXw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 27 Jan 2022 08:48:25 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.