[PATCH 0/3] xen: correct several bugs in new p2m list setup

[PATCH 0/3] xen: correct several bugs in new p2m list setup

[Juergen Gross]

In the setup code of the linear mapped p2m list several bugs have
been found, especially for 32 bit dom0. These patches correct the
errors and make 32 bit dom0 bootable again.

Juergen Gross (3):
  xen: correct error for building p2m list on 32 bits
  xen: correct race in alloc_p2m_pmd()
  xen: use correct type for physical addresses

 arch/x86/xen/p2m.c   | 11 ++++-------
 arch/x86/xen/setup.c |  6 ++++--
 2 files changed, 8 insertions(+), 9 deletions(-)

-- 
2.1.2

[PATCH 1/3] xen: correct error for building p2m list on 32 bits

[Juergen Gross]

In xen_rebuild_p2m_list() for large areas of invalid or identity
mapped memory the pmd entries on 32 bit systems are initialized
wrong. Correct this error.

Suggested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/p2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index d9660a5..36ae094 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -379,7 +379,7 @@ static void __init xen_rebuild_p2m_list(unsigned long *p2m)
 			p2m_missing_pte : p2m_identity_pte;
 		for (i = 0; i < PMDS_PER_MID_PAGE; i++) {
 			pmdp = populate_extra_pmd(
-				(unsigned long)(p2m + pfn + i * PTRS_PER_PTE));
+				(unsigned long)(p2m + pfn) + i * PMD_SIZE);
 			set_pmd(pmdp, __pmd(__pa(ptep) | _KERNPG_TABLE));
 		}
 	}
-- 
2.1.2

[PATCH 2/3] xen: correct race in alloc_p2m_pmd()

[Juergen Gross]

When allocating a new pmd for the linear mapped p2m list a check is
done for not introducing another pmd when this just happened on
another cpu. In this case the old pte pointer was returned which
points to the p2m_missing or p2m_identity page. The correct value
would be the pointer to the found new page.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/p2m.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 36ae094..fdb996e 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -440,10 +440,9 @@ EXPORT_SYMBOL_GPL(get_phys_to_machine);
  * a new pmd is to replace p2m_missing_pte or p2m_identity_pte by a individual
  * pmd. In case of PAE/x86-32 there are multiple pmds to allocate!
  */
-static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
+static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *pte_pg)
 {
 	pte_t *ptechk;
-	pte_t *pteret = ptep;
 	pte_t *pte_newpg[PMDS_PER_MID_PAGE];
 	pmd_t *pmdp;
 	unsigned int level;
@@ -477,8 +476,6 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
 		if (ptechk == pte_pg) {
 			set_pmd(pmdp,
 				__pmd(__pa(pte_newpg[i]) | _KERNPG_TABLE));
-			if (vaddr == (addr & ~(PMD_SIZE - 1)))
-				pteret = pte_offset_kernel(pmdp, addr);
 			pte_newpg[i] = NULL;
 		}
 
@@ -492,7 +489,7 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
 		vaddr += PMD_SIZE;
 	}
 
-	return pteret;
+	return lookup_address(addr, &level);
 }
 
 /*
@@ -521,7 +518,7 @@ static bool alloc_p2m(unsigned long pfn)
 
 	if (pte_pg == p2m_missing_pte || pte_pg == p2m_identity_pte) {
 		/* PMD level is missing, allocate a new one */
-		ptep = alloc_p2m_pmd(addr, ptep, pte_pg);
+		ptep = alloc_p2m_pmd(addr, pte_pg);
 		if (!ptep)
 			return false;
 	}
-- 
2.1.2

Re: [PATCH 2/3] xen: correct race in alloc_p2m_pmd()

[David Vrabel]

On 08/01/15 17:01, Juergen Gross wrote:
> When allocating a new pmd for the linear mapped p2m list a check is
> done for not introducing another pmd when this just happened on
> another cpu. In this case the old pte pointer was returned which
> points to the p2m_missing or p2m_identity page. The correct value
> would be the pointer to the found new page.

Looking at the check I don't see how it works at all.

alloc_p2m() looks up the address of the PTE page

	ptep = lookup_address(addr, &level);
	pte_pg = (pte_t *)((unsigned long)ptep & ~(PAGE_SIZE - 1));

But the check under the lock that is still true does:

	ptechk = lookup_address(vaddr, &level);
	if (ptechk == pte_pg) {

So I don't see how this works unless (by chance) we happen to get the
first entry in the PTE page.

It also doesn't handle racing with p2m_missing_pte to p2m_identity_pte
(or vice-versa) change.

Something like:

	ptechk = lookup_address(vaddr, &level);
	ptechk = (pte_t *)((unsigned long)ptechk & ~(PAGE_SIZE - 1));
	if (ptechk == p2m_missing_pte || ptechk == p2m_identity) {
              set_pmd(..)

Perhaps?

David

> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -440,10 +440,9 @@ EXPORT_SYMBOL_GPL(get_phys_to_machine);
>   * a new pmd is to replace p2m_missing_pte or p2m_identity_pte by a individual
>   * pmd. In case of PAE/x86-32 there are multiple pmds to allocate!
>   */
> -static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
> +static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *pte_pg)
>  {
>  	pte_t *ptechk;
> -	pte_t *pteret = ptep;
>  	pte_t *pte_newpg[PMDS_PER_MID_PAGE];
>  	pmd_t *pmdp;
>  	unsigned int level;
> @@ -477,8 +476,6 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>  		if (ptechk == pte_pg) {
>  			set_pmd(pmdp,
>  				__pmd(__pa(pte_newpg[i]) | _KERNPG_TABLE));
> -			if (vaddr == (addr & ~(PMD_SIZE - 1)))
> -				pteret = pte_offset_kernel(pmdp, addr);
>  			pte_newpg[i] = NULL;
>  		}
>  
> @@ -492,7 +489,7 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>  		vaddr += PMD_SIZE;
>  	}
>  
> -	return pteret;
> +	return lookup_address(addr, &level);
>  }
>  
>  /*
> @@ -521,7 +518,7 @@ static bool alloc_p2m(unsigned long pfn)
>  
>  	if (pte_pg == p2m_missing_pte || pte_pg == p2m_identity_pte) {
>  		/* PMD level is missing, allocate a new one */
> -		ptep = alloc_p2m_pmd(addr, ptep, pte_pg);
> +		ptep = alloc_p2m_pmd(addr, pte_pg);
>  		if (!ptep)
>  			return false;
>  	}
> 

Re: [PATCH 2/3] xen: correct race in alloc_p2m_pmd()

[David Vrabel]

On 08/01/15 17:01, Juergen Gross wrote:
> When allocating a new pmd for the linear mapped p2m list a check is
> done for not introducing another pmd when this just happened on
> another cpu. In this case the old pte pointer was returned which
> points to the p2m_missing or p2m_identity page. The correct value
> would be the pointer to the found new page.

Looking at the check I don't see how it works at all.

alloc_p2m() looks up the address of the PTE page

	ptep = lookup_address(addr, &level);
	pte_pg = (pte_t *)((unsigned long)ptep & ~(PAGE_SIZE - 1));

But the check under the lock that is still true does:

	ptechk = lookup_address(vaddr, &level);
	if (ptechk == pte_pg) {

So I don't see how this works unless (by chance) we happen to get the
first entry in the PTE page.

It also doesn't handle racing with p2m_missing_pte to p2m_identity_pte
(or vice-versa) change.

Something like:

	ptechk = lookup_address(vaddr, &level);
	ptechk = (pte_t *)((unsigned long)ptechk & ~(PAGE_SIZE - 1));
	if (ptechk == p2m_missing_pte || ptechk == p2m_identity) {
              set_pmd(..)

Perhaps?

David

> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -440,10 +440,9 @@ EXPORT_SYMBOL_GPL(get_phys_to_machine);
>   * a new pmd is to replace p2m_missing_pte or p2m_identity_pte by a individual
>   * pmd. In case of PAE/x86-32 there are multiple pmds to allocate!
>   */
> -static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
> +static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *pte_pg)
>  {
>  	pte_t *ptechk;
> -	pte_t *pteret = ptep;
>  	pte_t *pte_newpg[PMDS_PER_MID_PAGE];
>  	pmd_t *pmdp;
>  	unsigned int level;
> @@ -477,8 +476,6 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>  		if (ptechk == pte_pg) {
>  			set_pmd(pmdp,
>  				__pmd(__pa(pte_newpg[i]) | _KERNPG_TABLE));
> -			if (vaddr == (addr & ~(PMD_SIZE - 1)))
> -				pteret = pte_offset_kernel(pmdp, addr);
>  			pte_newpg[i] = NULL;
>  		}
>  
> @@ -492,7 +489,7 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>  		vaddr += PMD_SIZE;
>  	}
>  
> -	return pteret;
> +	return lookup_address(addr, &level);
>  }
>  
>  /*
> @@ -521,7 +518,7 @@ static bool alloc_p2m(unsigned long pfn)
>  
>  	if (pte_pg == p2m_missing_pte || pte_pg == p2m_identity_pte) {
>  		/* PMD level is missing, allocate a new one */
> -		ptep = alloc_p2m_pmd(addr, ptep, pte_pg);
> +		ptep = alloc_p2m_pmd(addr, pte_pg);
>  		if (!ptep)
>  			return false;
>  	}
> 

Re: [PATCH 2/3] xen: correct race in alloc_p2m_pmd()

[Juergen Gross]

On 01/09/2015 04:09 PM, David Vrabel wrote:
> On 08/01/15 17:01, Juergen Gross wrote:
>> When allocating a new pmd for the linear mapped p2m list a check is
>> done for not introducing another pmd when this just happened on
>> another cpu. In this case the old pte pointer was returned which
>> points to the p2m_missing or p2m_identity page. The correct value
>> would be the pointer to the found new page.
>
> Looking at the check I don't see how it works at all.
>
> alloc_p2m() looks up the address of the PTE page
>
> 	ptep = lookup_address(addr, &level);
> 	pte_pg = (pte_t *)((unsigned long)ptep & ~(PAGE_SIZE - 1));
>
> But the check under the lock that is still true does:
>
> 	ptechk = lookup_address(vaddr, &level);
> 	if (ptechk == pte_pg) {
>
> So I don't see how this works unless (by chance) we happen to get the
> first entry in the PTE page.

The check under lock tests vaddr which is pmd aligned.

> It also doesn't handle racing with p2m_missing_pte to p2m_identity_pte
> (or vice-versa) change.

The change is always and only for missing/identity to individual pmd.
There is no transition between all missing and identity pmds.

> Something like:
>
> 	ptechk = lookup_address(vaddr, &level);
> 	ptechk = (pte_t *)((unsigned long)ptechk & ~(PAGE_SIZE - 1));
> 	if (ptechk == p2m_missing_pte || ptechk == p2m_identity) {
>                set_pmd(..)
>
> Perhaps?

Would work, but is more complicated than needed. :-)


Juergen

>
> David
>
>> --- a/arch/x86/xen/p2m.c
>> +++ b/arch/x86/xen/p2m.c
>> @@ -440,10 +440,9 @@ EXPORT_SYMBOL_GPL(get_phys_to_machine);
>>    * a new pmd is to replace p2m_missing_pte or p2m_identity_pte by a individual
>>    * pmd. In case of PAE/x86-32 there are multiple pmds to allocate!
>>    */
>> -static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>> +static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *pte_pg)
>>   {
>>   	pte_t *ptechk;
>> -	pte_t *pteret = ptep;
>>   	pte_t *pte_newpg[PMDS_PER_MID_PAGE];
>>   	pmd_t *pmdp;
>>   	unsigned int level;
>> @@ -477,8 +476,6 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>>   		if (ptechk == pte_pg) {
>>   			set_pmd(pmdp,
>>   				__pmd(__pa(pte_newpg[i]) | _KERNPG_TABLE));
>> -			if (vaddr == (addr & ~(PMD_SIZE - 1)))
>> -				pteret = pte_offset_kernel(pmdp, addr);
>>   			pte_newpg[i] = NULL;
>>   		}
>>
>> @@ -492,7 +489,7 @@ static pte_t *alloc_p2m_pmd(unsigned long addr, pte_t *ptep, pte_t *pte_pg)
>>   		vaddr += PMD_SIZE;
>>   	}
>>
>> -	return pteret;
>> +	return lookup_address(addr, &level);
>>   }
>>
>>   /*
>> @@ -521,7 +518,7 @@ static bool alloc_p2m(unsigned long pfn)
>>
>>   	if (pte_pg == p2m_missing_pte || pte_pg == p2m_identity_pte) {
>>   		/* PMD level is missing, allocate a new one */
>> -		ptep = alloc_p2m_pmd(addr, ptep, pte_pg);
>> +		ptep = alloc_p2m_pmd(addr, pte_pg);
>>   		if (!ptep)
>>   			return false;
>>   	}
>>
>
>

[PATCH 3/3] xen: use correct type for physical addresses

[Juergen Gross]

When converting a pfn to a physical address be sure to use 64 bit
wide types.

Also avoid invalidating memory for zero sized non-aligned extra
memory regions.

Signed-off-by: Juergen Gross <jgross@suse.com>
Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/setup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index feb6d86..8839d7b 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -140,7 +140,7 @@ static void __init xen_del_extra_mem(u64 start, u64 size)
 unsigned long __ref xen_chk_extra_mem(unsigned long pfn)
 {
 	int i;
-	unsigned long addr = PFN_PHYS(pfn);
+	u64 addr = PFN_PHYS(pfn);
 
 	for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
 		if (addr >= xen_extra_mem[i].start &&
@@ -160,6 +160,8 @@ void __init xen_inv_extra_mem(void)
 	int i;
 
 	for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
+		if (!xen_extra_mem[i].size)
+			continue;
 		pfn_s = PFN_DOWN(xen_extra_mem[i].start);
 		pfn_e = PFN_UP(xen_extra_mem[i].start + xen_extra_mem[i].size);
 		for (pfn = pfn_s; pfn < pfn_e; pfn++)
@@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
 	}
 
 	/* Update kernel mapping, but not for highmem. */
-	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
+	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
 		return;
 
 	if (HYPERVISOR_update_va_mapping((unsigned long)__va(pfn << PAGE_SHIFT),
-- 
2.1.2

Re: [Xen-devel] [PATCH 3/3] xen: use correct type for physical addresses

[Jan Beulich]

>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -140,7 +140,7 @@ static void __init xen_del_extra_mem(u64 start, u64 size)
>  unsigned long __ref xen_chk_extra_mem(unsigned long pfn)
>  {
>  	int i;
> -	unsigned long addr = PFN_PHYS(pfn);
> +	u64 addr = PFN_PHYS(pfn);

Isn't phys_addr_t the type to use here?

> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>  	}
>  
>  	/* Update kernel mapping, but not for highmem. */
> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))

I don't think you really need the cast on the right side - __pa()
should be returning a value of suitable type (and unsigned long
would be sufficient for anything up to and including high_memory).

Jan

Re: [Xen-devel] [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 09/01/15 09:57, Jan Beulich wrote:
>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>> --- a/arch/x86/xen/setup.c
>> +++ b/arch/x86/xen/setup.c
>> @@ -140,7 +140,7 @@ static void __init xen_del_extra_mem(u64 start, u64 size)
>>  unsigned long __ref xen_chk_extra_mem(unsigned long pfn)
>>  {
>>  	int i;
>> -	unsigned long addr = PFN_PHYS(pfn);
>> +	u64 addr = PFN_PHYS(pfn);
> 
> Isn't phys_addr_t the type to use here?

Agreed.

>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>  	}
>>  
>>  	/* Update kernel mapping, but not for highmem. */
>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
> 
> I don't think you really need the cast on the right side - __pa()
> should be returning a value of suitable type (and unsigned long
> would be sufficient for anything up to and including high_memory).

I'd prefer:

    if (pfn >= PFN_DOWN(__pa(high_memory))

David

Re: [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 09/01/15 09:57, Jan Beulich wrote:
>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>> --- a/arch/x86/xen/setup.c
>> +++ b/arch/x86/xen/setup.c
>> @@ -140,7 +140,7 @@ static void __init xen_del_extra_mem(u64 start, u64 size)
>>  unsigned long __ref xen_chk_extra_mem(unsigned long pfn)
>>  {
>>  	int i;
>> -	unsigned long addr = PFN_PHYS(pfn);
>> +	u64 addr = PFN_PHYS(pfn);
> 
> Isn't phys_addr_t the type to use here?

Agreed.

>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>  	}
>>  
>>  	/* Update kernel mapping, but not for highmem. */
>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
> 
> I don't think you really need the cast on the right side - __pa()
> should be returning a value of suitable type (and unsigned long
> would be sufficient for anything up to and including high_memory).

I'd prefer:

    if (pfn >= PFN_DOWN(__pa(high_memory))

David

Re: [Xen-devel] [PATCH 3/3] xen: use correct type for physical addresses

[Jan Beulich]

>>> On 09.01.15 at 13:51, <david.vrabel@citrix.com> wrote:
> On 09/01/15 09:57, Jan Beulich wrote:
>>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>>  	}
>>>  
>>>  	/* Update kernel mapping, but not for highmem. */
>>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
>> 
>> I don't think you really need the cast on the right side - __pa()
>> should be returning a value of suitable type (and unsigned long
>> would be sufficient for anything up to and including high_memory).
> 
> I'd prefer:
> 
>     if (pfn >= PFN_DOWN(__pa(high_memory))

Even better indeed. Just one more point: Strictly speaking
__pa(high_memory) is invalid, as __pa() is defined for low
memory only. Hence perhaps PFN_UP(__pa(high_memory - 1))?

Jan

Re: [PATCH 3/3] xen: use correct type for physical addresses

[Jan Beulich]

>>> On 09.01.15 at 13:51, <david.vrabel@citrix.com> wrote:
> On 09/01/15 09:57, Jan Beulich wrote:
>>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>>  	}
>>>  
>>>  	/* Update kernel mapping, but not for highmem. */
>>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
>> 
>> I don't think you really need the cast on the right side - __pa()
>> should be returning a value of suitable type (and unsigned long
>> would be sufficient for anything up to and including high_memory).
> 
> I'd prefer:
> 
>     if (pfn >= PFN_DOWN(__pa(high_memory))

Even better indeed. Just one more point: Strictly speaking
__pa(high_memory) is invalid, as __pa() is defined for low
memory only. Hence perhaps PFN_UP(__pa(high_memory - 1))?

Jan

Re: [Xen-devel] [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 09/01/15 12:56, Jan Beulich wrote:
>>>> On 09.01.15 at 13:51, <david.vrabel@citrix.com> wrote:
>> On 09/01/15 09:57, Jan Beulich wrote:
>>>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>>>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>>>  	}
>>>>  
>>>>  	/* Update kernel mapping, but not for highmem. */
>>>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>>>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
>>>
>>> I don't think you really need the cast on the right side - __pa()
>>> should be returning a value of suitable type (and unsigned long
>>> would be sufficient for anything up to and including high_memory).
>>
>> I'd prefer:
>>
>>     if (pfn >= PFN_DOWN(__pa(high_memory))
> 
> Even better indeed. Just one more point: Strictly speaking
> __pa(high_memory) is invalid, as __pa() is defined for low
> memory only. Hence perhaps PFN_UP(__pa(high_memory - 1))?

Yes.

David

Re: [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 09/01/15 12:56, Jan Beulich wrote:
>>>> On 09.01.15 at 13:51, <david.vrabel@citrix.com> wrote:
>> On 09/01/15 09:57, Jan Beulich wrote:
>>>>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
>>>> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>>>>  	}
>>>>  
>>>>  	/* Update kernel mapping, but not for highmem. */
>>>> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
>>>> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))
>>>
>>> I don't think you really need the cast on the right side - __pa()
>>> should be returning a value of suitable type (and unsigned long
>>> would be sufficient for anything up to and including high_memory).
>>
>> I'd prefer:
>>
>>     if (pfn >= PFN_DOWN(__pa(high_memory))
> 
> Even better indeed. Just one more point: Strictly speaking
> __pa(high_memory) is invalid, as __pa() is defined for low
> memory only. Hence perhaps PFN_UP(__pa(high_memory - 1))?

Yes.

David

Re: [PATCH 3/3] xen: use correct type for physical addresses

[Jan Beulich]

>>> On 08.01.15 at 18:01, <JGross@suse.com> wrote:
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -140,7 +140,7 @@ static void __init xen_del_extra_mem(u64 start, u64 size)
>  unsigned long __ref xen_chk_extra_mem(unsigned long pfn)
>  {
>  	int i;
> -	unsigned long addr = PFN_PHYS(pfn);
> +	u64 addr = PFN_PHYS(pfn);

Isn't phys_addr_t the type to use here?

> @@ -284,7 +286,7 @@ static void __init xen_update_mem_tables(unsigned long pfn, unsigned long mfn)
>  	}
>  
>  	/* Update kernel mapping, but not for highmem. */
> -	if ((pfn << PAGE_SHIFT) >= __pa(high_memory))
> +	if (PFN_PHYS(pfn) >= (u64)(__pa(high_memory)))

I don't think you really need the cast on the right side - __pa()
should be returning a value of suitable type (and unsigned long
would be sufficient for anything up to and including high_memory).

Jan

Re: [Xen-devel] [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 08/01/15 17:01, Juergen Gross wrote:
> When converting a pfn to a physical address be sure to use 64 bit
> wide types.
> 
> Also avoid invalidating memory for zero sized non-aligned extra
> memory regions.

"Also" means this bit should be in another patch...

> Signed-off-by: Juergen Gross <jgross@suse.com>
> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  arch/x86/xen/setup.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index feb6d86..8839d7b 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -160,6 +160,8 @@ void __init xen_inv_extra_mem(void)
>  	int i;
>  
>  	for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
> +		if (!xen_extra_mem[i].size)
> +			continue;
>  		pfn_s = PFN_DOWN(xen_extra_mem[i].start);
>  		pfn_e = PFN_UP(xen_extra_mem[i].start + xen_extra_mem[i].size);
>  		for (pfn = pfn_s; pfn < pfn_e; pfn++)

i.e., this hunk should be in a separate path.

David

Re: [PATCH 3/3] xen: use correct type for physical addresses

[David Vrabel]

On 08/01/15 17:01, Juergen Gross wrote:
> When converting a pfn to a physical address be sure to use 64 bit
> wide types.
> 
> Also avoid invalidating memory for zero sized non-aligned extra
> memory regions.

"Also" means this bit should be in another patch...

> Signed-off-by: Juergen Gross <jgross@suse.com>
> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  arch/x86/xen/setup.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index feb6d86..8839d7b 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -160,6 +160,8 @@ void __init xen_inv_extra_mem(void)
>  	int i;
>  
>  	for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
> +		if (!xen_extra_mem[i].size)
> +			continue;
>  		pfn_s = PFN_DOWN(xen_extra_mem[i].start);
>  		pfn_e = PFN_UP(xen_extra_mem[i].start + xen_extra_mem[i].size);
>  		for (pfn = pfn_s; pfn < pfn_e; pfn++)

i.e., this hunk should be in a separate path.

David