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,

On 11/10/2019 01:32, Stefano Stabellini wrote:
> On Thu, 10 Oct 2019, Julien Grall wrote:
> > 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.
> Yes, I meant like vaddr_t or just "unsigned long" like boot_phys_offset.
> Even with your latest patch
> (https://marc.info/?l=xen-devel&m=157073004830894) which I like as a way
> to solve the original GRUB booting issue, it looks like we also need to
> change phys_addr to unsigned long to fix other arm32 problems.
>
> Are you going to send the patch for that too?
I am looking at dropping phys_offset completely.

Regarding Xen 4.13, the issue would only happen if you place Xen below 2MB on 
Arm32. Yet, I believe this works fine because of side effect (MFN can only be 
32-bit).
This is not pretty, but I don't view this as critical to fix for Xen 4.13. So I 
am thinking to defer this post 4.13.
Cheers,

--
Julien Grall

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