Re: [MINI-OS PATCH v2 2/2] x86: don't use a memory page for mapping the shared info page

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxx>
>
> As for the other patch, happy to make both adjustments while committing.
> As long as you agree, of course.
Yes, I agree, thanks.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature