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 Fri, 11 Oct 2019, Julien Grall wrote:
> 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.

If the issue doesn't trigger on 4.13, then I agree we shouldn't try to
fix it (badly) at this stage.

But we do have a "minus phys_offset" operation in dump_hyp_walk, I don't
follow why we wouldn't see a problem if Xen is loaded at 1MB on arm32.

Xen pa: 0x100000
Xen va: 0x200000
phys_offset: 0xfff00000

test_va: 0x202000
test_va - phys_offset = 0xffffffff00300200. But it should be 0x102000.

Given that the problem is in a BUG_ON maybe we could remove it? Or just
rework the BUG_ON?

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