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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 2 Feb 2022 13:13:06 +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=bH0J20QjsZiQMz+fLCrHrEdzdiz/5UYpEWCXb66QQYI=; b=PzgWFh25zV0LKhoJbf4uS+xtjSptqVFfcJeB3CcYB+Z+KCiThU3pzB5ZrIfWe3CrbeoMSuwdxZMgLmM6yw6WXph0D0sSjDHvAgfm9yYMJhSHi58ybQq9b4MquIZzOFYaQMz7dCy8vRGHFwW2ZNGbfYJ7z1MauIhDq5SqQYhBuH+SX+ElznGymNpVB78S0A5q91/fL5ApTp5Iq2mDJws5rTR+YVnixx2f6i6vbqpqPP9j0L89QcMXD+H1sKXrCE+Syy5Q+fuwgkIA/BHXm5e0OkRStAODqSRK8s3oq0PVVU+XArmFLt7e/UBn3ufKkPhpkxXPp1H84+ZFZMVSOSpKOQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=g9q+5JgozLD0V+8Vqc2X2ITwCtfBGz/GpTBy/8sJx1ThtzLN7I/3KaKn74/AfW+md1u7RmgFyshdxSLVIglgSqe69+HeZ8+D8/KSCGNxHre+8USwpATdghUl1e3xpiuzW+XBvWDCcM5QM6Vsx2HzxxR9mYHzl2THtYinct6nLqoARCRvzRK9GFsIATufx2vx19uMjGeLzOaWeNxVN4eyxqOpLprNAOQXeTykRXpdli/ojz/U/XJ83AILYmy+/BszH5qwz7CS5ztT0JI/Mkudgnrch4poSRSCxz3OhXv9R5RelzDLgl2ChAxGOOR5iIMKfrYunSft5HVyaw/VyF+oCg==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 02 Feb 2022 12:13:27 +0000
  • Ironport-data: A9a23:y+pM9KqnHatkz2GuNsUJIfxPNJ9eBmKQYhIvgKrLsJaIsI4StFCzt garIBmDa6mCN2P8eNtwO4rkpBhT65GDztYwSQdspSg2FyJE9JuZCYyVIHmrMnLJJKUvbq7GA +byyDXkBJppJpMJjk71atANlZT4vE2xbuKU5NTsY0idfic5Dndx4f5fs7Rh2NQw24DjW1rlV e7a+KUzBnf0g1aYDUpMg06zgEsHUCPa4W5wUvQWPJinjXeG/5UnJMt3yZKZdhMUdrJ8DO+iL 9sv+Znilo/vE7XBPfv++lrzWhVirrc/pmFigFIOM0SpqkAqSiDfTs/XnRfTAKtao2zhojx/9 DlCnYPoW0QrOqzUosMmXiIBSS15I/Fe+KCSdBBTseTLp6HHW37lwvEoB0AqJ4wIvO1wBAmi9 9RBdmpLNErawbvrnvTrEYGAhex6RCXvFJkYtXx6iynQEN4tQIzZQrWM7thdtNs1rp4VQq+PN pVCAdZpREnaRC9/awtINMIdg/eSj0bjcWUEk3vA8MLb5ECMlVcsgdABKuH9YceWTM9YmkKZo GPu/GnjBBwectuFxlKt6nuxgsffkCW9X5gdfJWo+/gvjFCNy2g7DBwNSUD9sfS/klS5Wd9UN woT4CVGhao4+VGvT9L9dwalu3PCtRkZM/JPF8Uq5QfLzbDbiy6bG2wFQzhpeNEg8sgsSlQXO kShxo2zQ2Y16fvMFCzbpuz8QS6O1TY9MjcQJi0ATBM/xOLum9hpghXREs5jD/vg5jHqIg3Yz zePpSk4orwci88Xyqm2lWz6byKQSovhFVBsuFiONo6xxkYgPdP+OdT0gbTOxasYdO6kok+9U G/ociR0xMQHFtmzmSOEW43h95n5tq/eYFUwbbOCdqTNFghBGVb/LOi8AxkkfS+F1/ronheyO CfuVft5vsM7AZdTRfYfj3iNI8or17P8Mt/uS+rZaNFDCrAoKlPco303NB/Nhj69+KTJrU3YE czDGftA8F5AUfg3pNZIb7t1PUAXKtAWmjqIGMGTI+WP2ruCfn+FIYrpw3PVBt3VGJis+V2Pm /4GbpPi40wGDIXWP3eLmaZOcwFiBSVrVPje9p0MHsbec1UOJY3UI6KLqV/XU9Y7z/09eyah1 izVZ3K0P3Kk1CCedFXXNis+AF4tNL4mxU8G0eUXFQ/A81AoYJq17bdZcJ0yfLI98/dkw+IyR P4AE/hsyNwWItge0zhCP5T7sqJ4cxGn2VCHMya/OWBtdJ98XQ3ZvNTje1K3piUJCyO2s+o4o qGhiVyHEcZSGVw6AZaEcu+rwnOwoWMZxLB4UXzXL4QBY07r6oVrdXD816dlP8EWJBzf7TKGz ALKUwwArOzArtZtotnEjKyJtamzFO56EhYIFmXX9+/uZyLb4nCi0clLV+PRJWLRU2b9+aODY +RJzq6jbK1bzQgS64clSuRl16Mz4dfrtoR29AU8ESWZdUmvB5NhPmKCgZtFuJpSy+ILogCxQ E+OpIVXYO3bJMP/HVcNDwM5deDfh+oMkzzf4PlpckX34Ch7oOiOXUlIZkTejSVcKP1+MZ8/w Pdns8kTslTthh0vO9eAryZV62XTcSBQD/R57skXUN3xlw4m6lBeepiNWCb57aaGZ8hILkR3c CSfg7DPhugEy0fPG5bp+aMhAQaJaUwyhS13
  • Ironport-hdrordr: A9a23:V3dF+K2Ex4KiutrO4GU8tAqjBVByeYIsimQD101hICG9Lfb2qy n+ppgmPEHP5Qr5OEtApTiBUJPwJk800aQFm7X5XI3SJzUO3VHHEGgM1/qB/9SNIVyaygcZ79 YcT0EcMqyPMbEZt7eC3ODQKb9Jq7PmgcOVbKXlvg9QpGlRGt5dBmxCe2Cm+yNNNW177c1TLu vh2iMLnUvpRV0nKuCAQlUVVenKoNPG0LrgfB49HhYirC2Dlymh5rLWGwWRmk52aUIE/Z4StU z+1yDp7KSqtP+2jjfaym/o9pxT3P/s0MFKCsCggtUcbh/slgGrToJ8XKDqhkF/nMifrHIR1P XcqRYpOMp+r1vXY2GOuBPonzLt1T4/gkWSvmOwsD/Gm4jUVTg6A81OicZyaR3C8Xctu9l6ze Ziw3+Zn4A/N2KOoA3No/zzEz16nEu9pnQv1cQJiWZEbIcYYLhN6aQC4UJuFosaFi6S0vFqLA BXNrCc2B9qSyLbU5iA1VMfg+BEH05DUytue3Jy9PB8iFNt7TJEJ0hx/r1qop5PzuN5d3B+3Z W1Dk1frsA6ciYnV9MNOA4/e7rFNoXse2O7DIvAGyWvKEk4U0i92aIfpo9FoN2XRA==
  • Ironport-sdr: NutfG0mFnQDs/QMpkKUbO/t0aahJKnxr453SVZ4IoVYmkUQsqT9CTzC0KWU30NViCNHKHFWDtJ HQkyBBYep9uk2Pq4TKwKytiHdNVHdd9Rtw4r1tjMPC9c8umaB4pNCm27u5H9dCVoKyVPCVP33R kFJ6UseTiD3UhrsQhS79rSOiDw+LeoYA9WVIpUIaUkRgTwFBsI5pa6EncLFEXSupiaICO4sTRM I+/XC7oIJ/t4v9ZWsx9Im0w9WHrS/ITQ8261hiDbkL5KwfDukScFnZnm67VgA3nq85nLWbd11m LC2E7kZ9/1e34iRZhQ2SY2bQ
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Feb 02, 2022 at 10:42:22AM +0100, Jan Beulich wrote:
> On 01.02.2022 13:45, Roger Pau Monne wrote:
> > --- 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_memory_hole(unsigned long start, unsigned long end)
> > +{
> > +    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 < PFN_DOWN(entry->addr + entry->size) &&
> > +             PFN_DOWN(entry->addr) <= end )
> > +            return false;
> > +    }
> > +
> > +    return true;
> > +}
> 
> Doesn't the left side of the && need to use PFN_UP()?

Hm, I had is using PFN_UP before and switched to PFN_DOWN for some
weird reasoning.

> 
> I also think it would help if a brief comment ahead of the
> function said that the range is inclusive. Otherwise the use
> of < and >= gives the impression of something being wrong.
> Then again it may be better to switch to <= anyway, as I
> think you want to avoid possible zero-size regions (at which
> point subtracting 1 for using <= is going to be valid).

Right, so that would end up being:

start <= PFN_DOWN(entry->addr + entry->size - 1) &&

Rejecting entries with size == 0 beforehand.

> Finally I wonder whether the function parameters wouldn't
> better be named e.g. spfn and epfn, but maybe their units can
> be inferred from their types being unsigned long (which,
> however, would build on the assumption that we use appropriate
> types everywhere).

I guess I should switch to using mfn_t for the types and convert them
locally to unsigned long for the comparisons.

> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -554,6 +554,8 @@ int __must_check steal_page(struct domain *d, struct 
> > page_info *page,
> >  int page_is_ram_type(unsigned long mfn, unsigned long mem_type);
> >  /* Returns the page type(s). */
> >  unsigned int page_get_ram_type(mfn_t mfn);
> > +/* Check if a range falls into a hole in the memory map. */
> > +bool is_memory_hole(paddr_t start, uint64_t size);
> 
> While resolving to the same type, these now also want to be
> "unsigned long".

Doh, yes, sorry. Will convert them to mfn_t if we agree on that.

Thanks, Roger.



 


Rackspace

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