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

Re: [PATCH v4 17/21] AMD/IOMMU: replace all-contiguous page tables by superpage mappings


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 20 May 2022 12:35:20 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; 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=rofpCty6Kvkv0LUEsCfwK0/E6lctfHUDZtt+xooZ7sk=; b=NrErsk+9iLWr62m9fefmmy+6xsFlNVJNJK/lokH8VSDn0n4duxMwmwq5wUUck+pfBZYoZbcgk0geyAmoF6hdqLr5mYr63NW/7XDsymDb+8cABraSuxXPh3iDXmnN7IIo/d7O2148o5bk4dp4vopZ6BQHX+FumtzDwEZDQRfMRZHegApnr654TVVUklPkj8KR1p1gAQHpSH4tZx+dNdk7oUgKx98eqyKiOtRPU8/9LwZmx/emiD9xlthaYVqQWJACg2+lwZWwYbmUppHnjtFb4E/JZ6GZ9/kUVMv69XBilLaUzRAtokMtANtnjkBUHDrzahxhyrBXll7G7fUnV8/djg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iUIPDmoQKKBIJ/CJ3VhGwrCKvaNMPVd6fOGAq34UmJLcuf9f2T95U1ewKxkVO9vrHQTTq66lqwnYZcl5cva7YvdsS0OsWhyjHtyTortTioMwOx8UtguRHPkSL4l/IcaXHtCWxTGYXdnVONdmDEUG64FOIReZxJbKlY0NecjbbQ9mQmFLjF/WIopbeeFj74jxwfEplhSRhZa87tMvNjiL78PRGd7bE9CxjKf86MC+SkzAn4xdiV2n4txlOzYBF/tmPRHPRCIUhQbSL75c2NW5GKRODuYjSW//7aBC/s7ldSjz3GfWtT5vDvqcUTZPeDFpRKYsQKh5uHSwHuzIfx2RYg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Fri, 20 May 2022 10:35:38 +0000
  • Ironport-data: A9a23:mfxy9KMZYpEjZuLvrR3SlsFynXyQoLVcMsEvi/4bfWQNrUpw3j0Bz jZMWz2DPK3cYDGjKNx2O4jgoRkEsZGAzYcwHAto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdleF+lH1dOKJQUBUjclkfJKlYAL/En03FFYMpBsJ00o5wbZk298w27BVPivW0 T/Mi5yHULOa82Yc3lI8s8pvfzs24ZweEBtB1rAPTagjUG32zhH5P7pGTU2FFFPqQ5E8IwKPb 72rIIdVXI/u10xF5tuNyt4Xe6CRK1LYFVDmZnF+A8BOjvXez8CbP2lS2Pc0MC9qZzu1c99Z0 49is42fWzoQPYrNyLUgCjgCDn9aMvgTkFPHCSDXXc276WTjKiGp5so0SUY8MMsf5/p9BnxI+ boAMjcRYxufhuWwhrWmVu1rgcdlJ87uVG8dkig4kXeFUrB4H9afGM0m5vcBtNs0rtpJEvvEI dIQdBJkbQjaYg0JMVASYH47tLj03CiiKWAEwL6TjbUrs2T3711c66K3MfOPcO6TZ+ROrEnN8 woq+Ey8WHn2Lue3yzCI73atje/nhj7gVcQZE7jQ3u5nhhify3IeDDUSVECnur+ph0imQdVdJ kcIvC00osAa7EW2SvHtUhv+p2SL1iPwQPJVGuw+rQSSkKzd5l/DAnBeFmIaLts7qMUxWDomk EeTmM/kDiBut7vTTm+B8rCTrnW5Pi19wXI+WBLohDAtu7HLyLzfRDqWJjq/OMZZVuHIJAw=
  • Ironport-hdrordr: A9a23:n3dgiahTF6LtCpLACeFzqGSLcHBQX0h13DAbv31ZSRFFG/FwyP rCoB1L73XJYWgqM03I+eruBEBPewK4yXdQ2/hoAV7EZnichILIFvAa0WKG+VHd8kLFltK1uZ 0QEJSWTeeAd2SS7vyKnzVQcexQp+VvmZrA7Ym+854ud3ANV0gJ1XYENu/xKDwTeOApP+taKH LKjfA32gZINE5nJ/iTNz0gZazuttfLnJXpbVovAAMm0hCHiXeN5KThGxaV8x8CW3cXqI1Sul Ttokjc3OGOovu7whjT2yv66IlXosLozp9mCNaXgsYYBz3wgkKDZZhnWZeFoDcpydvfoGoCoZ 3pmVMNLs5z43TeciWcpgbs4RDp1HIU53rr2Taj8A/eiP28YAh/J9tKhIpffBecwVEnpstA3K VC2H/cn4ZLDDvb9R6NqOTgZlVPrA6ZsHAimekcgzh0So0FcoJcqoQZ4Qd8DIoAJiTn84oqed MeQP003MwmMG9yUkqp/lWGmLeXLzcO91a9MwU/U/WuonZrdCsT9Tpb+CQd9k1wga7VBaM0ot gsCZ4Y5Y2mfvVmE56VO91xMfdfKla9Ni4kY1jiV2gOKsk8SgHwgq+yxokJz8eXX7FN5KcOuf 36ISFlXCgJCgjTNfE=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, May 18, 2022 at 12:40:59PM +0200, Jan Beulich wrote:
> On 10.05.2022 17:31, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:43:16AM +0200, Jan Beulich wrote:
> >> @@ -94,11 +95,15 @@ static union amd_iommu_pte set_iommu_pte
> >>           old.iw != iw || old.ir != ir )
> >>      {
> >>          set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> >> -        pt_update_contig_markers(&table->raw, pfn_to_pde_idx(dfn, level),
> >> -                                 level, PTE_kind_leaf);
> >> +        *contig = pt_update_contig_markers(&table->raw,
> >> +                                           pfn_to_pde_idx(dfn, level),
> >> +                                           level, PTE_kind_leaf);
> >>      }
> >>      else
> >> +    {
> >>          old.pr = false; /* signal "no change" to the caller */
> >> +        *contig = false;
> > 
> > So we assume that any caller getting contig == true must have acted
> > and coalesced the page table?
> 
> Yes, except that I wouldn't use "must", but "would". It's not a
> requirement after all, functionality-wise all will be fine without
> re-coalescing.
> 
> > Might be worth a comment, to note that the function assumes that a
> > previous return of contig == true will have coalesced the page table
> > and hence a "no change" PTE write is not expected to happen on a
> > contig page table.
> 
> I'm not convinced, as there's effectively only one caller,
> amd_iommu_map_page(). I also don't see why "no change" would be a
> problem. "No change" can't result in a fully contiguous page table
> if the page table wasn't fully contiguous already before (at which
> point it would have been replaced by a superpage).

Right, I agree, it's just that I would have preferred the result from
set_iommu_ptes_present() to be consistent, ie: repeated calls to it
using the same PTE should set contig to the same value.  Anyway,
that's not relevant to any current callers, so:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.



 


Rackspace

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