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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
