[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



(+ Juergen)

Hi Stefano,

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. :(

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?

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.