linux-kernel - Re: [PATCH hmm 5/5] mm/hmm: remove the customizable pfn format from hmm_range

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200423061023.GA9856@lst.de>
Date:   Thu, 23 Apr 2020 08:10:23 +0200
From:   Christoph Hellwig <hch@....de>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Christoph Hellwig <hch@....de>, linux-mm@...ck.org,
        Ralph Campbell <rcampbell@...dia.com>,
        Alex Deucher <alexander.deucher@....com>,
        amd-gfx@...ts.freedesktop.org, Ben Skeggs <bskeggs@...hat.com>,
        Christian König <christian.koenig@....com>,
        "David (ChunMing) Zhou" <David1.Zhou@....com>,
        dri-devel@...ts.freedesktop.org,
        "Kuehling, Felix" <Felix.Kuehling@....com>,
        intel-gfx@...ts.freedesktop.org,
        Jérôme Glisse <jglisse@...hat.com>,
        John Hubbard <jhubbard@...dia.com>,
        linux-kernel@...r.kernel.org,
        Niranjana Vishwanathapura <niranjana.vishwanathapura@...el.com>,
        nouveau@...ts.freedesktop.org
Subject: Re: [PATCH hmm 5/5] mm/hmm: remove the customizable pfn format
 from hmm_range_fault

On Wed, Apr 22, 2020 at 09:39:11AM -0300, Jason Gunthorpe wrote:
> > Nice callchain from hell..  Unfortunately such "code listings" tend to
> > get out of date very quickly, so I'm not sure it is worth keeping in
> > the code.  What would be really worthile is consolidating the two
> > different sets of defines (NVIF_VMM_PFNMAP_V0_ vs NVKM_VMM_PFN_)
> > to make the code a little easier to follow.
> 
> I was mainly concerned that this function is using hmm properly,
> becuase it sure looks like it is just forming the CPU physical address
> into a HW specific data. But it turns out it is just an internal data
> for some other code and the dma_map is impossibly far away
> 
> It took forever to find, I figured I'd leave a hint for the next poor
> soul that has to look at this.. 
> 
> Also, I think it shows there is no 'performance' argument here, if
> this path needs more performance the above should be cleaned
> before we abuse hmm_range_fault.
> 
> Put it in the commit message instead?

Yes, the graph itself sounds reasonable for the commit log as a point
of time information.

> > >  	npages = (range->end - range->start) >> PAGE_SHIFT;
> > >  	for (i = 0; i < npages; ++i) {
> > >  		struct page *page;
> > >  
> > > +		if (!(range->hmm_pfns[i] & HMM_PFN_VALID)) {
> > > +			ioctl_addr[i] = 0;
> > >  			continue;
> > > +		}
> > 
> > Can't we rely on the caller pre-zeroing the array?
> 
> This ends up as args.phys in nouveau_svm_fault - I didn't see a
> zeroing?
> 
> I think it makes sense that this routine fully sets the output array
> and does not assume pre-initialize

Ok.