Re: Issue with shared information page on Xen/ARM 4.17

[Elliott Mitchell <ehem+xen@xxxxxxx>]

On Tue, Oct 03, 2023 at 10:26:28AM +0200, Roger Pau Monné wrote:
> On Thu, Sep 28, 2023 at 07:49:18PM -0700, Elliott Mitchell wrote:
> > I'm trying to get FreeBSD/ARM operational on Xen/ARM.  Current issue is
> > the changes with the handling of the shared information page appear to
> > have broken things for me.
> > 
> > With a pre-4.17 build of Xen/ARM things worked fine.  Yet with a build
> > of the 4.17 release, mapping the shared information page doesn't work.
> 
> This is due to 71320946d5edf AFAICT.

Yes.  While the -EBUSY line may be the one triggering, I'm unsure why.
This seems a fairly reasonable change, so I had no intention of asking
for a revert (which likely would have been rejected).  There is also a
real possibility the -EBUSY comes from elsewhere.  Could also be
71320946d5edf caused a bug elsewhere to be exposed.

> > I'm using Tianocore as the first stage loader.  This continues to work
> > fine.  The build is using tag "edk2-stable202211", commit fff6d81270.
> > While Tianocore does map the shared information page, my reading of their
> > source is that it properly unmaps the page and therefore shouldn't cause
> > trouble.
> > 
> > Notes on the actual call is gpfn was 0x0000000000040072.  This is outside
> > the recommended address range, but my understanding is this is supposed
> > to be okay.
> > 
> > The return code is -16, which is EBUSY.
> > 
> > Ideas?
> 
> I think the issue is that you are mapping the shared info page over a
> guest RAM page, and in order to do that you would fist need to create
> a hole and then map the shared info page.  IOW: the issue is not with
> edk2 not having unmapped the page, but with FreeBSD trying to map the
> shared_info over a RAM page instead of a hole in the p2m.  x86
> behavior is different here, and does allow mapping the shared_info
> page over a RAM gfn (by first removing the backing RAM page on the
> gfn).

An interesting thought.  I thought I'd tried this, but since I didn't see
such in my experiments list.  What I had tried was removing all the pages
in the suggested mapping range.  Yet this failed.

Since this seemed reasonable, I've now tried and found it fails.  The
XENMEM_remove_from_physmap call returns 0.

> Ideally we would like to use an unpopulated gfn, but doing so is not
> trivial right now, as the point where the shared_info page is mapped
> we don't yet have an easy way to account for unpopulated regions.
> 
> My suggestion would be to do something like:
> 
> diff --git a/sys/x86/xen/hvm.c b/sys/x86/xen/hvm.c
> index 4122daeaf600..7251bc69ae15 100644
> --- a/sys/x86/xen/hvm.c
> +++ b/sys/x86/xen/hvm.c
> @@ -194,18 +194,20 @@ xen_hvm_init_shared_info_page(void)
>  {
>       struct xen_add_to_physmap xatp;
>  
> -     if (xen_pv_domain()) {
> -             /*
> -              * Already setup in the PV case, shared_info is passed inside
> -              * of the start_info struct at start of day.
> -              */
> -             return;
> -     }
> -
>       if (HYPERVISOR_shared_info == NULL) {
> +             struct xen_remove_from_physmap rm = {
> +                     .domid = DOMID_SELF,
> +             };
> +             int rc;
> +
>               HYPERVISOR_shared_info = malloc(PAGE_SIZE, M_XENHVM, M_NOWAIT);
>               if (HYPERVISOR_shared_info == NULL)
>                       panic("Unable to allocate Xen shared info page");
> +
> +             rm.gpfn = vtophys(HYPERVISOR_shared_info) >> PAGE_SHIFT;
> +             rc = HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &rm);
> +             if (rc != 0)
> +                     printf("Failed to remove shared_info GFN: %d\n", rc);
>       }
>  
>       xatp.domid = DOMID_SELF;
> 
> But in the long term we should see about initializing the shared_info
> page as part of xenpv_attach().

Didn't even look at the example since I already had ones handy.
Unfortunately this has also failed.  The XENMEM_remove_from_physmap call
returns 0.

If there is a need I can hand off a build of Xen and Tianocore, then let
someone else with better Xen/ARM diagnostics play with it.  Runs about
75MB, someone could place it in a better sharing place if desired.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445