linux-kernel - Re: [PATCH] SQUASHME: Fixes to e820 handling of pmem

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: <20150402093740.GA3706@gmail.com>
Date:	Thu, 2 Apr 2015 11:37:40 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Christoph Hellwig <hch@....de>
Cc:	Boaz Harrosh <boaz@...xistor.com>, linux-nvdimm@...1.01.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	x86@...nel.org, axboe@...nel.dk, ross.zwisler@...ux.intel.com
Subject: Re: [PATCH] SQUASHME: Fixes to e820 handling of pmem


* Christoph Hellwig <hch@....de> wrote:

> On Wed, Apr 01, 2015 at 05:25:22PM +0300, Boaz Harrosh wrote:
> >  		pfn = PFN_DOWN(ei->addr + ei->size);
> >  
> > -		switch (ei->type) {
> > -		case E820_RAM:
> > -		case E820_PRAM:
> > -		case E820_RESERVED_KERN:
> > -			break;
> > -		default:
> > +		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
> >  			register_nosave_region(PFN_UP(ei->addr), pfn);
> > -		}
> 
> I guess this makes sense - if the content is persistent already we don't need
> to save it.
> 
> > -		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
> > -			if (e820.map[i].type != E820_PRAM)
> > -				res->flags |= IORESOURCE_BUSY;
> > +		if (((e820.map[i].type != E820_RESERVED) &&
> > +		     (e820.map[i].type != E820_PRAM)) ||
> > +		     res->start < (1ULL<<20)) {
> 
> So now we also trigger for PRAM regions under 1ULL<<20, was that the
> intentional change?  Honestly I don't really understand this 1ULL<<20
> magic here even for the existing case.  Guess this is magic from the
> old ISA PC days?  
> 
> > +			res->flags |= IORESOURCE_BUSY;
> 
> Guess this is the real change, and I'd love to understand why this 
> makes a difference for you.  IORESOURCE_BUSY is checked almost 
> never, and is intented to mean it's a driver mapping.

So assuming this works on your test setup I'm inclined to squash 
Boaz's fixes into the original patch, assuming you see no outright bug 
in them. Anything else can be done as delta improvements.

Agreed?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/