Re: [Xen-devel] [PATCH] xen: arm: zero EL2 pagetable pages before use

[Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>]

Hi Julien,


On 03/12/2016 10:03 AM, Julien Grall wrote:
> Hi,
>
> On 11/03/2016 20:24, Andrew Cooper wrote:
>> On 11/03/16 13:13, Jan Beulich wrote:
>>>>>> On 11.03.16 at 13:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 11/03/16 11:29, Jan Beulich wrote:
>>>>>>>> On 10.03.16 at 23:00, <shankerd@xxxxxxxxxxxxxx> wrote:
>>>>>> @@ -771,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, 
>>>>>> paddr_t pe)
>>>>>>       nr_second = frametable_size >> SECOND_SHIFT;
>>>>>>       second_base = alloc_boot_pages(nr_second, 1);
>>>>>>       second = mfn_to_virt(second_base);
>>>>>> +    memset(second, 0, nr_second * PAGE_SIZE);
>>>>>>       for ( i = 0; i < nr_second; i++ )
>>>>>>       {
>>>>>>           pte = mfn_to_xen_entry(second_base + i, WRITEALLOC);
>>>>> Along those lines here - use clear_page(), presumably by moving it
>>>>> into the loop.
>>>> This need only initialise the entries which are not filled by the loop,
>>>> which will only be the rounding size up to the next 2M or 32M boundary.
>>>>
>>>> Most of the content of 'second' is explicitly initialised, so zeroing it
>>>> all first is redundant.
>>> Well, I certainly don't know all the details of how this works on
>>> ARM, but the way I remember the original problem description
>>> (sent a few days ago) the problem was with bogus translations
>>> to be visible transiently. Of course all depends on whether the
>>> page tables that are being modified here are live ones, which
>>> I simply don't know.
>>
>> Looking at the code here, second is hooked into the live pagetables
>> immediately after the loop.  Therefore, bogus translations will only be
>> present for the untouched PTEs which make up the alignment space.
>
> The frame table size is always aligned to 2MB/32MB. However, the frame table 
> may not use all the entries in a level 2 page table (which cover 1GB of 
> memory). Those unused entries will be unknown if we don't clear them.
>
> Keeping them unknown is not a problem as long as nobody is trying to access 
> the underlying virtual address.
>

I don't agree keeping a garbage value in PTE is not a problem. The ARMv8 
Architecture
allows to perform speculative data/instruction read accesses from memory (type 
normal)
as along as its PTE valid bit is set.

CPU prefetch logic might access garbage PTEs and cause system panic if VA-PA 
translation
happens to be physical address that is not addressable by system BUS.
 

> In the case of setup_frametable_mappings, Xen is still running with a single 
> processor and the frame_table is not access until after create_mappings is 
> called. The function should nuke all the TLBs at the end, so it looks like to 
> me that zeroed the entries will hide the real problem.
>

Not true, zeroed PTE entries fixing the asynchronous aborts and Serror 
exceptions due to garbage
PTEs.

> Nonetheless, I would invalidate all the entries in the table to avoid 
> polluting the TLBs with bogus entries and get a better crash.
>
> Regards,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project


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