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

[Ingo Molnar <mingo@kernel.org>]

* Christoph Hellwig <hch@lst.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