Re: [Xen-devel] [[PATCH for-4.13]] xen/arm: mm: Allow generic xen page-tables helpers to be called early

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 7 Oct 2019, Julien Grall wrote:
> Hi,
> 
> On 03/10/2019 02:02, Stefano Stabellini wrote:
> > On Fri, 20 Sep 2019, Julien Grall wrote:
> >> That's not correct. alloc_boot_pages() is actually here to allow dynamic
> >> allocation before the memory subsystem (and therefore the runtime 
> >> allocator)
> >> is initialized.
> > 
> > Let me change the question then: is the system_state ==
> > SYS_STATE_early_boot check strictly necessary? It looks like it is not:
> > the patch would work even if it was just:
> 
> I had a few thoughts about it. On Arm32, this only really works for 
> 32-bits machine address (it can go up to 40-bits). I haven't really 
> fully investigated what could go wrong, but it would be best to keep it 
> only for early boot.
> 
> Also, I don't really want to rely on this "workaround" after boot. Maybe 
> we would want to keep them unmapped in the future.

Yes, no problems, we agree on that. I am not asking in regards to the
check system_state == SYS_STATE_early_boot with the goal of asking you
to get rid of it. I am fine with keeping the check. (Maybe we want to add
an `unlikely()' around the check.)

I am trying to understand whether the code actually relies on
system_state == SYS_STATE_early_boot, and, if so, why. The goal is to
make sure that if there are some limitations that they are documented,
or just to double-check that there are no limitations.

In regards to your comment about only working for 32-bit addresses on
Arm32, you have a point. At least we should be careful with the mfn to
vaddr conversion because mfn_to_maddr returns a paddr_t which is 64-bit
and vaddr_t is 32-bit. I imagine that theoretically, even with
system_state == SYS_STATE_early_boot, it could get truncated with the
wrong combination of mfn and phys_offset.

If nothing else, maybe we should add a truncation check for safety?
Something like the following but that ideally would be applicable to
arm64 too without having to add an #ifdef:

    paddr_t pa = mfn_to_maddr(mfn) - phys_offset;

    if ( pa < _end && is_kernel((vaddr_t)pa) )
        return (lpae_t *)(vaddr_t)pa;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel