[v2] xen/arm: Clean-up & fixes in boot/mm code

[Xen-devel] [PATCH MM-PART2 RESEND v2 19/19] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

Posted by Julien Grall 6 years, 9 months ago

At the moment, set_fixmap may replace a valid entry without following
the break-before-make sequence. This may result to TLB conflict abort.

Rather than dealing with Break-Before-Make in set_fixmap, every call to
set_fixmap is paired with a call to clear_fixmap.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>

---
    Changes in v2:
        - Add Andrii's reviewed-by
---
 xen/arch/arm/kernel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index e3ffdb2fa1..389bef2afa 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -58,13 +58,12 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
         set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
         memcpy(dst, src + s, l);
         clean_dcache_va_range(dst, l);
+        clear_fixmap(FIXMAP_MISC);
 
         paddr += l;
         dst += l;
         len -= l;
     }
-
-    clear_fixmap(FIXMAP_MISC);
 }
 
 static void __init place_modules(struct kernel_info *info,
-- 
2.11.0


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

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 19/19] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

Posted by Stefano Stabellini 6 years, 8 months ago

On Tue, 14 May 2019, Julien Grall wrote:
> At the moment, set_fixmap may replace a valid entry without following
> the break-before-make sequence. This may result to TLB conflict abort.
> 
> Rather than dealing with Break-Before-Make in set_fixmap, every call to
> set_fixmap is paired with a call to clear_fixmap.

It is not every call to set_fixmap: it is every call to
set_fixmap(FIXMAP_MISC, ...

Please clarify, then you can add

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andrii Anisov <andrii_anisov@epam.com>
>
> ---
>     Changes in v2:
>         - Add Andrii's reviewed-by
> ---
>  xen/arch/arm/kernel.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index e3ffdb2fa1..389bef2afa 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -58,13 +58,12 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>          set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_WC);
>          memcpy(dst, src + s, l);
>          clean_dcache_va_range(dst, l);
> +        clear_fixmap(FIXMAP_MISC);
>  
>          paddr += l;
>          dst += l;
>          len -= l;
>      }
> -
> -    clear_fixmap(FIXMAP_MISC);
>  }
>  
>  static void __init place_modules(struct kernel_info *info,
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 19/19] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

Posted by Julien Grall 6 years, 8 months ago


On 6/4/19 6:59 PM, Stefano Stabellini wrote:
> On Tue, 14 May 2019, Julien Grall wrote:
>> At the moment, set_fixmap may replace a valid entry without following
>> the break-before-make sequence. This may result to TLB conflict abort.
>>
>> Rather than dealing with Break-Before-Make in set_fixmap, every call to
>> set_fixmap is paired with a call to clear_fixmap.
> 
> It is not every call to set_fixmap: it is every call to
> set_fixmap(FIXMAP_MISC, ...

I don't understand this request... The title explicit mention 
"copy_from_paddr" and fixmap is only called with FIXMAP_MISC.

So why should I need to specify the argument?

> 
> Please clarify, then you can add
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 19/19] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

Posted by Stefano Stabellini 6 years, 8 months ago

On Tue, 4 Jun 2019, Julien Grall wrote:
> On 6/4/19 6:59 PM, Stefano Stabellini wrote:
> > On Tue, 14 May 2019, Julien Grall wrote:
> > > At the moment, set_fixmap may replace a valid entry without following
> > > the break-before-make sequence. This may result to TLB conflict abort.
> > > 
> > > Rather than dealing with Break-Before-Make in set_fixmap, every call to
> > > set_fixmap is paired with a call to clear_fixmap.
> > 
> > It is not every call to set_fixmap: it is every call to
> > set_fixmap(FIXMAP_MISC, ...
> 
> I don't understand this request... The title explicit mention
> "copy_from_paddr" and fixmap is only called with FIXMAP_MISC.
> 
> So why should I need to specify the argument?

I wasn't asking to mention FIXMAP_MISC explicitely, I don't think it is
particularly useful. I was only trying to make the wording more
specific to what the patch does.

The statement "every call to set_fixmap is paired with a call to
clear_fixmap" is too generic and I would prefer if it was limited in
scope by something like

  "in copy_from_paddr"

Like you have done in the subject. Resulting in:
  
  every call to set_fixmap in copy_from_paddr is paired with a call to
  clear_fixmap


> > Please clarify, then you can add
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


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

Re: [Xen-devel] [PATCH MM-PART2 RESEND v2 19/19] xen/arm: Pair call to set_fixmap with call to clear_fixmap in copy_from_paddr

Posted by Julien Grall 6 years, 8 months ago

Hi Stefano,

On 05/06/2019 00:12, Stefano Stabellini wrote:
> On Tue, 4 Jun 2019, Julien Grall wrote:
>> On 6/4/19 6:59 PM, Stefano Stabellini wrote:
>>> On Tue, 14 May 2019, Julien Grall wrote:
>>>> At the moment, set_fixmap may replace a valid entry without following
>>>> the break-before-make sequence. This may result to TLB conflict abort.
>>>>
>>>> Rather than dealing with Break-Before-Make in set_fixmap, every call to
>>>> set_fixmap is paired with a call to clear_fixmap.
>>>
>>> It is not every call to set_fixmap: it is every call to
>>> set_fixmap(FIXMAP_MISC, ...
>>
>> I don't understand this request... The title explicit mention
>> "copy_from_paddr" and fixmap is only called with FIXMAP_MISC.
>>
>> So why should I need to specify the argument?
> 
> I wasn't asking to mention FIXMAP_MISC explicitely, I don't think it is
> particularly useful. I was only trying to make the wording more
> specific to what the patch does.

I have to admit this wasn't clear from your answer. Anyway,...

> 
> The statement "every call to set_fixmap is paired with a call to
> clear_fixmap" is too generic and I would prefer if it was limited in
> scope by something like
> 
>    "in copy_from_paddr"
> 
> Like you have done in the subject. Resulting in:
>    
>    every call to set_fixmap in copy_from_paddr is paired with a call to
>    clear_fixmap

... thank you for your clarification. I have updated the commit message accordingly.

Cheers,

-- 
Julien Grall

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