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

[Julien Grall <julien.grall@xxxxxxx>]

On 10/11/19 8:01 PM, Stefano Stabellini wrote:
> 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.
As I said before on a similar topic, if we document this, we also need 
to document that our Xen boot code is definitely not compliant with the 
Arm Arm and you are at risk to not be able to boot ;).
> > > 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?
We don't need it. Patch are welcomed.

Cheers,

--
Julien Grall

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