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

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

On 14/03/16 17:18, Shanker Donthineni wrote:
> Hi Julien,

Hi Shanker,

>
> 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@citrix.com> wrote:
>>>>> On 11/03/16 11:29, Jan Beulich wrote:
>>>>>>>>> On 10.03.16 at 23:00, <shankerd@codeaurora.org> 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.

When you quote the spec, can you give the section/version? It helps the 
reviewers to find more details.

More generally, having the section/version in the commit message is 
useful for future reference.

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

It would have been nice to see those kind of details in the commit message.

Regards,

-- 
Julien Grall

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