* [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
@ 2013-07-17 15:09 Jan Beulich
2013-07-17 15:40 ` Andrew Cooper
2013-07-17 16:31 ` Keir Fraser
0 siblings, 2 replies; 8+ messages in thread
From: Jan Beulich @ 2013-07-17 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 5684 bytes --]
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 <jbeulich@suse.com>
--- 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)
+
int map_pages_to_xen(
unsigned long virt,
unsigned long mfn,
@@ -5465,11 +5474,7 @@ int map_pages_to_xen(
if ( l3e_get_flags(ol3e) & _PAGE_PSE )
{
- if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l3e_get_flags(ol3e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+ flush_flags(lNf_to_l1f(l3e_get_flags(ol3e)));
flush_area(virt, flush_flags);
}
else
@@ -5481,27 +5486,14 @@ int map_pages_to_xen(
if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) )
continue;
if ( l2e_get_flags(ol2e) & _PAGE_PSE )
- {
- if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
else
{
unsigned int j;
pl1e = l2e_to_l1e(ol2e);
for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ )
- {
- ol1e = pl1e[j];
- if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(ol1e) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(l1e_get_flags(pl1e[j]));
}
}
flush_area(virt, flush_flags);
@@ -5595,24 +5587,14 @@ int map_pages_to_xen(
if ( l2e_get_flags(ol2e) & _PAGE_PSE )
{
- if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+ flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
flush_area(virt, flush_flags);
}
else
{
pl1e = l2e_to_l1e(ol2e);
for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
- {
- if ( l1e_get_flags(pl1e[i]) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(pl1e[i]) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(l1e_get_flags(pl1e[i]));
flush_area(virt, flush_flags);
free_xen_pagetable(pl1e);
}
@@ -5687,10 +5669,8 @@ int map_pages_to_xen(
if ( (l1e_get_flags(ol1e) & _PAGE_PRESENT) )
{
unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0);
- if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(ol1e) ^ flags) & PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+
+ flush_flags(l1e_get_flags(ol1e));
flush_area(virt, flush_flags);
}
@@ -5769,6 +5749,8 @@ int map_pages_to_xen(
return 0;
}
+#undef flush_flags
+
void destroy_xen_mappings(unsigned long s, unsigned long e)
{
bool_t locking = system_state > SYS_STATE_boot;
@@ -5908,6 +5890,8 @@ void destroy_xen_mappings(unsigned long
flush_area(NULL, FLUSH_TLB_GLOBAL);
}
+#undef flush_area
+
void __set_fixmap(
enum fixed_addresses idx, unsigned long mfn, unsigned long flags)
{
[-- Attachment #2: x86-mp2x-less-cache-flushing.patch --]
[-- Type: text/plain, Size: 5739 bytes --]
x86: fix cache flushing condition in map_pages_to_xen()
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 <jbeulich@suse.com>
--- 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)
+
int map_pages_to_xen(
unsigned long virt,
unsigned long mfn,
@@ -5465,11 +5474,7 @@ int map_pages_to_xen(
if ( l3e_get_flags(ol3e) & _PAGE_PSE )
{
- if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l3e_get_flags(ol3e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+ flush_flags(lNf_to_l1f(l3e_get_flags(ol3e)));
flush_area(virt, flush_flags);
}
else
@@ -5481,27 +5486,14 @@ int map_pages_to_xen(
if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) )
continue;
if ( l2e_get_flags(ol2e) & _PAGE_PSE )
- {
- if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
else
{
unsigned int j;
pl1e = l2e_to_l1e(ol2e);
for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ )
- {
- ol1e = pl1e[j];
- if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(ol1e) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(l1e_get_flags(pl1e[j]));
}
}
flush_area(virt, flush_flags);
@@ -5595,24 +5587,14 @@ int map_pages_to_xen(
if ( l2e_get_flags(ol2e) & _PAGE_PSE )
{
- if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+ flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
flush_area(virt, flush_flags);
}
else
{
pl1e = l2e_to_l1e(ol2e);
for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
- {
- if ( l1e_get_flags(pl1e[i]) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(pl1e[i]) ^ flags) &
- PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
- }
+ flush_flags(l1e_get_flags(pl1e[i]));
flush_area(virt, flush_flags);
free_xen_pagetable(pl1e);
}
@@ -5687,10 +5669,8 @@ int map_pages_to_xen(
if ( (l1e_get_flags(ol1e) & _PAGE_PRESENT) )
{
unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0);
- if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
- flush_flags |= FLUSH_TLB_GLOBAL;
- if ( (l1e_get_flags(ol1e) ^ flags) & PAGE_CACHE_ATTRS )
- flush_flags |= FLUSH_CACHE;
+
+ flush_flags(l1e_get_flags(ol1e));
flush_area(virt, flush_flags);
}
@@ -5769,6 +5749,8 @@ int map_pages_to_xen(
return 0;
}
+#undef flush_flags
+
void destroy_xen_mappings(unsigned long s, unsigned long e)
{
bool_t locking = system_state > SYS_STATE_boot;
@@ -5908,6 +5890,8 @@ void destroy_xen_mappings(unsigned long
flush_area(NULL, FLUSH_TLB_GLOBAL);
}
+#undef flush_area
+
void __set_fixmap(
enum fixed_addresses idx, unsigned long mfn, unsigned long flags)
{
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 15:09 [PATCH] x86: fix cache flushing condition in map_pages_to_xen() Jan Beulich
@ 2013-07-17 15:40 ` Andrew Cooper
2013-07-17 16:01 ` Jan Beulich
2013-07-17 16:31 ` Keir Fraser
1 sibling, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2013-07-17 15:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 6568 bytes --]
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 <jbeulich@suse.com>
>
> --- 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.
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()" ?
~Andrew
> int map_pages_to_xen(
> unsigned long virt,
> unsigned long mfn,
> @@ -5465,11 +5474,7 @@ int map_pages_to_xen(
>
> if ( l3e_get_flags(ol3e) & _PAGE_PSE )
> {
> - if ( l3e_get_flags(ol3e) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (lNf_to_l1f(l3e_get_flags(ol3e)) ^ flags) &
> - PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> + flush_flags(lNf_to_l1f(l3e_get_flags(ol3e)));
> flush_area(virt, flush_flags);
> }
> else
> @@ -5481,27 +5486,14 @@ int map_pages_to_xen(
> if ( !(l2e_get_flags(ol2e) & _PAGE_PRESENT) )
> continue;
> if ( l2e_get_flags(ol2e) & _PAGE_PSE )
> - {
> - if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
> - PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> - }
> + flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
> else
> {
> unsigned int j;
>
> pl1e = l2e_to_l1e(ol2e);
> for ( j = 0; j < L1_PAGETABLE_ENTRIES; j++ )
> - {
> - ol1e = pl1e[j];
> - if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (l1e_get_flags(ol1e) ^ flags) &
> - PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> - }
> + flush_flags(l1e_get_flags(pl1e[j]));
> }
> }
> flush_area(virt, flush_flags);
> @@ -5595,24 +5587,14 @@ int map_pages_to_xen(
>
> if ( l2e_get_flags(ol2e) & _PAGE_PSE )
> {
> - if ( l2e_get_flags(ol2e) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (lNf_to_l1f(l2e_get_flags(ol2e)) ^ flags) &
> - PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> + flush_flags(lNf_to_l1f(l2e_get_flags(ol2e)));
> flush_area(virt, flush_flags);
> }
> else
> {
> pl1e = l2e_to_l1e(ol2e);
> for ( i = 0; i < L1_PAGETABLE_ENTRIES; i++ )
> - {
> - if ( l1e_get_flags(pl1e[i]) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (l1e_get_flags(pl1e[i]) ^ flags) &
> - PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> - }
> + flush_flags(l1e_get_flags(pl1e[i]));
> flush_area(virt, flush_flags);
> free_xen_pagetable(pl1e);
> }
> @@ -5687,10 +5669,8 @@ int map_pages_to_xen(
> if ( (l1e_get_flags(ol1e) & _PAGE_PRESENT) )
> {
> unsigned int flush_flags = FLUSH_TLB | FLUSH_ORDER(0);
> - if ( l1e_get_flags(ol1e) & _PAGE_GLOBAL )
> - flush_flags |= FLUSH_TLB_GLOBAL;
> - if ( (l1e_get_flags(ol1e) ^ flags) & PAGE_CACHE_ATTRS )
> - flush_flags |= FLUSH_CACHE;
> +
> + flush_flags(l1e_get_flags(ol1e));
> flush_area(virt, flush_flags);
> }
>
> @@ -5769,6 +5749,8 @@ int map_pages_to_xen(
> return 0;
> }
>
> +#undef flush_flags
> +
> void destroy_xen_mappings(unsigned long s, unsigned long e)
> {
> bool_t locking = system_state > SYS_STATE_boot;
> @@ -5908,6 +5890,8 @@ void destroy_xen_mappings(unsigned long
> flush_area(NULL, FLUSH_TLB_GLOBAL);
> }
>
> +#undef flush_area
> +
> void __set_fixmap(
> enum fixed_addresses idx, unsigned long mfn, unsigned long flags)
> {
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 7336 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 15:40 ` Andrew Cooper
@ 2013-07-17 16:01 ` Jan Beulich
2013-07-17 16:31 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2013-07-17 16:01 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@citrix.com> 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 <jbeulich@suse.com>
>>
>> --- 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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 16:01 ` Jan Beulich
@ 2013-07-17 16:31 ` Keir Fraser
2013-07-18 5:49 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Keir Fraser @ 2013-07-17 16:31 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: xen-devel
On 17/07/2013 17:01, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@citrix.com> 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 <jbeulich@suse.com>
>>>
>>> --- 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 15:09 [PATCH] x86: fix cache flushing condition in map_pages_to_xen() Jan Beulich
2013-07-17 15:40 ` Andrew Cooper
@ 2013-07-17 16:31 ` Keir Fraser
2013-07-17 20:41 ` Sander Eikelenboom
1 sibling, 1 reply; 8+ messages in thread
From: Keir Fraser @ 2013-07-17 16:31 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 17/07/2013 16:09, "Jan Beulich" <JBeulich@suse.com> 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 <jbeulich@suse.com>
I'd prefer the macro modified as I just posted, but either way:
Acked-by: Keir Fraser <keir@xen.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 16:31 ` Keir Fraser
@ 2013-07-17 20:41 ` Sander Eikelenboom
0 siblings, 0 replies; 8+ messages in thread
From: Sander Eikelenboom @ 2013-07-17 20:41 UTC (permalink / raw)
To: Keir Fraser; +Cc: xen-devel, Jan Beulich
Wednesday, July 17, 2013, 6:31:41 PM, you wrote:
> On 17/07/2013 16:09, "Jan Beulich" <JBeulich@suse.com> 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 <jbeulich@suse.com>
> I'd prefer the macro modified as I just posted, but either way:
> Acked-by: Keir Fraser <keir@xen.org>
Fixes my issue (reported in thread http://lists.xenproject.org/archives/html/xen-devel/2013-07/msg01609.html)
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-17 16:31 ` Keir Fraser
@ 2013-07-18 5:49 ` Jan Beulich
2013-07-18 7:37 ` Keir Fraser
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2013-07-18 5:49 UTC (permalink / raw)
To: andrew.cooper3, keir.xen; +Cc: xen-devel
>>>> Keir Fraser <keir.xen@gmail.com> 07/17/13 6:31 PM >>>
>On 17/07/2013 17:01, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 17.07.13 at 17:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> On 17/07/13 16:09, Jan Beulich wrote:
>>>> +#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. ;)
But that would still leave the macro access "flags" directly. And while I realize
that this is (slightly) odd behavior for a macro, that's precisely the reason why
I #undef it right after the function. Perhaps this could be made even more clear
by moving #define and #undef inside the function...
Jan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86: fix cache flushing condition in map_pages_to_xen()
2013-07-18 5:49 ` Jan Beulich
@ 2013-07-18 7:37 ` Keir Fraser
0 siblings, 0 replies; 8+ messages in thread
From: Keir Fraser @ 2013-07-18 7:37 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper; +Cc: xen-devel
On 18/07/2013 06:49, "Jan Beulich" <jbeulich@suse.com> wrote:
>>> 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. ;)
>
> But that would still leave the macro access "flags" directly.
Hmmm true. Don't really want to expand the verbiage at each macro call site
any further, so I guess your existing approach is also my preferred one.
> And while I
> realize
> that this is (slightly) odd behavior for a macro, that's precisely the reason
> why
> I #undef it right after the function. Perhaps this could be made even more
> clear
> by moving #define and #undef inside the function...
Yes, that might be slightly preferable.
-- Keir
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-07-18 7:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-17 15:09 [PATCH] x86: fix cache flushing condition in map_pages_to_xen() Jan Beulich
2013-07-17 15:40 ` Andrew Cooper
2013-07-17 16:01 ` Jan Beulich
2013-07-17 16:31 ` Keir Fraser
2013-07-18 5:49 ` Jan Beulich
2013-07-18 7:37 ` Keir Fraser
2013-07-17 16:31 ` Keir Fraser
2013-07-17 20:41 ` Sander Eikelenboom
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.