diff mbox series

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

Message ID 20190514122456.28559-20-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show
Series xen/arm: Clean-up & fixes in boot/mm code | expand

Commit Message

Julien Grall May 14, 2019, 12:24 p.m. UTC
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(-)

Comments

Stefano Stabellini June 4, 2019, 5:59 p.m. UTC | #1
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
>
Julien Grall June 4, 2019, 8:18 p.m. UTC | #2
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,
Stefano Stabellini June 4, 2019, 11:12 p.m. UTC | #3
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>
Julien Grall June 6, 2019, 5:38 p.m. UTC | #4
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,
diff mbox series

Patch

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,