From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen: arm: invalidate caches after map_domain_page done Date: Fri, 01 Aug 2014 11:28:49 +0100 Message-ID: <53DB6BE1.2040005@linaro.org> References: <1406877914-31583-1-git-send-email-andrii.tseglytskyi@globallogic.com> <53DB5C88.8030602@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrii Tseglytskyi Cc: Tim Deegan , Stefano Stabellini , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 01/08/14 11:01, Andrii Tseglytskyi wrote: > On Fri, Aug 1, 2014 at 12:23 PM, Julien Grall wrote: >> >> Hi Andrii, >> >> >> On 01/08/14 08:25, Andrii Tseglytskyi wrote: >>> >>> In some cases, memory page returned by map_domain_page() contains >>> invalid data. Issue is observed when map_domain_page() is used >>> immediately after p2m_lookup() function, when random page of >>> guest domain memory is need to be mapped to xen. Data on this >>> already memory page may be not valid. Issue is fixed when >>> caches are invalidated after mapping is done. >>> >>> Signed-off-by: Andrii Tseglytskyi >>> --- >>> xen/arch/arm/mm.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>> index 0a243b0..085780a 100644 >>> --- a/xen/arch/arm/mm.c >>> +++ b/xen/arch/arm/mm.c >>> @@ -304,7 +304,7 @@ void *map_domain_page(unsigned long mfn) >>> * We may not have flushed this specific subpage at map time, >>> * since we only flush the 4k page not the superpage >>> */ >>> - flush_xen_data_tlb_range_va_local(va, PAGE_SIZE); >> >> >> Why did you remove the flush TLB? It's requirement to make sure the VA will pointed to the right PA. >> >>> + clean_and_invalidate_xen_dcache_va_range((void *)va, PAGE_SIZE); >> >> >> This is not the right approach, map_domain_page is heavily used to map hypercall data page. Those pages must reside in normal inner-cacheable > > What if use map_domain_page() outside hypercall ? In general we require cache enabled for any communication with Xen. Until now, we didn't have any case where map_domain_page is used outside this scope. >> memory. So cleaning the cache is useless and time consuming. > > I see that without cache invalidating page contains invalid data. Let > me explain more deeply: > As far as you know - I resumed my work with remoteproc MMU driver and > I started fixing review comments. One of your comments is that > ioremap_nocache() API should not be used for mapping domain pages, > therefore I tried using map_domain_page, and this works fine for me > only in case if I invalidate caches of already mapped va. > > I compared page dumps - 1 st page was mapped with ioremap_nocache() > function, second page was mapped with map_domain_page() function, and > I got the following output: > > (XEN) SGX_L2_MMU: pte_table[0] 0x9d428019 tmp[0] 0x9d428019 > (XEN) SGX_L2_MMU: pte_table[1] 0x9d429019 tmp[1] 0x9d429019 > (XEN) SGX_L2_MMU: pte_table[2] 0x9d42e019 tmp[2] 0x9d42e019 > (XEN) SGX_L2_MMU: pte_table[3] 0x9d42f019 tmp[3] 0x00000000 <-- data > is not valid here > > pte_table pointer is mapped using ioremap_nocache(), tmp is mapped > using map_domain_page() >> >> >> If you want to clean and invalidate the cache, even though I don't think this right by reading the commit message, you have to introduce a new helper. >> > > Taking in account your previous comment - that map_domain_page() is > used for mapping of hypercall data page, looks like I can't use this > API as is. In my code I solved this by calling of > clean_and_invalidate_xen_dcache_va_range() immediately after page is > mapped. This works fine for me - no invalid data is observed. What is the attribute of this page in guest? non-cacheable? I understand why you would need the invalidate, even though it's specific to your case. But not the clean... If the page is non-cacheable you may write stall data in the memory. Regards, -- Julien Grall