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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 26 Jan 2022 16:45:09 +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=SzOx2XXdsPxuqJitDKtVyu6svMqQ7P5ryci3cfaR15M=; b=Ycv64D3PyjEhgDkcAXdGwXbagjgjvE2NdzdOncMaR8xe0f9GiEygU3tPsxWjx1keq1np9ckrOc4vfI18jnOmZjWBKJKWmIFJ1unG/TgW34+luZ6mqR7F13J7KtvslobTS6JBi7XMsQoNvevmBHLYlGRURMUmrckltsPDye1clekyTFi0ZW+AAK4mnubNf6BxkGesstqY/qqDGL8ZklsqNn3Cz2lIlnZhkh6qiQtdGtHA2n+le++e02zh3SInBLRiS588H4FCufSPmbHuSfDYS1ZBYDuNyGZiv7jgf/tbDyzfLibz/pggkKo4TShE4Y2zkmGigpBcpLUpdqDsFiryFw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MZmaFBRbdWUbYyyYvxML7E83+8VoSftM3bWVaK1iBt4UC5PP72NdumKV9fLJtaWDRRz4nL6judIaPjfLa69KN/QHKd1G421169rfLtDjJCV3mp3+DQo8noty1xnbAuAYLhaWeeJrp1l2o/y1uJCgfsUaVV/XBSDRVN8Qq9jvcBE7zfNrnk11D9TwjeQSD9QeE6uKN7qX19SNYc9ekSnj1q5yDMKpiUfe1M+2WRaC//IvN/+wMXDfOtCDoQpT/Q0GKNY5w1XfUnW9kVNb4exHovORskZ2GFjXi/Jq8tduhld80YxQmOkbtcrsM+N5TmEW1iU5SP0Q/58WxtF3K95cJQ==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.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 15:45:35 +0000
  • Ironport-data: A9a23:zch/0aPcaC9fQVHvrR3vkcFynXyQoLVcMsEvi/4bfWQNrUp2hmAFy DcbWzuEb/+LYmryLdtzbY/n/E9QvpHSx4dhGgto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6UUsxNbVU8En150Eg9w7RRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYoy65xfFO2 f5ziby5VyQ2P73il/sASSANRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/uQuoUIgm1u7ixINfnwa M4bSydoVkqeYTsMAkZHEMwzmfj90xETdBUH8QnI9MLb+VP70whZwLXrdt3PdbSiT8hPglyRo G6A+m3jGwwbL/SW0z/D+XWp7sffkCW+VI8MGbmQ8v9xnEbV1mEVEAcRV1awvb++kEHWc8pWN kg86ico668o+ySDVcLhVhe1pHqFuB80WNdKFeA+rgaXxcL84QyUG2wFRT5pc8E9uYk9QjlC/ mGOm9TlFDl+qoq/QHiW9qqXhT6qMC1TJmgHDQcGUA8E7t/LsIw1yBXVQb5e/LWd14OvX2uqm nbT8XZ41+57YdM3O7uT5mCdog29jab1bgcrxRrWTkyq8R5jXdvwD2C30mTz4fFFJYefa1COu nkYhsSThNwz4YGxeD+lG7tUQuzwjxqRGHiF2AM0QcF9n9i40yP7JehtDCdCyFCF2yruURvge wfttAxY//e/11P6PPYsM+pd5ynHpJUM9OgJtNiIP7KigbArLWdrGR2Cg2bKjggBd2B3yckC1 W+zK5rEMJrjIf0PIMCKb+kcy6Q34Ss12HneQ5v2pzz+j+bFPSXIF+tZbQvRBgzc0E9iiF+Em zq4H5DSoyizrcWkOnWHmWLtBQ1iwYcH6WDe9JUMK7/rzvtOE2A9Ef7BqY7NiKQ+95m5Ytzgp ynnMmcBkQKXrSSedW2iNy4/AJuyA8cXhS9rbEQEYAfzs1B+MNnH0UvqX8ZtFVXR3LY9nacco jhsU5joP8mjvRyeq21CNsGs9dI7HPlp7CrXVxeYjPEEV8cIbyTC+8P+fxup8y8LDyGtstA5r aHm3QTeKafvjSw4ZCoPQP7wnV63o1YHn+d+AxnBLtVJIR2++4l2MS3hyPQwJphUexnEwzKb0 SeQAAsZ+raR89NkroGRiPDWtZqtHst/AlFeQzvR44GpOHSI5WGk24JBDrqFJGiPSGPu9ay+T uxJ1PWgYuYflVNHvtMkQbZmxK4z/fX1oLpewlg2FXnHdQ3zWLhhPmOHzY9EsagUnu1Vvg6/W 0Su/NhGOOrWZJO5QQBJfAd8N7aNz/AZnDXW/M8ZGkSi6X8l5qeDXGVTIwKI1H5XIoxqPd532 uwmosMXtVCy00J4LtacgylI3G2QNXhcAb4/v5QXDYK32AomzlZOPc7VBiPsuczdbtxNNg8hI yOOhbqEjLNZnxKQf302HHnL/OxcmZVR50wakA5cfwyEyojfm/s6/BxN6jBmHA1awyJO3/93J mU2ZVZ+Ir+D/mswicVON4x299qt2PFNFpTN9mY0
  • Ironport-hdrordr: A9a23:Fk+I8aCkrWZWXJTlHemF55DYdb4zR+YMi2TDgXoBMSC9Ffbo8/ xG/c5rsCMd6l4qMk3I/OrsBEDuex/hHPJOjrX5Xo3SPjUO2lHJEGg41/qa/9SIIUSXndK1s5 0PT0EUMqySMbEVt6fHCKbTKada/DEqmprY4ts3bh1WPGdXV50=
  • Ironport-sdr: TYxtxBX139C8Vf1+o4QMWM+TU/Mj6ZUlISzy5gEPTa7H2nfXyO8h2iafvdRRsKN3MSBaCLvPtH gw+fVABsXLu4c5eYb8d2kQDwMJoo7OYMoZ/dmaPGFW+K57dULDPMX2vKlvX3CNqLYF29HEsXqe tZEJY6joAH/h15Vr0eyHqG9eSihSbNxhV7t1/lfS/pEkaYtUv8+undUIGKvhGrIR2oHu0cnRAN 8HMB11DIZljnjiXdLSC0f8dFadUa8Vn39sSIVzbfeSAie/7S11nIyca9lFUR55Sq2Rve8Icaqw U8b+UVyjKHwpUsNNMY89V9BY
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Jan 26, 2022 at 03:08:28PM +0100, Jan Beulich wrote:
> 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?

(paddr_t, size_t) would be better, but AFAICT size_t can be an
unsigned long and on Arm32 that won't be suitable for holding a 64bit
BAR size.

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

Hm, I've though about adding UNUSABLE and RESERVED (and should have
added a note about this), but that might be too restrictive.

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

Would the message below be better?

"%pp disabled: BAR%u [%" PRIx64 ", %" PRIx64 ") overlap detected\n"

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

Should have mentioned: this is required so that ignore_bars is set
prior to calling check_pdev.

Thanks, Roger.



 


Rackspace

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