From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen() Date: Wed, 17 Jul 2013 17:31:05 +0100 Message-ID: References: <51E6DBE302000078000E5B36@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1UzUdH-0001GD-T9 for xen-devel@lists.xenproject.org; Wed, 17 Jul 2013 16:31:16 +0000 Received: by mail-we0-f179.google.com with SMTP id w59so1984112wes.38 for ; Wed, 17 Jul 2013 09:31:14 -0700 (PDT) In-Reply-To: <51E6DBE302000078000E5B36@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 17/07/2013 17:01, "Jan Beulich" wrote: >>>> On 17.07.13 at 17:40, Andrew Cooper wrote: >> On 17/07/13 16:09, Jan Beulich wrote: >>> This fixes yet another shortcoming of the function (exposed by 8bfaa2c2 >>> ["x86: add locking to map_pages_to_xen()"]'s adjustment to >>> msix_put_fixmap()): It must not flush caches when transitioning to a >>> non-present mapping. Doing so causes the CLFLUSH to fault, if used in >>> favor of WBINVD. >>> >>> To help code readability, factor out the whole flush flags updating >>> in map_pages_to_xen() into a helper macro. >>> >>> Signed-off-by: Jan Beulich >>> >>> --- a/xen/arch/x86/mm.c >>> +++ b/xen/arch/x86/mm.c >>> @@ -5430,6 +5430,15 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned l >>> flush_area_local((const void *)v, f) : \ >>> flush_area_all((const void *)v, f)) >>> >>> +#define flush_flags(oldf) do { \ >>> + unsigned int o_ = (oldf); \ >>> + if ( (o_) & _PAGE_GLOBAL ) \ >>> + flush_flags |= FLUSH_TLB_GLOBAL; \ >>> + if ( (flags & _PAGE_PRESENT) && \ >>> + (((o_) ^ flags) & PAGE_CACHE_ATTRS) ) \ >>> + flush_flags |= FLUSH_CACHE; \ >>> +} while (0) >>> + >> >> I have to admit to being surprised that the compiler is even happy with >> a macro aliasing a variable, but please can it be renamed to something >> else (perhaps "set_flush_flags" ?) for the sanity of other humans trying >> to read the code. > > Ugly. I actually picked the same name intentionally. I'm not too strongly opinionated on this one, but it did make me look twice. I think it would be cleaner something like: #define flush_flags(oldf) ({ unsigned int f_ = 0; ...; _f; }) ... flush_flags |= flush_flags(...); The 'name collision' I'm fine with, whereas going directly at a caller's variable within a macro is rather grubby behaviour. ;) Either way though I find the patch acceptable. And I will Ack it as already posted. -- Keir >> Furthermore, are we not introducing consistency errors? >> >> Previously, we occasionally decided to flush specific cache lines, and >> are now conditionally not flushing the cache lines depending on the >> mappings. >> >> Should the fix not be "If we need to flush parts of the cache and dont >> have mappings to what we want to flush, use wbinvd()" ? > > No. The cache flushing (a half hearted attempt only anyway) is > necessary only when changing cachability of a mapping (and in > fact the condition could be further relaxed to cover only the case > where a translation for the same physical address changes > cachability, but that would require not only comparing the > single MFNs from the old and new PTE, but the sets of all the > ones affected by the current function invocation). It could also > be relaxed to e.g. not get done when transitioning from UC or > to WB. But that's all pretty pointless. > > As to the "half hearted" above: What we do currently isn't really > correct anyway: When transitioning a translation to a different > caching mode, we'd have to tear down the translation, flush TLB, > flush cache, and _only then_ establish the new translation. But > what we do right now seems to be good enough in practice. > > Jan >