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 Tue, 8 Oct 2019, Julien Grall wrote:
> On 10/8/19 1:18 AM, Stefano Stabellini wrote:
> > 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.
> 
> The check is not strictly necessary.
> 
> > 
> > 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?
> 
> Except that phys_offset is not defined correctly, so your check below will
> break some setup :(. Let's take the following example:
> 
>    Xen is loaded at PA 0x100000
> 
> The boot offset is computed using 32-bit address (see head.S):
>     PA - VA = 0x100000 - 0x200000
>             = 0xfff00000
> 
> This value will be passed to C code as an unsigned long. But then we will
> store it in a uint64_t/paddr_t (see phys_offset which is set in
> setup_page_tables). Because this is a conversion from unsigned to unsigned,
> the "sign bit" will not be propagated.
>
> This means that phys_offset will be equal to 0xfff00000 and not
> 0xfffffffffff00000!
> 
> Therefore if we try to convert 0x100000 (where Xen has been loaded) back to
> its VA, the resulting value will be 0xffffffff00200100.
>
> Looking at the code, I think pte_of_xenaddr() has also the exact problem. :(

One way to fix it would be to change phys_offset to register_t (or just
declare it as unsigned long on arm32 and unsigned long long on arm64):

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index be23acfe26..c7ead39654 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -170,7 +170,7 @@ static DEFINE_PAGE_TABLE(xen_xenmap);
 /* Non-boot CPUs use this to find the correct pagetables. */
 uint64_t init_ttbr;
 
-static paddr_t phys_offset;
+static register_t phys_offset;
 
 /* Limits of the Xen heap */
 mfn_t xenheap_mfn_start __read_mostly = INVALID_MFN_INITIALIZER;

It would work OK for virtual to physical conversions. Physical to
virtual is more challenging because physical could go up to 40bits.
Fortunately, it doesn't look like we are using phys_offset to do many
phys-to-virt conversions. The only example is the one in this patch,
and dump_hyp_walk.


> I guess nobody tried to load Xen that low in memory on Arm32? But that's going
> to be definitely an issues with the memory rework I have in mind.
> 
> I have some other important work to finish for Xen 4.13. So I am thinking to
> defer the problem I mention above for post Xen 4.13. Although, the GRUB issues
> would still need to be fix. Any opinions?

I think for Xen 4.13 I would like to get in your patch to fix GRUB
booting, with a little bit more attention to integer issues. Something
like the following:

    paddr_t pa_end = _end + phys_offset;
    paddr_t pa_start = _start + phys_offset;
    paddr_t pa = mfn_to_maddr(mfn);

    if ( pa < pa_end && pa >= pa_start )
        return (lpae_t *)(vaddr_t)(pa - pa_start + XEN_VIRT_ADDR);

I think it should work if phys_offset is register_t. Or we could have a
check on pa >= 4G, something like:

    paddr_t pa = (register_t)mfn_to_maddr(mfn) - phys_offset;

    if ( mfn_to_maddr(mfn) <= UINT32_MAX &&
         pa < _end &&
         is_kernel((vaddr_t)pa) )
        return (lpae_t *)(vaddr_t)pa;


> Note that this is also more reasons to limit the use of "MA - phys_offset". So
> the mess is kept to boot code.
> 
> > 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