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>]

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