xen/arm: remove special dom0 case in dump_hyp_walk

Commit Message

Stefano Stabellini Oct. 15, 2019, 3:49 a.m. UTC

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&m=157081398022401
Signed-off-by: Stefano Stabellini <[e-mail]>

Comments

Jürgen Groß Oct. 15, 2019, 4:21 a.m. UTC | #1

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&m=157081398022401
> Signed-off-by: Stefano Stabellini <[e-mail]>
> 
> 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

Julien Grall Oct. 15, 2019, 10:58 a.m. UTC | #2

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,

Stefano Stabellini Oct. 15, 2019, 3:50 p.m. UTC | #3

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&m=157081398022401
> > Signed-off-by: Stefano Stabellini <[e-mail]>
> > 
> > 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.

Stefano Stabellini Oct. 15, 2019, 3:51 p.m. UTC | #4

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[...]" ?

Julien Grall Oct. 15, 2019, 3:59 p.m. UTC | #5

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,

Patch

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);
 }