Re: [Xen-devel] [PATCH v2] arm: Fix asynchronous aborts (SError exceptions) due to bogus PTEs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 22 Mar 2016, Shanker Donthineni wrote:
> On 03/22/2016 05:21 PM, Julien Grall wrote:
> > (CC some ARM folks)
> >
> > On 21/03/2016 23:18, Shanker Donthineni wrote:
> >> Hi Julien,
> >
> > Hello Shanker,
> >
> > Sorry for the late answer.
> >
> >> Do you have any other comments to be addressed?
> >
> > I have a question regarding the implication for what you wrote in the 
> > commit.
> >
> > As far as I understand, any speculative table walk might cause an imprecise 
> > asynchronous abort. So if a guest is using page tables that contain 
> > garbage, it would be possible to receive an SError. Am I right?
> >
> 
> Yes, you are right (applies to EL1 TTBR0/TTBR1, EL2 TTBR0/TTBR1 and EL3 TTBR0 
> tables).
> 
> >>
> >> On 03/16/2016 02:08 PM, Shanker Donthineni wrote:
> >>> From: Vikram Sethi <vikrams@xxxxxxxxxxxxxx>
> >>>
> >>> ARMv8 architecture allows performing prefetch data/instructions
> >>> from memory locations marked as normal memory. Prefetch does not
> >>> mean that the data/instruction has to be used/executed in code
> >>> flow. All PTEs that appear to be valid to MMU must contain valid
> >>> physical address with proper attributes otherwise MMU table walk
> >>> might cause imprecise asynchronous aborts.
> >>>
> >>> The way current XEN code is preparing page tables for frametable
> >>> and xenheap memory can create bogus PTEs. This patch fixes the
> >>> issue by clearing page table memory before populating EL2 L0/L1
> >>> PTEs. Without this patch XEN crashes on Qualcomm Technologies
> >>> server chips due to asynchronous aborts.
> >>>
> >>> The speculative/prefetch feature explanation is scattered everywhere
> >>> in ARM specification but below two sections have useful information.
> >>>
> >>> E2.8 Memory types and attributes
> >>> G4.12.6 External abort on a translation table walk
> >
> > As said on an earlier version of this patch, please mention the version of 
> > the spec when you quote it.
> >
> 
> Sure, should I post V3 patch mentioning ARM spec version?

Yes, please.


> >>> Signed-off-by: Vikram Sethi <vikrams@xxxxxxxxxxxxxx>
> >>> Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
> >>> ---
> >>> Changes since v1:
> >>>      Replace memset() with clear_page()
> >>>      Edit commit description
> >>>
> >>>   xen/arch/arm/mm.c | 3 +++
> >>>   1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> >>> index 81f9e2e..3fda8f3 100644
> >>> --- a/xen/arch/arm/mm.c
> >>> +++ b/xen/arch/arm/mm.c
> >>> @@ -730,6 +730,8 @@ void __init setup_xenheap_mappings(unsigned long 
> >>> base_mfn,
> >>>           else
> >>>           {
> >>>               unsigned long first_mfn = alloc_boot_pages(1, 1);
> >>> +
> >>> +            clear_page(mfn_to_virt(first_mfn));
> >>>               pte = mfn_to_xen_entry(first_mfn, WRITEALLOC);
> >>>               pte.pt.table = 1;
> >>>               write_pte(p, pte);
> >>> @@ -773,6 +775,7 @@ void __init setup_frametable_mappings(paddr_t ps, 
> >>> paddr_t pe)
> >>>       second = mfn_to_virt(second_base);
> >>>       for ( i = 0; i < nr_second; i++ )
> >>>       {
> >>> +        clear_page(mfn_to_virt(second_base + i));
> >>>           pte = mfn_to_xen_entry(second_base + i, WRITEALLOC);
> >>>           pte.pt.table = 1;
> >>>           
> >>> write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
> >>
> >
> > 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