Re: [Xen-devel] [[PATCH for-4.13]] xen/arm: mm: Allow generic xen page-tables helpers to be called early

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 11 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/11/19 6:11 PM, Stefano Stabellini wrote:
> > > This is not pretty, but I don't view this as critical to fix for Xen 4.13.
> > > So
> > > I am thinking to defer this post 4.13.
> > 
> > If the issue doesn't trigger on 4.13, then I agree we shouldn't try to
> > fix it (badly) at this stage.
> > 
> > But we do have a "minus phys_offset" operation in dump_hyp_walk, I don't
> > follow why we wouldn't see a problem if Xen is loaded at 1MB on arm32.
> 
> I haven't said the issue cannot be triggered. I pointed out I don't view this
> as critical.
> 
> One of the reason is that I bet nobody ever run Xen below 1MB on Arm32.
> Otherwise they would have seen an error...
> 
> In other words, I am not going to plan to fix it for Xen 4.13. However, if
> someone wants to spend time on it (and have platform to test it), then patch
> are welcomed.

If the issue can be triggered then I think we have a choice between
fixing it (you don't necessarily have to be the one to do it) or
documenting that Xen needs to be loaded above XEN_VIRT_ADDR on arm32.
But see below.


> > Xen pa: 0x100000
> > Xen va: 0x200000
> > phys_offset: 0xfff00000
> > 
> > test_va: 0x202000
> > test_va - phys_offset = 0xffffffff00300200. But it should be 0x102000. >
> > Given that the problem is in a BUG_ON maybe we could remove it? Or just
> > rework the BUG_ON?
> 
> For arm32, dump_hyp_walk() is only called when the AT instruction fails to
> translate a physical address. You are already doing something wrong if you
> hit, so you will panic in either case. The only difference is you don't get
> the page-table dumped.

Why don't we change the BUG_ON like the following:

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a6637ce347..b7883cfbab 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -284,10 +284,7 @@ void dump_hyp_walk(vaddr_t addr)
            "on CPU%d via TTBR 0x%016"PRIx64"\n",
            addr, smp_processor_id(), ttbr);
 
-    if ( smp_processor_id() == 0 )
-        BUG_ON( (lpae_t *)(unsigned long)(ttbr - phys_offset) != pgtable );
-    else
-        BUG_ON( virt_to_maddr(pgtable) != ttbr );
+    BUG_ON( virt_to_maddr(pgtable) != ttbr );
     dump_pt_walk(ttbr, addr, HYP_PT_ROOT_LEVEL, 1);
 }
 
We probably don't need the CPU0 special case?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel