Re: [PATCH] xen/setup: Remap Xen Identity Mapped RAM

[Matthew Rushton <mvrushton@gmail.com>]

On 07/08/14 08:57, Konrad Rzeszutek Wilk wrote:
> Responding :-)
>>
>> @@ -797,10 +794,9 @@ unsigned long __init set_phys_range_identity(unsigned long pfn_s,
>>   		if (!__set_phys_to_machine(pfn, IDENTITY_FRAME(pfn)))
>>   			break;
>> -	if (!WARN((pfn - pfn_s) != (pfn_e - pfn_s),
>> +	WARN((pfn - pfn_s) != (pfn_e - pfn_s),
>>   		"Identity mapping failed. We are %ld short of 1-1 mappings!\n",
>> -		(pfn_e - pfn_s) - (pfn - pfn_s)))
>> -		printk(KERN_DEBUG "1-1 mapping on %lx->%lx\n", pfn_s, pfn);
>> +		(pfn_e - pfn_s) - (pfn - pfn_s));
>>> I would leave that as is. If you really want to modify it - spin another
>>> patch for it.
>> The problem is because we now call set_phys_range_identity() on small blocks
>> the number of these messages printed is large. I can split it out to a
>> separate patch if you'd like.
> Please do. Also you would have to rebase all this code on the latest Linus's
> tree as there are some changes there.

I posted a v2 which split this patch out, addressed feedback and rebased 
on Linus's tree.


>>>>   	return pfn - pfn_s;
>>>>   }
>>>> diff --git a/arch/x86/xen/p2m.h b/arch/x86/xen/p2m.h
>>>> new file mode 100644
>>>> index 0000000..9ce4d51
>>>> --- /dev/null
>>>> +++ b/arch/x86/xen/p2m.h
>>>> @@ -0,0 +1,15 @@
>>>> +#ifndef _XEN_P2M_H
>>>> +#define _XEM_P2M_H
>>>> +
>>>> +#define P2M_PER_PAGE        (PAGE_SIZE / sizeof(unsigned long))
>>>> +#define P2M_MID_PER_PAGE    (PAGE_SIZE / sizeof(unsigned long *))
>>>> +#define P2M_TOP_PER_PAGE    (PAGE_SIZE / sizeof(unsigned long **))
>>>> +
>>>> +#define MAX_P2M_PFN         (P2M_TOP_PER_PAGE * P2M_MID_PER_PAGE * P2M_PER_PAGE)
>>>> +
>>>> +#define MAX_REMAP_RANGES    10
>>> Could you mention why 10 please?
>> 10 is simply what I thought a reasonable max could ever be for the number of
>> remapped ranges. A range here is defined as the combination of a contiguous
>> e820 I/O range and a contiguous e820 RAM range where the underlying I/O
>> memory will be remapped. I'm not really excited about this but I don't have
>> a better solution. Any ideas? I believe the original implementation has a
>> similar issue that it appears to ignore.
> Is that based on a worst case scenario? As in could you write in the comment
> an example of a worst case E820 and scenario and when would 10 be more than
> enough to cover it?
>

I changed reserved brk space slightly in my updated patch. Really 
MAX_REMAP_RANGES in the worst case should be E820MAX. That seems like an 
unrealistic worst case though and a lot of space to reserve. I'm fairly 
certain the existing code also has a similar worst case which it doesn't 
take into account. It currently seems to assume there is only one E820 
I/O region that will be identity mapped? I think it works because in 
practice we don't come close to the worst case. I'm still not sure how 
to best handle this.

>>>> +	mod = remap_pfn % P2M_PER_PAGE;
>>>> +	remap_start_pfn_align =
>>>> +		mod ? (remap_pfn - mod + P2M_PER_PAGE):remap_pfn;
>>>> +	mod = (start_pfn + size) % P2M_PER_PAGE;
>>>> +	ident_end_pfn_align = start_pfn + size - mod;
>>>> +	mod = (remap_pfn + size) % P2M_PER_PAGE;
>>>> +	remap_end_pfn_align = remap_pfn + size - mod;
>>> Should you do a check to make sure that 'ident_[start|end]_pfn_align'
>>> don't overlap with 'remap_[start|end]_pfn_align' ?
>>>
>>>> +
>>>> +	/* Iterate over each p2m leaf node in each range */
>>>> +	for (ident_pfn_iter = ident_start_pfn_align, remap_pfn_iter = remap_start_pfn_align;
>>>> +	     ident_pfn_iter < ident_end_pfn_align && remap_pfn_iter < remap_end_pfn_align;
>>>> +	     ident_pfn_iter += P2M_PER_PAGE, remap_pfn_iter += P2M_PER_PAGE) {
>>>> +		/* Check we aren't past the end */
>>>> +		BUG_ON(ident_pfn_iter + P2M_PER_PAGE > start_pfn + size);
>>>> +		BUG_ON(remap_pfn_iter + P2M_PER_PAGE > remap_pfn + size);
>>> Ah, here you check for it. But wouldn't it be easier to adjust the
>>> remap_[start|end]_pfn_align above to check for this and make changes?
>> The ident_* and remap_* address ranges will never overlap each other unless
>> the e820 map is broken. I'm treating both ranges as independent and
> Right.
>> iterating over each seperately at the same time. Each range will have
>> different boundary conditions potentially. Does that make sense? Am I
>> understanding the comment correctly?
> I was thinking that if the E820 is broken then try our best (and stop
> trying to remap) and continue. The E820 is guaranteed to be sane (no
> overlapping regions), but the sizes could be bogus (zero size for example).

I think the current code does handle the zero case ok.

>>>> +
>>>> +		/* Save p2m mappings */
>>>> +		for (i = 0; i < P2M_PER_PAGE; i++)
>>>> +			xen_remap_buf[i] = pfn_to_mfn(ident_pfn_iter + i);
>>>> +
>>>> +		/* Set identity map which also frees a p2m leaf */
>>>                                                 ^- might
>> Under what conditions is this not true? The goal of processing these in
>> blocks is to guarantee that a leaf is freed.
> Perhaps change it 'also frees' to 'MUST free'
> and then have (debug code) to double-check that Naturally
> after calling the early_set_phys_identity?
>
> 	for (i = 0; i < P2M_PER_PAGE; i++) {
> 		unsigned int pfn = ident_pfn_iter + i;
> 		BUG_ON(pfn_to_mfn(pfn) != INVALID_P2M_ENTRY)
> ?
I added a debug check. It must check for the identity page and not 
INVALID_P2M_ENTRY however.

>>>> +		ident_cnt += set_phys_range_identity(ident_pfn_iter,
>>>> +			ident_pfn_iter + P2M_PER_PAGE);
>>>> +
>>>> +		/* Now remap memory */
>>>> +		for (i = 0; i < P2M_PER_PAGE; i++) {
>>>> +			unsigned long mfn = xen_remap_buf[i];
>>>> +			struct mmu_update update = {
>>>> +				.ptr = (mfn << PAGE_SHIFT) |
>>>> +					MMU_MACHPHYS_UPDATE,
>>>> +				.val = remap_pfn_iter + i
>>>> +			};
>>>> +
>>>> +			/* Update p2m, will use the page freed above */
>>> What page freed above?
>> See above comment.
> Perhaps 'use the page free above' with 'use the P2M leaf freed above'.
ACK