| Message ID | 20190514122456.28559-20-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Clean-up & fixes in boot/mm code | expand |
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 >
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,
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>
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 --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,