x86/pv: Unhide writes to d->arch.hv_compat_vstart

[PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Andrew Cooper 1 day, 13 hours ago

The variable is never 0, but because the writes into it are hidden behind the
HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
to be 0.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Grygorii Strashko <grygorii_strashko@epam.com>
---
 xen/arch/x86/domain.c        | 2 +-
 xen/arch/x86/pv/dom0_build.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bd75d044a01b..d33a42c8824c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -891,7 +891,7 @@ int arch_domain_create(struct domain *d,
     d->arch.emulation_flags = emflags;
 
 #ifdef CONFIG_PV32
-    HYPERVISOR_COMPAT_VIRT_START(d) =
+    d->arch.hv_compat_vstart =
         is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
 #endif
 
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 21158ce1812e..fed03dc15dcf 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -521,7 +521,7 @@ static int __init dom0_construct(const struct boot_domain *bd)
                 printk("Dom0 expects too high a hypervisor start address\n");
                 return -ERANGE;
             }
-            HYPERVISOR_COMPAT_VIRT_START(d) =
+            d->arch.hv_compat_vstart =
                 max_t(unsigned int, m2p_compat_vstart, value);
         }
 

base-commit: 63137a87311e1081bce0c5a4364492b4fc728bfb
-- 
2.39.5

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Grygorii Strashko 1 day, 10 hours ago


On 09.12.25 17:57, Andrew Cooper wrote:
> The variable is never 0, but because the writes into it are hidden behind the
> HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
> to be 0.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Grygorii Strashko <grygorii_strashko@epam.com>
> ---
>   xen/arch/x86/domain.c        | 2 +-
>   xen/arch/x86/pv/dom0_build.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index bd75d044a01b..d33a42c8824c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -891,7 +891,7 @@ int arch_domain_create(struct domain *d,
>       d->arch.emulation_flags = emflags;
>   
>   #ifdef CONFIG_PV32
> -    HYPERVISOR_COMPAT_VIRT_START(d) =
> +    d->arch.hv_compat_vstart =
>           is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>   #endif
>   
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 21158ce1812e..fed03dc15dcf 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -521,7 +521,7 @@ static int __init dom0_construct(const struct boot_domain *bd)
>                   printk("Dom0 expects too high a hypervisor start address\n");
>                   return -ERANGE;
>               }
> -            HYPERVISOR_COMPAT_VIRT_START(d) =
> +            d->arch.hv_compat_vstart =
>                   max_t(unsigned int, m2p_compat_vstart, value);
>           }
>   
> 

Thank you.
Reviewed-by: Grygorii Strashko <grygorii_strashko@epam.com>

-- 
Best regards,
-grygorii

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Jan Beulich 1 day, 12 hours ago

On 09.12.2025 16:57, Andrew Cooper wrote:
> The variable is never 0, but because the writes into it are hidden behind the
> HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
> to be 0.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I don't kind it being this way or the original one, so
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Andrew Cooper 1 day, 12 hours ago

On 09/12/2025 4:41 pm, Jan Beulich wrote:
> On 09.12.2025 16:57, Andrew Cooper wrote:
>> The variable is never 0, but because the writes into it are hidden behind the
>> HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
>> to be 0.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I don't kind it being this way or the original one, so
> Acked-by: Jan Beulich <jbeulich@suse.com>

Do you mean s/kind/mind/ ?

Thanks,

~Andrew

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Jan Beulich 21 hours ago

On 09.12.2025 18:11, Andrew Cooper wrote:
> On 09/12/2025 4:41 pm, Jan Beulich wrote:
>> On 09.12.2025 16:57, Andrew Cooper wrote:
>>> The variable is never 0, but because the writes into it are hidden behind the
>>> HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
>>> to be 0.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> I don't kind it being this way or the original one, so
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Do you mean s/kind/mind/ ?

Of course, sorry; must have hit the neighboring key.

Jan

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Grygorii Strashko 1 day, 13 hours ago

Hi Andrew,

On 09.12.25 17:57, Andrew Cooper wrote:
> The variable is never 0, but because the writes into it are hidden behind the
> HYPERVISOR_COMPAT_VIRT_START() macro, it does a good impression of appearing
> to be 0.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Grygorii Strashko <grygorii_strashko@epam.com>
> ---
>   xen/arch/x86/domain.c        | 2 +-
>   xen/arch/x86/pv/dom0_build.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index bd75d044a01b..d33a42c8824c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -891,7 +891,7 @@ int arch_domain_create(struct domain *d,
>       d->arch.emulation_flags = emflags;
>   
>   #ifdef CONFIG_PV32
> -    HYPERVISOR_COMPAT_VIRT_START(d) =
> +    d->arch.hv_compat_vstart =
>           is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>   #endif

Any chances it can be moved in pv_domain_initialise()?

>   
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 21158ce1812e..fed03dc15dcf 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -521,7 +521,7 @@ static int __init dom0_construct(const struct boot_domain *bd)
>                   printk("Dom0 expects too high a hypervisor start address\n");
>                   return -ERANGE;
>               }
> -            HYPERVISOR_COMPAT_VIRT_START(d) =
> +            d->arch.hv_compat_vstart =
>                   max_t(unsigned int, m2p_compat_vstart, value);
>           }


Thank you.

-- 
Best regards,
-grygorii

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Jan Beulich 1 day, 12 hours ago

On 09.12.2025 17:06, Grygorii Strashko wrote:
> On 09.12.25 17:57, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -891,7 +891,7 @@ int arch_domain_create(struct domain *d,
>>       d->arch.emulation_flags = emflags;
>>   
>>   #ifdef CONFIG_PV32
>> -    HYPERVISOR_COMPAT_VIRT_START(d) =
>> +    d->arch.hv_compat_vstart =
>>           is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>>   #endif
> 
> Any chances it can be moved in pv_domain_initialise()?

Probably, but one thing at a time? The field itself would also want to move
from struct arch_domain to struct pv_domain, I think.

Jan

Re: [PATCH] x86/pv: Unhide writes to d->arch.hv_compat_vstart

Posted by Andrew Cooper 1 day, 11 hours ago

On 09/12/2025 4:42 pm, Jan Beulich wrote:
> On 09.12.2025 17:06, Grygorii Strashko wrote:
>> On 09.12.25 17:57, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -891,7 +891,7 @@ int arch_domain_create(struct domain *d,
>>>       d->arch.emulation_flags = emflags;
>>>   
>>>   #ifdef CONFIG_PV32
>>> -    HYPERVISOR_COMPAT_VIRT_START(d) =
>>> +    d->arch.hv_compat_vstart =
>>>           is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>>>   #endif
>> Any chances it can be moved in pv_domain_initialise()?
> Probably, but one thing at a time? The field itself would also want to move
> from struct arch_domain to struct pv_domain, I think.

Agreed to one thing at a time.

The value itself is a total mess.  Storage exists based on CONFIG_PV32,
with 0 yielded in !PV32 builds.

Yet, in CONFIG_PV32 builds, it has the value 0xF5800000 for PV domains,
and 0xFFFFFFFF for HVM domains. 

This is nonsense, causing e.g. COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT() to
return answers at opposite ends of the pagetable for non-PV32 VMs
depending on CONFIG_PV32.  I think this all works because the logic is
behind suitable is_pv32_$FOO() checks, but it's far from clear.

It is only a PV32 dom0 which can have this set to anything besides
0xF5800000, so the "correct" thing to do would be to leave it 0 in
domain create, and set it to 0xF5800000 in switch_compat(), along with
the custom setup in dom0_construct().

But, lets get the d->arch.physaddr_bitsize adjustments sorted first
before conflicting those with this change.

~Andrew