From: Shanker Donthineni <shankerd@codeaurora.org>
To: Julien Grall <julien.grall@arm.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Jan Beulich <JBeulich@suse.com>
Cc: Julien Grall <julien.grall@citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Philip Elcan <pelcan@codeaurora.org>,
Vikram Sethi <vikrams@codeaurora.org>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Subject: Re: [PATCH] xen: arm: zero EL2 pagetable pages before use
Date: Mon, 14 Mar 2016 12:18:43 -0500 [thread overview]
Message-ID: <56E6F273.8030701@codeaurora.org> (raw)
In-Reply-To: <56E43DCE.6070409@arm.com>
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@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.
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@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-14 17:18 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-10 22:00 [PATCH] xen: arm: zero EL2 pagetable pages before use Shanker Donthineni
2016-03-11 11:29 ` Jan Beulich
2016-03-11 12:56 ` Andrew Cooper
2016-03-11 13:13 ` Jan Beulich
2016-03-11 13:24 ` Andrew Cooper
2016-03-12 16:03 ` Julien Grall
2016-03-14 17:18 ` Shanker Donthineni [this message]
2016-03-15 17:37 ` Julien Grall
2016-03-12 14:32 ` Julien Grall
2016-03-14 7:37 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56E6F273.8030701@codeaurora.org \
--to=shankerd@codeaurora.org \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=julien.grall@arm.com \
--cc=julien.grall@citrix.com \
--cc=pelcan@codeaurora.org \
--cc=stefano.stabellini@eu.citrix.com \
--cc=vikrams@codeaurora.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).