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

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

On 28.07.25 16:21, Jan Beulich wrote:
> On 08.07.2025 08:37, Juergen Gross wrote:
> > --- 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,14 +72,16 @@ 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();
> > +    shinfo_va = map_frame_virt(shinfo_pfn);
> In the PV variant you first check whether you already have a VA. Why is
> that needed there, but not here? (Originally I meant to ask why you don't
> use map_frame_virt() there as well.)
Good catch. It is needed here, otherwise we'll leak the virtual page
during a suspend/resume cycle.

> Talking of map_frame_virt() - I take it its 2nd parameter being named
> "mfn" is kind of stale (pre-dating the addition of PVH support)?
I think it was misnamed back then.

> > @@ -84,9 +89,11 @@ void unmap_shared_info(void)
> >       struct xen_remove_from_physmap xrtp;
> >   
> >       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();
> > +    unmap_frames(shinfo_va, 1);
> Better do this ahead of the hypercall?
Agreed.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature