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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 08/10/2019 23:24, Stefano Stabellini wrote:
> 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):
sizeof (unsigned long) = 32 (resp. 64) on Arm32 (resp. arm64). This is what we 
already rely on for boot_phys_offset (see setup_pagetables). So I am not sure 
why phys_offset needs to be defined differently.
An alternative is to use vaddr_t.

> 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. 
This is because the addition will be done using vaddr_t (aka unsigned long) and 
then promote to 64-bit, right?
Although, I am not too happy with making this assumption. I would much prefer if 
we keep phys_offset a paddr_t and make sure sign-bit is propagated. So this 
would cater user doing the conversion either using 64-bit or 32-bit.
> 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;
This could be a good idea. Thinking a bit more, we don't need to use phys_offset 
here. We could use hardware translation directly (via virt_to_maddr).
So we would keep phys_offset as it is today for Xen 4.13. For next, we can queue 
a patch to completely drop it. Let me have a think and try to come up with 
something.
>      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;
There is nothing preventing Xen to be loaded above 4GB on Arm64. For instance on 
AMD Seattle, Xen is loaded after at 512GB.
> > 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;
Cheers,

--
Julien Grall

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