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

Re: [PATCH RFC] xen/pci: detect when BARs overlap RAM


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 26 Jan 2022 15:08:28 +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=MmZ1USskdLzeJF9Wk9MCWDo5Q/qi0RGnSUpeupKL0pw=; b=BiMr9Mck8MYRDmuYKEXdqaMKzXnnbzn0YWCskfONJ/EicbIqkWkJMGWHuaV2765HHWuBAnC2S85JD9TZDjJJMc9qFUI3SdKG/oTXMoiXrD4rgBSOMpKqk1Pbhg6k//ZhpWgqXZhczkmi/mwZMmX6+tpruRMjp/LF8eJTac8U9ZzIWPuSXQKvFMRs80+o1nznPT69SvkU1kx4LzymC+q/q1hFXlzMTaYjB26Qhx93hR6cf+xoVyYUwCP16HE5GcYHCxIR+E1yj+sGqxgIzaFnkVIpxoI/Z2DAoU+TPgPrEBIciZpra6i8KHyRIp5IvyEoqxP/a+cs2KDgu1Ab7IPC2Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nuL50o/WvPkGXRRX7gDcoTf4MvPdUs03VO0Q7NjqqvZwtXMoFKY/ys/7VI4utZ23V5Zsyukl/mEZl2C5SCx7/3kbxw53YFBejuXuvHMMBl/STt/dVvHrjqLuhIEPJxxhAnQQm/BIELm2YA2CIgD5ij//JTB5l7uhHfYRyAq4R+ntGbt+atZbdCtupMTL2UYFbOaumkpyy6DpBXk4bmrvfPL+aGrQiBpGOLsjJSWIHYfmGgQGqe910QWwo8FbRDOGv7cYrq3UXMiBydN4Xbi5mIJS4Xp41CCGxLiVsiaJu15rtk5uNdWelTUm5nqZQJfafv1wTHcDqeVlo1DoAvHIuw==
  • 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: Wed, 26 Jan 2022 14:08:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.01.2022 13:26, Roger Pau Monne wrote:
> RFC because:
>  - Not sure the best way to implement is_iomem_range on Arm. BARs can
>    be quite big, so iterating over every possible page is not ideal.
>  - is_iomem_page cannot be used for this purpose on x86, because all
>    the low 1MB will return true due to belonging to dom_io.

I don't see an issue there - if you were to us it, you'd end up with
the same scalability issue as you point out for Arm.

>  - VF BARs are not checked. Should we also check them and disable VF
>    if overlaps in a followup patch?

Not sure about "followup patch", but once we support SR-IOV for PVH,
that'll want checking, yes.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -479,6 +479,26 @@ unsigned int page_get_ram_type(mfn_t mfn)
>      return type ?: RAM_TYPE_UNKNOWN;
>  }
>  
> +bool is_iomem_range(uint64_t start, uint64_t size)

Might be nice to have this sit next it is_iomem_page(). And wouldn't
at least "start" better be paddr_t?

> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < e820.nr_map; i++ )
> +    {
> +        struct e820entry *entry = &e820.map[i];

const?

> +        if ( entry->type != E820_RAM && entry->type != E820_ACPI &&
> +             entry->type != E820_NVS )
> +            continue;

I think UNUSABLE also needs checking for here. Even further, it might
be best to reject any range overlapping an E820 entry, since colliding
with a RESERVED one could mean an overlap with e.g. MMCFG space.

> @@ -267,11 +270,74 @@ 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 RAM 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 )
> +            return;

This isn't very nice, but probably the best you can do. Might be worth
a comment, though.

> +        if ( size && !is_iomem_range(addr, size) )
> +        {
> +            /*
> +             * Return without enabling memory decoding if BAR overlaps with
> +             * RAM region.
> +             */
> +            printk(XENLOG_WARNING
> +                   "%pp: disabled: BAR%u [%" PRIx64 ", %" PRIx64
> +                   ") overlaps with RAM\n",

Mentioning "RAM" in comment and log message is potentially misleading
when considering what other types get also checked (irrespective of my
earlier comment). (Ftaod I don't mind the title using "RAM".)

Also please don't wrap the format string (also again for ROM below).

> @@ -399,8 +465,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, 
> u8 bus, u8 devfn)
>              break;
>      }
>  
> -    check_pdev(pdev);
>      apply_quirks(pdev);
> +    check_pdev(pdev);

I can't find the description say anything about this re-ordering. What's
the deal here?

Jan




 


Rackspace

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