There is no need to use a populated memory page for mapping the shared
info page at that location. Just use an allocated virtual address for
the shared info page. For PVH allocate an unused pfn.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- don't leak virtual page in map_shared_page() (Jan Beulich)
---
 arch/x86/mm.c     |  7 -------
 arch/x86/setup.c  | 15 ++++++++-------
 arch/x86/x86_32.S |  7 +------
 arch/x86/x86_64.S |  7 +------
 hypervisor.c      | 18 ++++++++++++++----
 5 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index fd808884..2ffef976 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -498,7 +498,6 @@ static void build_pagetable(unsigned long *start_pfn, unsigned long *max_pfn)
 /*
  * Mark portion of the address space read only.
  */
-extern struct shared_info shared_info;
 
 struct change_readonly_par {
     unsigned long etext;
@@ -520,12 +519,6 @@ static int change_readonly_func(unsigned long va, unsigned int lvl,
     if ( va + (1UL << ptdata[lvl].shift) > ro->etext )
         return 1;
 
-    if ( va == (unsigned long)&shared_info )
-    {
-        printk("skipped %lx\n", va);
-        return 0;
-    }
-
     newval = ro->readonly ? (*pte & ~_PAGE_RW) : (*pte | _PAGE_RW);
 
 #ifdef CONFIG_PARAVIRT
diff --git a/arch/x86/setup.c b/arch/x86/setup.c
index 299ff8c7..8fd55c51 100644
--- a/arch/x86/setup.c
+++ b/arch/x86/setup.c
@@ -47,8 +47,6 @@ shared_info_t *HYPERVISOR_shared_info;
  */
 char stack[2*STACK_SIZE];
 
-extern char shared_info[PAGE_SIZE];
-
 static inline void fpu_init(void) {
 	asm volatile("fninit");
 }
@@ -76,18 +74,21 @@ static void set_info_ptr(start_info_t *ptr)
 
 #define hpc_init()
 
+static unsigned long shared_info_va;
+
 shared_info_t *map_shared_info(void)
 {
     int rc;
-    unsigned long pa = start_info_ptr->shared_info;
 
-    if ( (rc = HYPERVISOR_update_va_mapping((unsigned long)shared_info,
-                                            __pte(pa | 7), UVMF_INVLPG)) )
+    if ( !shared_info_va )
+        shared_info_va = alloc_virt_kernel(1);
+    rc = map_frame_rw(shared_info_va, PHYS_PFN(start_info_ptr->shared_info));
+    if ( rc )
     {
         printk("Failed to map shared_info!! rc=%d\n", rc);
         do_exit();
     }
-    return (shared_info_t *)shared_info;
+    return (shared_info_t *)shared_info_va;
 }
 
 void unmap_shared_info(void)
@@ -95,7 +96,7 @@ void unmap_shared_info(void)
     int rc;
     pte_t nullpte = { };
 
-    if ( (rc = HYPERVISOR_update_va_mapping((unsigned long)shared_info,
+    if ( (rc = HYPERVISOR_update_va_mapping(shared_info_va,
                                             nullpte, UVMF_INVLPG)) )
     {
         printk("Failed to unmap shared_info page!! rc=%d\n", rc);
diff --git a/arch/x86/x86_32.S b/arch/x86/x86_32.S
index 3de00277..5d891164 100644
--- a/arch/x86/x86_32.S
+++ b/arch/x86/x86_32.S
@@ -36,13 +36,8 @@ _start:
 stack_start:
 	.long stack+(2*__STACK_SIZE), __KERNEL_SS
 
-.globl shared_info, hypercall_page
-        /* Unpleasant -- the PTE that maps this page is actually overwritten */
-        /* to map the real shared-info page! :-)                             */
         .align __PAGE_SIZE
-shared_info:
-        .fill __PAGE_SIZE,1,0
-
+.globl hypercall_page
 hypercall_page:
         .fill __PAGE_SIZE,1,0
 
diff --git a/arch/x86/x86_64.S b/arch/x86/x86_64.S
index 7529c02e..09b93e39 100644
--- a/arch/x86/x86_64.S
+++ b/arch/x86/x86_64.S
@@ -33,13 +33,8 @@ _start:
 stack_start:
         .quad stack+(2*__STACK_SIZE)
 
-.globl shared_info, hypercall_page
-        /* Unpleasant -- the PTE that maps this page is actually overwritten */
-        /* to map the real shared-info page! :-)                             */
         .align __PAGE_SIZE
-shared_info:
-        .fill __PAGE_SIZE,1,0
-
+.globl hypercall_page
 hypercall_page:
         .fill __PAGE_SIZE,1,0
 
diff --git a/hypervisor.c b/hypervisor.c
index 6476d658..213eb49d 100644
--- a/hypervisor.c
+++ b/hypervisor.c
@@ -27,8 +27,10 @@
 
 #include <mini-os/os.h>
 #include <mini-os/lib.h>
+#include <mini-os/e820.h>
 #include <mini-os/hypervisor.h>
 #include <mini-os/events.h>
+#include <mini-os/mm.h>
 #include <xen/memory.h>
 
 EXPORT_SYMBOL(hypercall_page);
@@ -37,7 +39,8 @@ EXPORT_SYMBOL(hypercall_page);
     ((sh)->evtchn_pending[idx] & ~(sh)->evtchn_mask[idx])
 
 #ifndef CONFIG_PARAVIRT
-extern shared_info_t shared_info;
+static unsigned long shinfo_pfn;
+static unsigned long shinfo_va;
 
 int hvm_get_parameter(int idx, uint64_t *value)
 {
@@ -69,24 +72,31 @@ shared_info_t *map_shared_info(void)
 {
     struct xen_add_to_physmap xatp;
 
+    shinfo_pfn = e820_get_reserved_pfns(1);
     xatp.domid = DOMID_SELF;
     xatp.idx = 0;
     xatp.space = XENMAPSPACE_shared_info;
-    xatp.gpfn = virt_to_pfn(&shared_info);
+    xatp.gpfn = shinfo_pfn;
     if ( HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
         BUG();
+    if ( !shinfo_va )
+        shinfo_va = alloc_virt_kernel(1);
+    if ( !shinfo_va || map_frame_rw(shinfo_va, shinfo_pfn) )
+        BUG();
 
-    return &shared_info;
+    return (shared_info_t *)shinfo_va;
 }
 
 void unmap_shared_info(void)
 {
     struct xen_remove_from_physmap xrtp;
 
+    unmap_frames(shinfo_va, 1);
     xrtp.domid = DOMID_SELF;
-    xrtp.gpfn = virt_to_pfn(&shared_info);
+    xrtp.gpfn = shinfo_pfn;
     if ( HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrtp) != 0 )
         BUG();
+    e820_put_reserved_pfns(shinfo_pfn, 1);
 }
 #endif
 
-- 
2.43.0

On 29.07.2025 10:38, Juergen Gross wrote:
> --- a/arch/x86/x86_64.S
> +++ b/arch/x86/x86_64.S
> @@ -33,13 +33,8 @@ _start:
>  stack_start:
>          .quad stack+(2*__STACK_SIZE)
>  
> -.globl shared_info, hypercall_page
> -        /* Unpleasant -- the PTE that maps this page is actually overwritten */
> -        /* to map the real shared-info page! :-)                             */
>          .align __PAGE_SIZE
> -shared_info:
> -        .fill __PAGE_SIZE,1,0
> -
> +.globl hypercall_page

While touching this line, may I suggest to indent this directive to match all
other directives in context? Even if assemblers accept them for most targets,
directives starting in the first column strictly speaking are misplaced.

> --- a/hypervisor.c
> +++ b/hypervisor.c
> @@ -27,8 +27,10 @@
>  
>  #include <mini-os/os.h>
>  #include <mini-os/lib.h>
> +#include <mini-os/e820.h>
>  #include <mini-os/hypervisor.h>
>  #include <mini-os/events.h>
> +#include <mini-os/mm.h>
>  #include <xen/memory.h>
>  
>  EXPORT_SYMBOL(hypercall_page);
> @@ -37,7 +39,8 @@ EXPORT_SYMBOL(hypercall_page);
>      ((sh)->evtchn_pending[idx] & ~(sh)->evtchn_mask[idx])
>  
>  #ifndef CONFIG_PARAVIRT
> -extern shared_info_t shared_info;
> +static unsigned long shinfo_pfn;
> +static unsigned long shinfo_va;
>  
>  int hvm_get_parameter(int idx, uint64_t *value)
>  {
> @@ -69,24 +72,31 @@ shared_info_t *map_shared_info(void)
>  {
>      struct xen_add_to_physmap xatp;
>  
> +    shinfo_pfn = e820_get_reserved_pfns(1);
>      xatp.domid = DOMID_SELF;
>      xatp.idx = 0;
>      xatp.space = XENMAPSPACE_shared_info;
> -    xatp.gpfn = virt_to_pfn(&shared_info);
> +    xatp.gpfn = shinfo_pfn;
>      if ( HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>          BUG();
> +    if ( !shinfo_va )
> +        shinfo_va = alloc_virt_kernel(1);
> +    if ( !shinfo_va || map_frame_rw(shinfo_va, shinfo_pfn) )
> +        BUG();

Now there's a new asymmetry: Here you check whether alloc_virt_kernel()
(appears to have) failed, whereas in the PV variant you don't. And it's
really only "appears to", as the function won't return 0 in the failure
case, afaics. I therefore think that extra condition simply wants
dropping here. Then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

As for the other patch, happy to make both adjustments while committing.
As long as you agree, of course.

Jan

On 29.07.25 11:00, Jan Beulich wrote:
> On 29.07.2025 10:38, Juergen Gross wrote:
>> --- a/arch/x86/x86_64.S
>> +++ b/arch/x86/x86_64.S
>> @@ -33,13 +33,8 @@ _start:
>>   stack_start:
>>           .quad stack+(2*__STACK_SIZE)
>>   
>> -.globl shared_info, hypercall_page
>> -        /* Unpleasant -- the PTE that maps this page is actually overwritten */
>> -        /* to map the real shared-info page! :-)                             */
>>           .align __PAGE_SIZE
>> -shared_info:
>> -        .fill __PAGE_SIZE,1,0
>> -
>> +.globl hypercall_page
> 
> While touching this line, may I suggest to indent this directive to match all
> other directives in context? Even if assemblers accept them for most targets,
> directives starting in the first column strictly speaking are misplaced.
> 
>> --- a/hypervisor.c
>> +++ b/hypervisor.c
>> @@ -27,8 +27,10 @@
>>   
>>   #include <mini-os/os.h>
>>   #include <mini-os/lib.h>
>> +#include <mini-os/e820.h>
>>   #include <mini-os/hypervisor.h>
>>   #include <mini-os/events.h>
>> +#include <mini-os/mm.h>
>>   #include <xen/memory.h>
>>   
>>   EXPORT_SYMBOL(hypercall_page);
>> @@ -37,7 +39,8 @@ EXPORT_SYMBOL(hypercall_page);
>>       ((sh)->evtchn_pending[idx] & ~(sh)->evtchn_mask[idx])
>>   
>>   #ifndef CONFIG_PARAVIRT
>> -extern shared_info_t shared_info;
>> +static unsigned long shinfo_pfn;
>> +static unsigned long shinfo_va;
>>   
>>   int hvm_get_parameter(int idx, uint64_t *value)
>>   {
>> @@ -69,24 +72,31 @@ shared_info_t *map_shared_info(void)
>>   {
>>       struct xen_add_to_physmap xatp;
>>   
>> +    shinfo_pfn = e820_get_reserved_pfns(1);
>>       xatp.domid = DOMID_SELF;
>>       xatp.idx = 0;
>>       xatp.space = XENMAPSPACE_shared_info;
>> -    xatp.gpfn = virt_to_pfn(&shared_info);
>> +    xatp.gpfn = shinfo_pfn;
>>       if ( HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>>           BUG();
>> +    if ( !shinfo_va )
>> +        shinfo_va = alloc_virt_kernel(1);
>> +    if ( !shinfo_va || map_frame_rw(shinfo_va, shinfo_pfn) )
>> +        BUG();
> 
> Now there's a new asymmetry: Here you check whether alloc_virt_kernel()
> (appears to have) failed, whereas in the PV variant you don't. And it's
> really only "appears to", as the function won't return 0 in the failure
> case, afaics. I therefore think that extra condition simply wants
> dropping here. Then

Oh, right. First I had

if ( !shinfo_va )
     shinfo_va = map_frame_virt(shinfo_pfn);
else
     ret = map_frame_rw(shinfo_va, shinfo_pfn);
if ( ret || !shinfo_va )
     BUG();

which I then simplified to above sequence, but without dropping the test for
shinfo_va being not 0.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> As for the other patch, happy to make both adjustments while committing.
> As long as you agree, of course.

Yes, I agree, thanks.


Juergen