Modern hardware may report more than 48 bits of physical address width.
For paging-external guests our P2M implementation does not cope with
larger values. Telling the guest of more available bits means misleading
it into perhaps trying to actually put some page there (like was e.g.
intermediately done in OVMF for the shared info page).

While there also convert the PV check to a paging-external one (which in
our current code base are synonyms of one another anyway).

Fixes: 5dbd60e16a1f ("x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -401,11 +401,18 @@ static always_inline unsigned int paging
 {
     unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
 
-    if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) &&
-         !is_pv_domain(d) )
+    if ( paging_mode_external(d) )
     {
-        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
-        bits = min(bits, 32U + PAGE_SHIFT);
+        if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) )
+        {
+            /* Shadowed superpages store GFNs in 32-bit page_info fields. */
+            bits = min(bits, 32U + PAGE_SHIFT);
+        }
+        else
+        {
+            /* Both p2m-ept and p2m-pt only support 4-level page tables. */
+            bits = min(bits, 48U);
+        }
     }
 
     return bits;

On 29.09.2021 14:47, Jan Beulich wrote:
> Modern hardware may report more than 48 bits of physical address width.
> For paging-external guests our P2M implementation does not cope with
> larger values. Telling the guest of more available bits means misleading
> it into perhaps trying to actually put some page there (like was e.g.
> intermediately done in OVMF for the shared info page).
> 
> While there also convert the PV check to a paging-external one (which in
> our current code base are synonyms of one another anyway).
> 
> Fixes: 5dbd60e16a1f ("x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Anyone?

Thanks, Jan

> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -401,11 +401,18 @@ static always_inline unsigned int paging
>  {
>      unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>  
> -    if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) &&
> -         !is_pv_domain(d) )
> +    if ( paging_mode_external(d) )
>      {
> -        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> -        bits = min(bits, 32U + PAGE_SHIFT);
> +        if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) )
> +        {
> +            /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> +            bits = min(bits, 32U + PAGE_SHIFT);
> +        }
> +        else
> +        {
> +            /* Both p2m-ept and p2m-pt only support 4-level page tables. */
> +            bits = min(bits, 48U);
> +        }
>      }
>  
>      return bits;
> 
> 

On Wed, Sep 29, 2021 at 02:47:49PM +0200, Jan Beulich wrote:
> Modern hardware may report more than 48 bits of physical address width.
> For paging-external guests our P2M implementation does not cope with
> larger values. Telling the guest of more available bits means misleading
> it into perhaps trying to actually put some page there (like was e.g.
> intermediately done in OVMF for the shared info page).
> 
> While there also convert the PV check to a paging-external one (which in
> our current code base are synonyms of one another anyway).
> 
> Fixes: 5dbd60e16a1f ("x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -401,11 +401,18 @@ static always_inline unsigned int paging
>  {
>      unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>  
> -    if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) &&
> -         !is_pv_domain(d) )
> +    if ( paging_mode_external(d) )
>      {
> -        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> -        bits = min(bits, 32U + PAGE_SHIFT);
> +        if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) )
> +        {
> +            /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> +            bits = min(bits, 32U + PAGE_SHIFT);
> +        }
> +        else
> +        {
> +            /* Both p2m-ept and p2m-pt only support 4-level page tables. */
> +            bits = min(bits, 48U);

It would be nice if there was a way to not have to manually change the
values here if we ever support 5-level page tables for example, but I
don't know of any way.

Thanks, Roger.

On 18.10.2021 15:46, Roger Pau Monné wrote:
> On Wed, Sep 29, 2021 at 02:47:49PM +0200, Jan Beulich wrote:
>> Modern hardware may report more than 48 bits of physical address width.
>> For paging-external guests our P2M implementation does not cope with
>> larger values. Telling the guest of more available bits means misleading
>> it into perhaps trying to actually put some page there (like was e.g.
>> intermediately done in OVMF for the shared info page).
>>
>> While there also convert the PV check to a paging-external one (which in
>> our current code base are synonyms of one another anyway).
>>
>> Fixes: 5dbd60e16a1f ("x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- a/xen/include/asm-x86/paging.h
>> +++ b/xen/include/asm-x86/paging.h
>> @@ -401,11 +401,18 @@ static always_inline unsigned int paging
>>  {
>>      unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
>>  
>> -    if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) &&
>> -         !is_pv_domain(d) )
>> +    if ( paging_mode_external(d) )
>>      {
>> -        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
>> -        bits = min(bits, 32U + PAGE_SHIFT);
>> +        if ( !IS_ENABLED(CONFIG_BIGMEM) && paging_mode_shadow(d) )
>> +        {
>> +            /* Shadowed superpages store GFNs in 32-bit page_info fields. */
>> +            bits = min(bits, 32U + PAGE_SHIFT);
>> +        }
>> +        else
>> +        {
>> +            /* Both p2m-ept and p2m-pt only support 4-level page tables. */
>> +            bits = min(bits, 48U);
> 
> It would be nice if there was a way to not have to manually change the
> values here if we ever support 5-level page tables for example, but I
> don't know of any way.

Well, I have a vague plan, along the lines of "IOMMU: have vendor
code announce supported page sizes": Once the value isn't constant
anymore, vendor code needs to supply it.

Jan