arm: Boot allocator fails with multi node memory

arm: Boot allocator fails with multi node memory

[Vijay Kilari]

Hi,

    With ACPI numa support, I came across this panic from init_node_heap() when
initializing for node1 which is called from end_boot_allocator().

(XEN) Walking Hypervisor VA 0x847ffffffffe on CPU0 via TTBR 0x00000000ffcf8000
(XEN) 0TH[0x108] = 0x0000000000000000
(XEN) PAR: 0000000000000809: Translation fault stage 1 (level invalid)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Error during Hypervisor-to-physical address translation
(XEN) ****************************************

After looking at closely, I see this translation fault is coming from
below code.
When there is call (virt_to_mfn(eva - 1) where eva is  DIRECTMAP_VIRT_END.
This virt_to_mfn(eva-1) is not called for first node initialization.
In case of multinode it fails when init_node_heap() is called for
second node.

I observed that virt_to_mfn(eva-1) always fails if we call from any part of the
code, meaning dump_hyp_walk(eva) always prints as below, which indicates
this VA is not mapped.

Question: Why this address is not mapped?. If mapped where this va is mapped?.

(XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR 0x00000000ffcf8000
(XEN) 0TH[0x108] = 0x0000000000000000


static unsigned long init_node_heap(int node, unsigned long mfn,
                                    unsigned long nr, bool_t *use_tail, int d)
{
#ifdef DIRECTMAP_VIRT_END
    unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
#endif


....

    else if ( nr >= needed &&
              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
FAILS here
              (!xenheap_bits ||
               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
    {
        _heap[node] = mfn_to_virt(mfn);
        avail[node] = mfn_to_virt(mfn + needed - 1) +
                      PAGE_SIZE - sizeof(**avail) * NR_ZONES;
        *use_tail = 0;
    }

...
}

Regards
Vijay

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Jan Beulich]

>>> On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
> Question: Why this address is not mapped?. If mapped where this va is 
> mapped?.

Well, I think this is the wrong question to ask. Why would it be mapped
if there's no memory there?

> (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR 
> 0x00000000ffcf8000
> (XEN) 0TH[0x108] = 0x0000000000000000
> 
> 
> static unsigned long init_node_heap(int node, unsigned long mfn,
>                                     unsigned long nr, bool_t *use_tail, int d)
> {
> #ifdef DIRECTMAP_VIRT_END
>     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
> #endif
> 
> 
> ....
> 
>     else if ( nr >= needed &&
>               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
> FAILS here

The assumption here is that a virtual address inside the direct map
can always be translated, and from your report I'm gaining the
understanding that this is simply not true on ARM (but the code
here pre-dates ARM iirc). If that assumption doesn't hold (and
cannot be made so), apart from adjusting the code here there may
need to be a full audit of common code to see whether there are
any other such uses.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Julien Grall]

Hi Jan,

On 09/01/17 08:40, Jan Beulich wrote:
>>>> On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
>> Question: Why this address is not mapped?. If mapped where this va is
>> mapped?.
>
> Well, I think this is the wrong question to ask. Why would it be mapped
> if there's no memory there?
>
>> (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR
>> 0x00000000ffcf8000
>> (XEN) 0TH[0x108] = 0x0000000000000000
>>
>>
>> static unsigned long init_node_heap(int node, unsigned long mfn,
>>                                     unsigned long nr, bool_t *use_tail, int d)
>> {
>> #ifdef DIRECTMAP_VIRT_END
>>     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>> #endif
>>
>>
>> ....
>>
>>     else if ( nr >= needed &&
>>               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
>> FAILS here
>
> The assumption here is that a virtual address inside the direct map
> can always be translated, and from your report I'm gaining the
> understanding that this is simply not true on ARM (but the code
> here pre-dates ARM iirc). If that assumption doesn't hold (and
> cannot be made so), apart from adjusting the code here there may
> need to be a full audit of common code to see whether there are\
> any other such uses.

On ARM, the function virt_to_mfn uses the hardware to do the address 
translation. So if the virtual address is not mapped, it will fail.

This is different from the assumption made by the code and x86
behavior. I'd still like to keep the current behavior on ARM as it is 
very handy to directly get debug information when the virtual address is 
not mapped. Stefano, do you have any opinions?

Looking at the code pointed out by Vijay. If I understand correctly, 
virt_to_mfn(eva) is used to find out the maximum MFN that can be mapped.
On ARM64, all the memory is direct mapped so far, so this check will 
always be false. This might not be true in the future.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Stefano Stabellini]

On Mon, 16 Jan 2017, Julien Grall wrote:
> Hi Jan,
> 
> On 09/01/17 08:40, Jan Beulich wrote:
> > > > > On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
> > > Question: Why this address is not mapped?. If mapped where this va is
> > > mapped?.
> > 
> > Well, I think this is the wrong question to ask. Why would it be mapped
> > if there's no memory there?
> > 
> > > (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR
> > > 0x00000000ffcf8000
> > > (XEN) 0TH[0x108] = 0x0000000000000000
> > > 
> > > 
> > > static unsigned long init_node_heap(int node, unsigned long mfn,
> > >                                     unsigned long nr, bool_t *use_tail,
> > > int d)
> > > {
> > > #ifdef DIRECTMAP_VIRT_END
> > >     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
> > > #endif
> > > 
> > > 
> > > ....
> > > 
> > >     else if ( nr >= needed &&
> > >               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
> > > FAILS here
> > 
> > The assumption here is that a virtual address inside the direct map
> > can always be translated, and from your report I'm gaining the
> > understanding that this is simply not true on ARM (but the code
> > here pre-dates ARM iirc). If that assumption doesn't hold (and
> > cannot be made so), apart from adjusting the code here there may
> > need to be a full audit of common code to see whether there are\
> > any other such uses.
> 
> On ARM, the function virt_to_mfn uses the hardware to do the address
> translation. So if the virtual address is not mapped, it will fail.
> 
> This is different from the assumption made by the code and x86
> behavior. I'd still like to keep the current behavior on ARM as it is very
> handy to directly get debug information when the virtual address is not
> mapped. Stefano, do you have any opinions?

I think it is great to use the hardware to implement virt_to_mfn in the
general case, for both performance and simplicity. However, it is
important not to diverge too much from x86 to avoid this class of
problems in the future. In other words, the semantics of virt_to_mfn
must be the same on x86 and ARM. We can either:

1) change x86 to match ARM
This could be as simple as adding a check, only for debug builds, in the
x86 implementation of virt_to_mfn to make sure that the virtual address
passed is mapped or mappable, throw an error if it is not.

2) change ARM to match x86
Add a special case in the arm64 implementation of virt_to_mfn to check
if the address is in the directmap region and translate it directly,
without ATS1HR and friends, in that case.

Either way is fine by me.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Jan Beulich]

>>> On 16.01.17 at 19:46, <julien.grall@arm.com> wrote:
> On 09/01/17 08:40, Jan Beulich wrote:
>>>>> On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
>>> Question: Why this address is not mapped?. If mapped where this va is
>>> mapped?.
>>
>> Well, I think this is the wrong question to ask. Why would it be mapped
>> if there's no memory there?
>>
>>> (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR
>>> 0x00000000ffcf8000
>>> (XEN) 0TH[0x108] = 0x0000000000000000
>>>
>>>
>>> static unsigned long init_node_heap(int node, unsigned long mfn,
>>>                                     unsigned long nr, bool_t *use_tail, int d)
>>> {
>>> #ifdef DIRECTMAP_VIRT_END
>>>     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>>> #endif
>>>
>>>
>>> ....
>>>
>>>     else if ( nr >= needed &&
>>>               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
>>> FAILS here
>>
>> The assumption here is that a virtual address inside the direct map
>> can always be translated, and from your report I'm gaining the
>> understanding that this is simply not true on ARM (but the code
>> here pre-dates ARM iirc). If that assumption doesn't hold (and
>> cannot be made so), apart from adjusting the code here there may
>> need to be a full audit of common code to see whether there are\
>> any other such uses.
> 
> On ARM, the function virt_to_mfn uses the hardware to do the address 
> translation. So if the virtual address is not mapped, it will fail.
> 
> This is different from the assumption made by the code and x86
> behavior. I'd still like to keep the current behavior on ARM as it is 
> very handy to directly get debug information when the virtual address is 
> not mapped. Stefano, do you have any opinions?
> 
> Looking at the code pointed out by Vijay. If I understand correctly, 
> virt_to_mfn(eva) is used to find out the maximum MFN that can be mapped.

Right.

> On ARM64, all the memory is direct mapped so far, so this check will 
> always be false.

DYM "will always be true"?

> This might not be true in the future.

Right. But regardless of that we clearly need another arch_*()
abstraction here, which (for now) returns constant true on ARM.

Or wait - why does ARM have DIRECTMAP_VIRT_END defined? I
can't see any use of it outside of page_alloc.c, and there all the
problematic code would be compiled out if the symbol wasn't
defined.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Julien Grall]

Hi Stefano,

On 16/01/17 19:59, Stefano Stabellini wrote:
> On Mon, 16 Jan 2017, Julien Grall wrote:
>> On 09/01/17 08:40, Jan Beulich wrote:
>>>>>> On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
>>>> Question: Why this address is not mapped?. If mapped where this va is
>>>> mapped?.
>>>
>>> Well, I think this is the wrong question to ask. Why would it be mapped
>>> if there's no memory there?
>>>
>>>> (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR
>>>> 0x00000000ffcf8000
>>>> (XEN) 0TH[0x108] = 0x0000000000000000
>>>>
>>>>
>>>> static unsigned long init_node_heap(int node, unsigned long mfn,
>>>>                                     unsigned long nr, bool_t *use_tail,
>>>> int d)
>>>> {
>>>> #ifdef DIRECTMAP_VIRT_END
>>>>     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>>>> #endif
>>>>
>>>>
>>>> ....
>>>>
>>>>     else if ( nr >= needed &&
>>>>               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
>>>> FAILS here
>>>
>>> The assumption here is that a virtual address inside the direct map
>>> can always be translated, and from your report I'm gaining the
>>> understanding that this is simply not true on ARM (but the code
>>> here pre-dates ARM iirc). If that assumption doesn't hold (and
>>> cannot be made so), apart from adjusting the code here there may
>>> need to be a full audit of common code to see whether there are\
>>> any other such uses.
>>
>> On ARM, the function virt_to_mfn uses the hardware to do the address
>> translation. So if the virtual address is not mapped, it will fail.
>>
>> This is different from the assumption made by the code and x86
>> behavior. I'd still like to keep the current behavior on ARM as it is very
>> handy to directly get debug information when the virtual address is not
>> mapped. Stefano, do you have any opinions?
>
> I think it is great to use the hardware to implement virt_to_mfn in the
> general case, for both performance and simplicity.

To be honest, I don't think implementing virt_to_mfn using hardware 
instruction will result to a faster translation. If you look at the 
virt_to_mfn implementation on x86, it is a few instructions because it 
only cares about the direct mapping and xen binary address.

In the case of ARM, virt_to_mfn is able to translate any addresses (such 
as vmap one).

 > However, it is
> important not to diverge too much from x86 to avoid this class of
> problems in the future. In other words, the semantics of virt_to_mfn
> must be the same on x86 and ARM. We can either:
>
> 1) change x86 to match ARM
> This could be as simple as adding a check, only for debug builds, in the
> x86 implementation of virt_to_mfn to make sure that the virtual address
> passed is mapped or mappable, throw an error if it is not.
>
> 2) change ARM to match x86
> Add a special case in the arm64 implementation of virt_to_mfn to check
> if the address is in the directmap region and translate it directly,
> without ATS1HR and friends, in that case.

Both directmap and xen virtual address would need to have a specific 
case. I am not a big fan of adding specific case because I think it is 
just a workaround to make the code happy. After all the MFN returned is 
just theoretical and should only be used to do checking. You will also 
lose the ability to catch as soon as possible a problem with page table.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: arm: Boot allocator fails with multi node memory

[Julien Grall]

Hi Jan,

On 17/01/17 09:09, Jan Beulich wrote:
>>>> On 16.01.17 at 19:46, <julien.grall@arm.com> wrote:
>> On 09/01/17 08:40, Jan Beulich wrote:
>>>>>> On 07.01.17 at 07:05, <vijay.kilari@gmail.com> wrote:
>>>> Question: Why this address is not mapped?. If mapped where this va is
>>>> mapped?.
>>>
>>> Well, I think this is the wrong question to ask. Why would it be mapped
>>> if there's no memory there?
>>>
>>>> (XEN) Walking Hypervisor VA 0x847fffffffff on CPU0 via TTBR
>>>> 0x00000000ffcf8000
>>>> (XEN) 0TH[0x108] = 0x0000000000000000
>>>>
>>>>
>>>> static unsigned long init_node_heap(int node, unsigned long mfn,
>>>>                                     unsigned long nr, bool_t *use_tail, int d)
>>>> {
>>>> #ifdef DIRECTMAP_VIRT_END
>>>>     unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>>>> #endif
>>>>
>>>>
>>>> ....
>>>>
>>>>     else if ( nr >= needed &&
>>>>               (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&  // <===
>>>> FAILS here
>>>
>>> The assumption here is that a virtual address inside the direct map
>>> can always be translated, and from your report I'm gaining the
>>> understanding that this is simply not true on ARM (but the code
>>> here pre-dates ARM iirc). If that assumption doesn't hold (and
>>> cannot be made so), apart from adjusting the code here there may
>>> need to be a full audit of common code to see whether there are\
>>> any other such uses.
>>
>> On ARM, the function virt_to_mfn uses the hardware to do the address
>> translation. So if the virtual address is not mapped, it will fail.
>>
>> This is different from the assumption made by the code and x86
>> behavior. I'd still like to keep the current behavior on ARM as it is
>> very handy to directly get debug information when the virtual address is
>> not mapped. Stefano, do you have any opinions?
>>
>> Looking at the code pointed out by Vijay. If I understand correctly,
>> virt_to_mfn(eva) is used to find out the maximum MFN that can be mapped.
>
> Right.
>
>> On ARM64, all the memory is direct mapped so far, so this check will
>> always be false.
>
> DYM "will always be true"?

Yes sorry.

>
>> This might not be true in the future.
>
> Right. But regardless of that we clearly need another arch_*()
> abstraction here, which (for now) returns constant true on ARM.
>
> Or wait - why does ARM have DIRECTMAP_VIRT_END defined? I
> can't see any use of it outside of page_alloc.c, and there all the
> problematic code would be compiled out if the symbol wasn't
> defined.

DIRECTMAP_VIRT_END is defined because all the virtual region defined are 
bound using 2 defines (*_VIRT_START and *_VIRT_END). Removing 
DIRECTMAP_VIRT_END would be the worst solution because it only defers 
the problem until we decide to not map all the memory in Xen for ARM64.

So I would prefer to see an arch_* helper here.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel