Re: [Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
Hi Stefano, On 10/15/19 4:51 PM, Stefano Stabellini wrote: On Tue, 15 Oct 2019, Julien Grall wrote: Hi Stefano, On 10/15/19 4:49 AM, Stefano Stabellini wrote: There is no need to have a special dom0 case to convert the pgtable virtual address into a physical address. The virt_to_maddr function can work both in the dom0 case and the domU case. This is more than a cleanup: when Xen is loaded at addresses lower than 2MB on arm32 phys_offset might not hold the right value and be unable to perform a virt to phys conversion properly. Reducing the unnecessary usage of phys_offset is a good idea. Aside what Juergen said, this paragraph raises the question why phys_offset is actually not modified (or completely dropped)? After all, if the value holds is wrong then other users may get wrong value as well... Should I add something like "this is not a fix, but reducing unnecessary[...]" ? dump_hyp_walk() is only used in two places: - when hardware translation failed - Hypervisor data abort In both case we will panic afterwards. So hitting the BUG_ON() is just only a matter of losing page-table information. So I see limited reason to see this patch alone in Xen 4.13. We should either completely fix phys_offset or do nothing. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
On Tue, 15 Oct 2019, Julien Grall wrote: > Hi Stefano, > > On 10/15/19 4:49 AM, Stefano Stabellini wrote: > > There is no need to have a special dom0 case to convert the pgtable > > virtual address into a physical address. The virt_to_maddr function can > > work both in the dom0 case and the domU case. > > > > This is more than a cleanup: when Xen is loaded at addresses lower than > > 2MB on arm32 phys_offset might not hold the right value and be unable to > > perform a virt to phys conversion properly. Reducing the unnecessary > > usage of phys_offset is a good idea. > > Aside what Juergen said, this paragraph raises the question why phys_offset is > actually not modified (or completely dropped)? After all, if the value holds > is wrong then other users may get wrong value as well... Should I add something like "this is not a fix, but reducing unnecessary[...]" ? ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
On Tue, 15 Oct 2019, Jürgen Groß wrote:
> On 15.10.19 05:49, Stefano Stabellini wrote:
> > There is no need to have a special dom0 case to convert the pgtable
> > virtual address into a physical address. The virt_to_maddr function can
> > work both in the dom0 case and the domU case.
> >
> > This is more than a cleanup: when Xen is loaded at addresses lower than
> > 2MB on arm32 phys_offset might not hold the right value and be unable to
> > perform a virt to phys conversion properly. Reducing the unnecessary
> > usage of phys_offset is a good idea.
> >
> > Link: https://marc.info/?l=xen-devel=157081398022401
> > Signed-off-by: Stefano Stabellini
> >
> > 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);
> > }
>
> I can't make a connection between commit message ("special dom0 case")
> and the code modification. The special case removed is about cpu 0, not
> dom0.
That's what happen when I write a commit message late in the night.
Sorry about that. I'll resend it.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
Hi Stefano, On 10/15/19 4:49 AM, Stefano Stabellini wrote: There is no need to have a special dom0 case to convert the pgtable virtual address into a physical address. The virt_to_maddr function can work both in the dom0 case and the domU case. This is more than a cleanup: when Xen is loaded at addresses lower than 2MB on arm32 phys_offset might not hold the right value and be unable to perform a virt to phys conversion properly. Reducing the unnecessary usage of phys_offset is a good idea. Aside what Juergen said, this paragraph raises the question why phys_offset is actually not modified (or completely dropped)? After all, if the value holds is wrong then other users may get wrong value as well... Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
On 15.10.19 05:49, Stefano Stabellini wrote:
There is no need to have a special dom0 case to convert the pgtable
virtual address into a physical address. The virt_to_maddr function can
work both in the dom0 case and the domU case.
This is more than a cleanup: when Xen is loaded at addresses lower than
2MB on arm32 phys_offset might not hold the right value and be unable to
perform a virt to phys conversion properly. Reducing the unnecessary
usage of phys_offset is a good idea.
Link: https://marc.info/?l=xen-devel=157081398022401
Signed-off-by: Stefano Stabellini
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);
}
I can't make a connection between commit message ("special dom0 case")
and the code modification. The special case removed is about cpu 0, not
dom0.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] xen/arm: remove special dom0 case in dump_hyp_walk
There is no need to have a special dom0 case to convert the pgtable virtual address into a physical address. The virt_to_maddr function can work both in the dom0 case and the domU case. This is more than a cleanup: when Xen is loaded at addresses lower than 2MB on arm32 phys_offset might not hold the right value and be unable to perform a virt to phys conversion properly. Reducing the unnecessary usage of phys_offset is a good idea. Link: https://marc.info/?l=xen-devel=157081398022401 Signed-off-by: Stefano Stabellini 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); } ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
