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

[Julien Grall <julien.grall@arm.com>]

(+ 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel