[PATCH 0/2] Use invpcid

[PATCH 0/2] Use invpcid

[Wei Liu]

Wei Liu (2):
  x86: report if PCID and INVPCID are supported
  x86: use invpcid to do global flushing

 xen/arch/x86/flushtlb.c | 22 ++++++++++++++++++----
 xen/arch/x86/setup.c    |  7 +++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/2] x86: report if PCID and INVPCID are supported

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/setup.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ac530ece2c..89e42865a4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
            cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
            cpu_has_nx ? "" : "not ");
 
+
+    printk(XENLOG_INFO
+           "PCID (Process-Context IDentifier) %ssupported\n",
+           cpu_has_pcid ? "" : "not ");
+
+    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
+
     /*
      * We're going to setup domain0 using the module(s) that we stashed safely
      * above our heap. The second module, if present, is an initrd ramdisk.
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Juergen Gross]

On 05/03/18 10:50, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Jan Beulich]

>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>             cpu_has_nx ? "" : "not ");
>  
> +
> +    printk(XENLOG_INFO
> +           "PCID (Process-Context IDentifier) %ssupported\n",
> +           cpu_has_pcid ? "" : "not ");
> +
> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");

Do we really need this? We log a message for NX as an exception,
we don't do so for other features (and things would get pretty
unwieldy if we did).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Wei Liu]

On Mon, Mar 05, 2018 at 04:20:48AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> >             cpu_has_nx ? "" : "not ");
> >  
> > +
> > +    printk(XENLOG_INFO
> > +           "PCID (Process-Context IDentifier) %ssupported\n",
> > +           cpu_has_pcid ? "" : "not ");
> > +
> > +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
> 
> Do we really need this? We log a message for NX as an exception,
> we don't do so for other features (and things would get pretty
> unwieldy if we did).
> 

I'm not too fuss really. It helped me to pick a machine when I didn't
have the exact spec of the pool of machines. The same information can
certainly be obtained from hardware cpuid info.

On another note, what is the most reliable way to extract hardware cpuid
info from Xen? AIUI now cpuid faulting can be enabled for both PV and
HVM guests so running cpuid in PV guests might not return the real bits
(which is a good thing).

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Andrew Cooper]

On 05/03/18 11:31, Wei Liu wrote:
> On Mon, Mar 05, 2018 at 04:20:48AM -0700, Jan Beulich wrote:
>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>>>             cpu_has_nx ? "" : "not ");
>>>  
>>> +
>>> +    printk(XENLOG_INFO
>>> +           "PCID (Process-Context IDentifier) %ssupported\n",
>>> +           cpu_has_pcid ? "" : "not ");
>>> +
>>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
>> Do we really need this? We log a message for NX as an exception,
>> we don't do so for other features (and things would get pretty
>> unwieldy if we did).
>>
> I'm not too fuss really. It helped me to pick a machine when I didn't
> have the exact spec of the pool of machines. The same information can
> certainly be obtained from hardware cpuid info.
>
> On another note, what is the most reliable way to extract hardware cpuid
> info from Xen? AIUI now cpuid faulting can be enabled for both PV and
> HVM guests so running cpuid in PV guests might not return the real bits
> (which is a good thing).

For now, `xen-cpuid`, which uses SYSCTL_get_cpufeatureset internally.

This will shortly be changing to get the fully cpuid/msr policies rather
than just the feature bitmaps.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Juergen Gross]

On 05/03/18 12:20, Jan Beulich wrote:
>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>>             cpu_has_nx ? "" : "not ");
>>  
>> +
>> +    printk(XENLOG_INFO
>> +           "PCID (Process-Context IDentifier) %ssupported\n",
>> +           cpu_has_pcid ? "" : "not ");
>> +
>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
> 
> Do we really need this? We log a message for NX as an exception,
> we don't do so for other features (and things would get pretty
> unwieldy if we did).

I'd rather keep this message. As we are hiding PCID and INPCID from dom0
this is the only indicator of those features being supported. In case of
an error related to TLB consistency this information is important IMO.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Jan Beulich]

>>> On 05.03.18 at 12:43, <jgross@suse.com> wrote:
> On 05/03/18 12:20, Jan Beulich wrote:
>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>>>             cpu_has_nx ? "" : "not ");
>>>  
>>> +
>>> +    printk(XENLOG_INFO
>>> +           "PCID (Process-Context IDentifier) %ssupported\n",
>>> +           cpu_has_pcid ? "" : "not ");
>>> +
>>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
>> 
>> Do we really need this? We log a message for NX as an exception,
>> we don't do so for other features (and things would get pretty
>> unwieldy if we did).
> 
> I'd rather keep this message. As we are hiding PCID and INPCID from dom0
> this is the only indicator of those features being supported. In case of
> an error related to TLB consistency this information is important IMO.

Well, this argument would hold for almost every other advanced
feature we use.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Juergen Gross]

On 05/03/18 13:37, Jan Beulich wrote:
>>>> On 05.03.18 at 12:43, <jgross@suse.com> wrote:
>> On 05/03/18 12:20, Jan Beulich wrote:
>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>> --- a/xen/arch/x86/setup.c
>>>> +++ b/xen/arch/x86/setup.c
>>>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>>>>             cpu_has_nx ? "" : "not ");
>>>>  
>>>> +
>>>> +    printk(XENLOG_INFO
>>>> +           "PCID (Process-Context IDentifier) %ssupported\n",
>>>> +           cpu_has_pcid ? "" : "not ");
>>>> +
>>>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
>>>
>>> Do we really need this? We log a message for NX as an exception,
>>> we don't do so for other features (and things would get pretty
>>> unwieldy if we did).
>>
>> I'd rather keep this message. As we are hiding PCID and INPCID from dom0
>> this is the only indicator of those features being supported. In case of
>> an error related to TLB consistency this information is important IMO.
> 
> Well, this argument would hold for almost every other advanced
> feature we use.

Then I'd suggest to either add individual messages for those features or
to have a single message (or a few) to list all features like the flags
in /proc/cpuinfo. This list could even be part of the "xl info" output.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Jan Beulich]

>>> On 05.03.18 at 13:49, <jgross@suse.com> wrote:
> On 05/03/18 13:37, Jan Beulich wrote:
>>>>> On 05.03.18 at 12:43, <jgross@suse.com> wrote:
>>> On 05/03/18 12:20, Jan Beulich wrote:
>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>>>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
>>>>>             cpu_has_nx ? "" : "not ");
>>>>>  
>>>>> +
>>>>> +    printk(XENLOG_INFO
>>>>> +           "PCID (Process-Context IDentifier) %ssupported\n",
>>>>> +           cpu_has_pcid ? "" : "not ");
>>>>> +
>>>>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
>>>>
>>>> Do we really need this? We log a message for NX as an exception,
>>>> we don't do so for other features (and things would get pretty
>>>> unwieldy if we did).
>>>
>>> I'd rather keep this message. As we are hiding PCID and INPCID from dom0
>>> this is the only indicator of those features being supported. In case of
>>> an error related to TLB consistency this information is important IMO.
>> 
>> Well, this argument would hold for almost every other advanced
>> feature we use.
> 
> Then I'd suggest to either add individual messages for those features or
> to have a single message (or a few) to list all features like the flags
> in /proc/cpuinfo. This list could even be part of the "xl info" output.

I have no idea whether there are plans to make this part of
"xl info" output; xen-cpuid certainly can tell you.

Wei, btw - for the case of there being problems with this new use
of INVPCID, perhaps you will also want to extend parse_xen_cpuid()
so that the use can be disabled from the command line.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] x86: report if PCID and INVPCID are supported

[Wei Liu]

On Mon, Mar 05, 2018 at 05:57:48AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 13:49, <jgross@suse.com> wrote:
> > On 05/03/18 13:37, Jan Beulich wrote:
> >>>>> On 05.03.18 at 12:43, <jgross@suse.com> wrote:
> >>> On 05/03/18 12:20, Jan Beulich wrote:
> >>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
> >>>>> --- a/xen/arch/x86/setup.c
> >>>>> +++ b/xen/arch/x86/setup.c
> >>>>> @@ -1701,6 +1701,13 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >>>>>             cpu_has_nx ? XENLOG_INFO : XENLOG_WARNING "Warning: ",
> >>>>>             cpu_has_nx ? "" : "not ");
> >>>>>  
> >>>>> +
> >>>>> +    printk(XENLOG_INFO
> >>>>> +           "PCID (Process-Context IDentifier) %ssupported\n",
> >>>>> +           cpu_has_pcid ? "" : "not ");
> >>>>> +
> >>>>> +    printk(XENLOG_INFO "INVPCID %ssupported\n", cpu_has_invpcid ? "" : "not ");
> >>>>
> >>>> Do we really need this? We log a message for NX as an exception,
> >>>> we don't do so for other features (and things would get pretty
> >>>> unwieldy if we did).
> >>>
> >>> I'd rather keep this message. As we are hiding PCID and INPCID from dom0
> >>> this is the only indicator of those features being supported. In case of
> >>> an error related to TLB consistency this information is important IMO.
> >> 
> >> Well, this argument would hold for almost every other advanced
> >> feature we use.
> > 
> > Then I'd suggest to either add individual messages for those features or
> > to have a single message (or a few) to list all features like the flags
> > in /proc/cpuinfo. This list could even be part of the "xl info" output.
> 
> I have no idea whether there are plans to make this part of
> "xl info" output; xen-cpuid certainly can tell you.
> 
> Wei, btw - for the case of there being problems with this new use
> of INVPCID, perhaps you will also want to extend parse_xen_cpuid()
> so that the use can be disabled from the command line.
> 

Noted.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/2] x86: use invpcid to do global flushing

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/flushtlb.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/flushtlb.c b/xen/arch/x86/flushtlb.c
index 8a7a76b8ff..e4ea4f3297 100644
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -9,6 +9,7 @@
 
 #include <xen/sched.h>
 #include <xen/softirq.h>
+#include <asm/invpcid.h>
 #include <asm/flushtlb.h>
 #include <asm/page.h>
 
@@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
         else
         {
             u32 t = pre_flush();
-            unsigned long cr4 = read_cr4();
 
-            write_cr4(cr4 & ~X86_CR4_PGE);
-            barrier();
-            write_cr4(cr4);
+            if ( !cpu_has_invpcid )
+            {
+                unsigned long cr4 = read_cr4();
+
+                write_cr4(cr4 & ~X86_CR4_PGE);
+                barrier();
+                write_cr4(cr4);
+            }
+            else
+            {
+                /*
+                 * Using invpcid to flush all mappings works
+                 * regardless of whether PCID is enabled or not.
+                 * It is faster than read-modify-write CR4.
+                 */
+                invpcid_flush_all();
+            }
 
             post_flush(t);
         }
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 05/03/18 10:50, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

No description at all? I'd at least expect mention of how much of a
performance win this is (for whichever hardware you happen to
know that).

> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>          else
>          {
>              u32 t = pre_flush();
> -            unsigned long cr4 = read_cr4();
>  
> -            write_cr4(cr4 & ~X86_CR4_PGE);
> -            barrier();
> -            write_cr4(cr4);
> +            if ( !cpu_has_invpcid )
> +            {
> +                unsigned long cr4 = read_cr4();
> +
> +                write_cr4(cr4 & ~X86_CR4_PGE);
> +                barrier();
> +                write_cr4(cr4);
> +            }
> +            else
> +            {
> +                /*
> +                 * Using invpcid to flush all mappings works
> +                 * regardless of whether PCID is enabled or not.
> +                 * It is faster than read-modify-write CR4.
> +                 */
> +                invpcid_flush_all();
> +            }

The reference to PCID in the comment isn't really meaningful imo.
PCID and INVPCID are independent features anyway. Also please
don't create artificially short comment lines.

Generally I also think such if() conditions would better be inverted:
There's no reason to make the legacy form look as if it was
preferred.

And then - what about the use in write_cr3() and the two uses that
remain after my XPTI follow-up series (which sadly looks to be stuck
for whatever reason), or (without that series) the write_cr3
assembler macro?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper]

On 05/03/18 11:31, Jan Beulich wrote:
>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> No description at all? I'd at least expect mention of how much of a
> performance win this is (for whichever hardware you happen to
> know that).
>
>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>          else
>>          {
>>              u32 t = pre_flush();
>> -            unsigned long cr4 = read_cr4();
>>  
>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>> -            barrier();
>> -            write_cr4(cr4);
>> +            if ( !cpu_has_invpcid )
>> +            {
>> +                unsigned long cr4 = read_cr4();
>> +
>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>> +                barrier();
>> +                write_cr4(cr4);
>> +            }
>> +            else
>> +            {
>> +                /*
>> +                 * Using invpcid to flush all mappings works
>> +                 * regardless of whether PCID is enabled or not.
>> +                 * It is faster than read-modify-write CR4.
>> +                 */

Its a cr4 double write, rather than RMW.  We read from a cached value
anyway, not from hardware.

>> +                invpcid_flush_all();
>> +            }
> The reference to PCID in the comment isn't really meaningful imo.
> PCID and INVPCID are independent features anyway. Also please
> don't create artificially short comment lines.
>
> Generally I also think such if() conditions would better be inverted:
> There's no reason to make the legacy form look as if it was
> preferred.
>
> And then - what about the use in write_cr3() and the two uses that
> remain after my XPTI follow-up series (which sadly looks to be stuck
> for whatever reason), or (without that series) the write_cr3
> assembler macro?

I don't think it is safe to use invpcid when we're also switching cr3. 
The new cr3 may have global pages with different translations, as they
are guest controlled.

(In fact - I'm considering using this property to try and allow us to
deliberately trigger MCEs on demand, for testing purposes).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 05/03/18 12:50, Andrew Cooper wrote:
> On 05/03/18 11:31, Jan Beulich wrote:
>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> No description at all? I'd at least expect mention of how much of a
>> performance win this is (for whichever hardware you happen to
>> know that).
>>
>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>>          else
>>>          {
>>>              u32 t = pre_flush();
>>> -            unsigned long cr4 = read_cr4();
>>>  
>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>> -            barrier();
>>> -            write_cr4(cr4);
>>> +            if ( !cpu_has_invpcid )
>>> +            {
>>> +                unsigned long cr4 = read_cr4();
>>> +
>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>> +                barrier();
>>> +                write_cr4(cr4);
>>> +            }
>>> +            else
>>> +            {
>>> +                /*
>>> +                 * Using invpcid to flush all mappings works
>>> +                 * regardless of whether PCID is enabled or not.
>>> +                 * It is faster than read-modify-write CR4.
>>> +                 */
> 
> Its a cr4 double write, rather than RMW.  We read from a cached value
> anyway, not from hardware.
> 
>>> +                invpcid_flush_all();
>>> +            }
>> The reference to PCID in the comment isn't really meaningful imo.
>> PCID and INVPCID are independent features anyway. Also please
>> don't create artificially short comment lines.
>>
>> Generally I also think such if() conditions would better be inverted:
>> There's no reason to make the legacy form look as if it was
>> preferred.
>>
>> And then - what about the use in write_cr3() and the two uses that
>> remain after my XPTI follow-up series (which sadly looks to be stuck
>> for whatever reason), or (without that series) the write_cr3
>> assembler macro?
> 
> I don't think it is safe to use invpcid when we're also switching cr3. 
> The new cr3 may have global pages with different translations, as they
> are guest controlled.

Can you elaborate a little bit more?

How can a guest control any hypervisor mappings? As long as the new cr3
is being loaded before the TLB is flushed via INVPCID I can't see how
a problem should occur.

In fact my series does exactly what Jan is asking above: it is replacing
the remaining cr4 based TLB flushing by INVPCID if possible. So in case
there is a flaw in my design please tell me.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper]

On 05/03/18 12:06, Juergen Gross wrote:
> On 05/03/18 12:50, Andrew Cooper wrote:
>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> No description at all? I'd at least expect mention of how much of a
>>> performance win this is (for whichever hardware you happen to
>>> know that).
>>>
>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>>>          else
>>>>          {
>>>>              u32 t = pre_flush();
>>>> -            unsigned long cr4 = read_cr4();
>>>>  
>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>> -            barrier();
>>>> -            write_cr4(cr4);
>>>> +            if ( !cpu_has_invpcid )
>>>> +            {
>>>> +                unsigned long cr4 = read_cr4();
>>>> +
>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>> +                barrier();
>>>> +                write_cr4(cr4);
>>>> +            }
>>>> +            else
>>>> +            {
>>>> +                /*
>>>> +                 * Using invpcid to flush all mappings works
>>>> +                 * regardless of whether PCID is enabled or not.
>>>> +                 * It is faster than read-modify-write CR4.
>>>> +                 */
>> Its a cr4 double write, rather than RMW.  We read from a cached value
>> anyway, not from hardware.
>>
>>>> +                invpcid_flush_all();
>>>> +            }
>>> The reference to PCID in the comment isn't really meaningful imo.
>>> PCID and INVPCID are independent features anyway. Also please
>>> don't create artificially short comment lines.
>>>
>>> Generally I also think such if() conditions would better be inverted:
>>> There's no reason to make the legacy form look as if it was
>>> preferred.
>>>
>>> And then - what about the use in write_cr3() and the two uses that
>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>> for whatever reason), or (without that series) the write_cr3
>>> assembler macro?
>> I don't think it is safe to use invpcid when we're also switching cr3. 
>> The new cr3 may have global pages with different translations, as they
>> are guest controlled.
> Can you elaborate a little bit more?
>
> How can a guest control any hypervisor mappings? As long as the new cr3
> is being loaded before the TLB is flushed via INVPCID I can't see how
> a problem should occur.
>
> In fact my series does exactly what Jan is asking above: it is replacing
> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
> there is a flaw in my design please tell me.

At the moment, we have guest and hypervisor controlled global mappings.

The current switch is:
cr4 &= ~PGE;
cr3 = new_cr3;
cr4 |= PGE;

which means that all global mappings are flushed by the first action,
and no new global mappings can come into existence.  We then switch to
the new cr3 (again with global fully disabled), then allow global
mappings to come back into existence.

With the invpcid route, we switch via:

cr3 = new_cr3;
invpcid all+global;

This has a race window where global mappings are active, and could
mismatch what is in cr3.  This yields #MC on at least some hardware, and
is specified to have undefined behaviour. 

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 13:35, <andrew.cooper3@citrix.com> wrote:
> On 05/03/18 12:06, Juergen Gross wrote:
>> On 05/03/18 12:50, Andrew Cooper wrote:
>>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>> No description at all? I'd at least expect mention of how much of a
>>>> performance win this is (for whichever hardware you happen to
>>>> know that).
>>>>
>>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned 
> int flags)
>>>>>          else
>>>>>          {
>>>>>              u32 t = pre_flush();
>>>>> -            unsigned long cr4 = read_cr4();
>>>>>  
>>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>>> -            barrier();
>>>>> -            write_cr4(cr4);
>>>>> +            if ( !cpu_has_invpcid )
>>>>> +            {
>>>>> +                unsigned long cr4 = read_cr4();
>>>>> +
>>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>>> +                barrier();
>>>>> +                write_cr4(cr4);
>>>>> +            }
>>>>> +            else
>>>>> +            {
>>>>> +                /*
>>>>> +                 * Using invpcid to flush all mappings works
>>>>> +                 * regardless of whether PCID is enabled or not.
>>>>> +                 * It is faster than read-modify-write CR4.
>>>>> +                 */
>>> Its a cr4 double write, rather than RMW.  We read from a cached value
>>> anyway, not from hardware.
>>>
>>>>> +                invpcid_flush_all();
>>>>> +            }
>>>> The reference to PCID in the comment isn't really meaningful imo.
>>>> PCID and INVPCID are independent features anyway. Also please
>>>> don't create artificially short comment lines.
>>>>
>>>> Generally I also think such if() conditions would better be inverted:
>>>> There's no reason to make the legacy form look as if it was
>>>> preferred.
>>>>
>>>> And then - what about the use in write_cr3() and the two uses that
>>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>>> for whatever reason), or (without that series) the write_cr3
>>>> assembler macro?
>>> I don't think it is safe to use invpcid when we're also switching cr3. 
>>> The new cr3 may have global pages with different translations, as they
>>> are guest controlled.
>> Can you elaborate a little bit more?
>>
>> How can a guest control any hypervisor mappings? As long as the new cr3
>> is being loaded before the TLB is flushed via INVPCID I can't see how
>> a problem should occur.
>>
>> In fact my series does exactly what Jan is asking above: it is replacing
>> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
>> there is a flaw in my design please tell me.
> 
> At the moment, we have guest and hypervisor controlled global mappings.
> 
> The current switch is:
> cr4 &= ~PGE;
> cr3 = new_cr3;
> cr4 |= PGE;
> 
> which means that all global mappings are flushed by the first action,
> and no new global mappings can come into existence.  We then switch to
> the new cr3 (again with global fully disabled), then allow global
> mappings to come back into existence.
> 
> With the invpcid route, we switch via:
> 
> cr3 = new_cr3;
> invpcid all+global;
> 
> This has a race window where global mappings are active, and could
> mismatch what is in cr3.  This yields #MC on at least some hardware, and
> is specified to have undefined behaviour. 

Oh, right, this would be okay only without what used to be named
USER_MAPPINGS_ARE_GLOBAL (and what is now implied).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper]

On 05/03/18 12:54, Jan Beulich wrote:
>>>> On 05.03.18 at 13:35, <andrew.cooper3@citrix.com> wrote:
>> On 05/03/18 12:06, Juergen Gross wrote:
>>> On 05/03/18 12:50, Andrew Cooper wrote:
>>>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>>> No description at all? I'd at least expect mention of how much of a
>>>>> performance win this is (for whichever hardware you happen to
>>>>> know that).
>>>>>
>>>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned 
>> int flags)
>>>>>>          else
>>>>>>          {
>>>>>>              u32 t = pre_flush();
>>>>>> -            unsigned long cr4 = read_cr4();
>>>>>>  
>>>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>> -            barrier();
>>>>>> -            write_cr4(cr4);
>>>>>> +            if ( !cpu_has_invpcid )
>>>>>> +            {
>>>>>> +                unsigned long cr4 = read_cr4();
>>>>>> +
>>>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>> +                barrier();
>>>>>> +                write_cr4(cr4);
>>>>>> +            }
>>>>>> +            else
>>>>>> +            {
>>>>>> +                /*
>>>>>> +                 * Using invpcid to flush all mappings works
>>>>>> +                 * regardless of whether PCID is enabled or not.
>>>>>> +                 * It is faster than read-modify-write CR4.
>>>>>> +                 */
>>>> Its a cr4 double write, rather than RMW.  We read from a cached value
>>>> anyway, not from hardware.
>>>>
>>>>>> +                invpcid_flush_all();
>>>>>> +            }
>>>>> The reference to PCID in the comment isn't really meaningful imo.
>>>>> PCID and INVPCID are independent features anyway. Also please
>>>>> don't create artificially short comment lines.
>>>>>
>>>>> Generally I also think such if() conditions would better be inverted:
>>>>> There's no reason to make the legacy form look as if it was
>>>>> preferred.
>>>>>
>>>>> And then - what about the use in write_cr3() and the two uses that
>>>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>>>> for whatever reason), or (without that series) the write_cr3
>>>>> assembler macro?
>>>> I don't think it is safe to use invpcid when we're also switching cr3. 
>>>> The new cr3 may have global pages with different translations, as they
>>>> are guest controlled.
>>> Can you elaborate a little bit more?
>>>
>>> How can a guest control any hypervisor mappings? As long as the new cr3
>>> is being loaded before the TLB is flushed via INVPCID I can't see how
>>> a problem should occur.
>>>
>>> In fact my series does exactly what Jan is asking above: it is replacing
>>> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
>>> there is a flaw in my design please tell me.
>> At the moment, we have guest and hypervisor controlled global mappings.
>>
>> The current switch is:
>> cr4 &= ~PGE;
>> cr3 = new_cr3;
>> cr4 |= PGE;
>>
>> which means that all global mappings are flushed by the first action,
>> and no new global mappings can come into existence.  We then switch to
>> the new cr3 (again with global fully disabled), then allow global
>> mappings to come back into existence.
>>
>> With the invpcid route, we switch via:
>>
>> cr3 = new_cr3;
>> invpcid all+global;
>>
>> This has a race window where global mappings are active, and could
>> mismatch what is in cr3.  This yields #MC on at least some hardware, and
>> is specified to have undefined behaviour. 
> Oh, right, this would be okay only without what used to be named
> USER_MAPPINGS_ARE_GLOBAL (and what is now implied).

When we start using PCID for user mappings, then we don't need them to
be global, at which point we can require/expect that the only global
mappings are hypervisor ones which we expect to remain correct across a
write to cr3.  However, if we do this, then we need to use a bit other
than PAGE_GLOBAL to signify guest user mappings.

I think this is doable, but I don't think it is going to be trivial to
get correct.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 05/03/18 13:57, Andrew Cooper wrote:
> On 05/03/18 12:54, Jan Beulich wrote:
>>>>> On 05.03.18 at 13:35, <andrew.cooper3@citrix.com> wrote:
>>> On 05/03/18 12:06, Juergen Gross wrote:
>>>> On 05/03/18 12:50, Andrew Cooper wrote:
>>>>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>>>> No description at all? I'd at least expect mention of how much of a
>>>>>> performance win this is (for whichever hardware you happen to
>>>>>> know that).
>>>>>>
>>>>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned 
>>> int flags)
>>>>>>>          else
>>>>>>>          {
>>>>>>>              u32 t = pre_flush();
>>>>>>> -            unsigned long cr4 = read_cr4();
>>>>>>>  
>>>>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>>> -            barrier();
>>>>>>> -            write_cr4(cr4);
>>>>>>> +            if ( !cpu_has_invpcid )
>>>>>>> +            {
>>>>>>> +                unsigned long cr4 = read_cr4();
>>>>>>> +
>>>>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>>>>> +                barrier();
>>>>>>> +                write_cr4(cr4);
>>>>>>> +            }
>>>>>>> +            else
>>>>>>> +            {
>>>>>>> +                /*
>>>>>>> +                 * Using invpcid to flush all mappings works
>>>>>>> +                 * regardless of whether PCID is enabled or not.
>>>>>>> +                 * It is faster than read-modify-write CR4.
>>>>>>> +                 */
>>>>> Its a cr4 double write, rather than RMW.  We read from a cached value
>>>>> anyway, not from hardware.
>>>>>
>>>>>>> +                invpcid_flush_all();
>>>>>>> +            }
>>>>>> The reference to PCID in the comment isn't really meaningful imo.
>>>>>> PCID and INVPCID are independent features anyway. Also please
>>>>>> don't create artificially short comment lines.
>>>>>>
>>>>>> Generally I also think such if() conditions would better be inverted:
>>>>>> There's no reason to make the legacy form look as if it was
>>>>>> preferred.
>>>>>>
>>>>>> And then - what about the use in write_cr3() and the two uses that
>>>>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>>>>> for whatever reason), or (without that series) the write_cr3
>>>>>> assembler macro?
>>>>> I don't think it is safe to use invpcid when we're also switching cr3. 
>>>>> The new cr3 may have global pages with different translations, as they
>>>>> are guest controlled.
>>>> Can you elaborate a little bit more?
>>>>
>>>> How can a guest control any hypervisor mappings? As long as the new cr3
>>>> is being loaded before the TLB is flushed via INVPCID I can't see how
>>>> a problem should occur.
>>>>
>>>> In fact my series does exactly what Jan is asking above: it is replacing
>>>> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
>>>> there is a flaw in my design please tell me.
>>> At the moment, we have guest and hypervisor controlled global mappings.
>>>
>>> The current switch is:
>>> cr4 &= ~PGE;
>>> cr3 = new_cr3;
>>> cr4 |= PGE;
>>>
>>> which means that all global mappings are flushed by the first action,
>>> and no new global mappings can come into existence.  We then switch to
>>> the new cr3 (again with global fully disabled), then allow global
>>> mappings to come back into existence.
>>>
>>> With the invpcid route, we switch via:
>>>
>>> cr3 = new_cr3;
>>> invpcid all+global;
>>>
>>> This has a race window where global mappings are active, and could
>>> mismatch what is in cr3.  This yields #MC on at least some hardware, and
>>> is specified to have undefined behaviour. 
>> Oh, right, this would be okay only without what used to be named
>> USER_MAPPINGS_ARE_GLOBAL (and what is now implied).
> 
> When we start using PCID for user mappings, then we don't need them to
> be global, at which point we can require/expect that the only global
> mappings are hypervisor ones which we expect to remain correct across a
> write to cr3.  However, if we do this, then we need to use a bit other
> than PAGE_GLOBAL to signify guest user mappings.
> 
> I think this is doable, but I don't think it is going to be trivial to
> get correct.

Why would we want to keep any global mappings at all? What are they good
for? Today the only case I could find where they make sense at all is
for 64-bit pv-guests to keep hypervisor mappings in the TLB when the
guest is switching between user and kernel mode. In all other cases the
complete TLB is flushed when cr3 is loaded with a new value (either due
to a Xen context switch or a guest cr3 load being performed by Xen).

With using PCID we can just keep cr4.pge being 0 and everything is fine.
No additional work required, we can just keep the G bit in the PTEs as a
flag which doesn't affect the TLB at all.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 14:11, <jgross@suse.com> wrote:
> On 05/03/18 13:57, Andrew Cooper wrote:
>> When we start using PCID for user mappings, then we don't need them to
>> be global, at which point we can require/expect that the only global
>> mappings are hypervisor ones which we expect to remain correct across a
>> write to cr3.  However, if we do this, then we need to use a bit other
>> than PAGE_GLOBAL to signify guest user mappings.
>> 
>> I think this is doable, but I don't think it is going to be trivial to
>> get correct.
> 
> Why would we want to keep any global mappings at all? What are they good
> for? Today the only case I could find where they make sense at all is
> for 64-bit pv-guests to keep hypervisor mappings in the TLB when the
> guest is switching between user and kernel mode.

Hypervisor and guest user mappings, that is.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Wei Liu]

On Mon, Mar 05, 2018 at 06:24:37AM -0700, Jan Beulich wrote:
> >>> On 05.03.18 at 14:11, <jgross@suse.com> wrote:
> > On 05/03/18 13:57, Andrew Cooper wrote:
> >> When we start using PCID for user mappings, then we don't need them to
> >> be global, at which point we can require/expect that the only global
> >> mappings are hypervisor ones which we expect to remain correct across a
> >> write to cr3.  However, if we do this, then we need to use a bit other
> >> than PAGE_GLOBAL to signify guest user mappings.
> >> 
> >> I think this is doable, but I don't think it is going to be trivial to
> >> get correct.
> > 
> > Why would we want to keep any global mappings at all? What are they good
> > for? Today the only case I could find where they make sense at all is
> > for 64-bit pv-guests to keep hypervisor mappings in the TLB when the
> > guest is switching between user and kernel mode.
> 
> Hypervisor and guest user mappings, that is.
> 

I'm not sure I understand the rationale behind global guest mappings. Is
it to keep guest user mappings when switching to guest kernel mode?

(A normal kernel like Linux makes global kernel mappings, but obv. we
can't do that in 64bit PV guests because kernel is also in ring3.)

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper]

On 05/03/18 13:31, Wei Liu wrote:
> On Mon, Mar 05, 2018 at 06:24:37AM -0700, Jan Beulich wrote:
>>>>> On 05.03.18 at 14:11, <jgross@suse.com> wrote:
>>> On 05/03/18 13:57, Andrew Cooper wrote:
>>>> When we start using PCID for user mappings, then we don't need them to
>>>> be global, at which point we can require/expect that the only global
>>>> mappings are hypervisor ones which we expect to remain correct across a
>>>> write to cr3.  However, if we do this, then we need to use a bit other
>>>> than PAGE_GLOBAL to signify guest user mappings.
>>>>
>>>> I think this is doable, but I don't think it is going to be trivial to
>>>> get correct.
>>> Why would we want to keep any global mappings at all? What are they good
>>> for? Today the only case I could find where they make sense at all is
>>> for 64-bit pv-guests to keep hypervisor mappings in the TLB when the
>>> guest is switching between user and kernel mode.
>> Hypervisor and guest user mappings, that is.
>>
> I'm not sure I understand the rationale behind global guest mappings. Is
> it to keep guest user mappings when switching to guest kernel mode?

Yes - it is a performance optimisation so on guest user => guest kernel
context switch, guest user mappings are still present in the TLB.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 14:31, <wei.liu2@citrix.com> wrote:
> On Mon, Mar 05, 2018 at 06:24:37AM -0700, Jan Beulich wrote:
>> >>> On 05.03.18 at 14:11, <jgross@suse.com> wrote:
>> > On 05/03/18 13:57, Andrew Cooper wrote:
>> >> When we start using PCID for user mappings, then we don't need them to
>> >> be global, at which point we can require/expect that the only global
>> >> mappings are hypervisor ones which we expect to remain correct across a
>> >> write to cr3.  However, if we do this, then we need to use a bit other
>> >> than PAGE_GLOBAL to signify guest user mappings.
>> >> 
>> >> I think this is doable, but I don't think it is going to be trivial to
>> >> get correct.
>> > 
>> > Why would we want to keep any global mappings at all? What are they good
>> > for? Today the only case I could find where they make sense at all is
>> > for 64-bit pv-guests to keep hypervisor mappings in the TLB when the
>> > guest is switching between user and kernel mode.
>> 
>> Hypervisor and guest user mappings, that is.
>> 
> 
> I'm not sure I understand the rationale behind global guest mappings. Is
> it to keep guest user mappings when switching to guest kernel mode?

Yes, exactly (and especially when - like for some syscalls - there's
a fast user->kernel->user round trip). The goal is to avoid at least
some of the TLB reloads, when we already can't have global guest
kernel mappings.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 05/03/18 13:35, Andrew Cooper wrote:
> On 05/03/18 12:06, Juergen Gross wrote:
>> On 05/03/18 12:50, Andrew Cooper wrote:
>>> On 05/03/18 11:31, Jan Beulich wrote:
>>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>> No description at all? I'd at least expect mention of how much of a
>>>> performance win this is (for whichever hardware you happen to
>>>> know that).
>>>>
>>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>>>>          else
>>>>>          {
>>>>>              u32 t = pre_flush();
>>>>> -            unsigned long cr4 = read_cr4();
>>>>>  
>>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>>> -            barrier();
>>>>> -            write_cr4(cr4);
>>>>> +            if ( !cpu_has_invpcid )
>>>>> +            {
>>>>> +                unsigned long cr4 = read_cr4();
>>>>> +
>>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>>> +                barrier();
>>>>> +                write_cr4(cr4);
>>>>> +            }
>>>>> +            else
>>>>> +            {
>>>>> +                /*
>>>>> +                 * Using invpcid to flush all mappings works
>>>>> +                 * regardless of whether PCID is enabled or not.
>>>>> +                 * It is faster than read-modify-write CR4.
>>>>> +                 */
>>> Its a cr4 double write, rather than RMW.  We read from a cached value
>>> anyway, not from hardware.
>>>
>>>>> +                invpcid_flush_all();
>>>>> +            }
>>>> The reference to PCID in the comment isn't really meaningful imo.
>>>> PCID and INVPCID are independent features anyway. Also please
>>>> don't create artificially short comment lines.
>>>>
>>>> Generally I also think such if() conditions would better be inverted:
>>>> There's no reason to make the legacy form look as if it was
>>>> preferred.
>>>>
>>>> And then - what about the use in write_cr3() and the two uses that
>>>> remain after my XPTI follow-up series (which sadly looks to be stuck
>>>> for whatever reason), or (without that series) the write_cr3
>>>> assembler macro?
>>> I don't think it is safe to use invpcid when we're also switching cr3. 
>>> The new cr3 may have global pages with different translations, as they
>>> are guest controlled.
>> Can you elaborate a little bit more?
>>
>> How can a guest control any hypervisor mappings? As long as the new cr3
>> is being loaded before the TLB is flushed via INVPCID I can't see how
>> a problem should occur.
>>
>> In fact my series does exactly what Jan is asking above: it is replacing
>> the remaining cr4 based TLB flushing by INVPCID if possible. So in case
>> there is a flaw in my design please tell me.
> 
> At the moment, we have guest and hypervisor controlled global mappings.
> 
> The current switch is:
> cr4 &= ~PGE;
> cr3 = new_cr3;
> cr4 |= PGE;
> 
> which means that all global mappings are flushed by the first action,
> and no new global mappings can come into existence.  We then switch to
> the new cr3 (again with global fully disabled), then allow global
> mappings to come back into existence.
> 
> With the invpcid route, we switch via:
> 
> cr3 = new_cr3;
> invpcid all+global;
> 
> This has a race window where global mappings are active, and could
> mismatch what is in cr3.  This yields #MC on at least some hardware, and
> is specified to have undefined behaviour. 

Okay, so I will modify my patch series to use INVPCID only when global
mappings are disabled. With PCID available I'll use it for the non-XPTI
case, too. This will enable us to disable global mappings completely, so
INVPCID can always be used for flushing the TLB.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>          else
>          {
>              u32 t = pre_flush();
> -            unsigned long cr4 = read_cr4();
>  
> -            write_cr4(cr4 & ~X86_CR4_PGE);
> -            barrier();
> -            write_cr4(cr4);
> +            if ( !cpu_has_invpcid )
> +            {
> +                unsigned long cr4 = read_cr4();
> +
> +                write_cr4(cr4 & ~X86_CR4_PGE);
> +                barrier();
> +                write_cr4(cr4);
> +            }
> +            else
> +            {
> +                /*
> +                 * Using invpcid to flush all mappings works
> +                 * regardless of whether PCID is enabled or not.
> +                 * It is faster than read-modify-write CR4.
> +                 */
> +                invpcid_flush_all();
> +            }

As just validly indicated by Jürgen, this is where my comment I
gave to one of his patches actually belongs: This is correct for
FLUSH_TLB_GLOBAL, but goes too far for FLUSH_TLB.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 09/03/18 16:29, Jan Beulich wrote:
>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>          else
>>          {
>>              u32 t = pre_flush();
>> -            unsigned long cr4 = read_cr4();
>>  
>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>> -            barrier();
>> -            write_cr4(cr4);
>> +            if ( !cpu_has_invpcid )
>> +            {
>> +                unsigned long cr4 = read_cr4();
>> +
>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>> +                barrier();
>> +                write_cr4(cr4);
>> +            }
>> +            else
>> +            {
>> +                /*
>> +                 * Using invpcid to flush all mappings works
>> +                 * regardless of whether PCID is enabled or not.
>> +                 * It is faster than read-modify-write CR4.
>> +                 */
>> +                invpcid_flush_all();
>> +            }
> 
> As just validly indicated by Jürgen, this is where my comment I
> gave to one of his patches actually belongs: This is correct for
> FLUSH_TLB_GLOBAL, but goes too far for FLUSH_TLB.

And again it was so even before this patch.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> Juergen Gross <jgross@suse.com> 03/09/18 7:05 PM >>>
>On 09/03/18 16:29, Jan Beulich wrote:
>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>>          else
>>>          {
>>>              u32 t = pre_flush();
>>> -            unsigned long cr4 = read_cr4();
>>>  
>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>> -            barrier();
>>> -            write_cr4(cr4);
>>> +            if ( !cpu_has_invpcid )
>>> +            {
>>> +                unsigned long cr4 = read_cr4();
>>> +
>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>> +                barrier();
>>> +                write_cr4(cr4);
>>> +            }
>>> +            else
>>> +            {
>>> +                /*
>>> +                 * Using invpcid to flush all mappings works
>>> +                 * regardless of whether PCID is enabled or not.
>>> +                 * It is faster than read-modify-write CR4.
>>> +                 */
>>> +                invpcid_flush_all();
>>> +            }
>> 
>> As just validly indicated by Jürgen, this is where my comment I
>> gave to one of his patches actually belongs: This is correct for
>> FLUSH_TLB_GLOBAL, but goes too far for FLUSH_TLB.
>
>And again it was so even before this patch.

Not exactly - "before this patch" should include the state things were in before
32-bit code got removed. And that's where we had a proper separation between
flushes including and excluding global entries. And now that we regain that
ability, we should leverage it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 12/03/18 13:59, Jan Beulich wrote:
>>>> Juergen Gross <jgross@suse.com> 03/09/18 7:05 PM >>>
>> On 09/03/18 16:29, Jan Beulich wrote:
>>>>>> On 05.03.18 at 10:50, <wei.liu2@citrix.com> wrote:
>>>> @@ -120,11 +121,24 @@ unsigned int flush_area_local(const void *va, unsigned int flags)
>>>>          else
>>>>          {
>>>>              u32 t = pre_flush();
>>>> -            unsigned long cr4 = read_cr4();
>>>>  
>>>> -            write_cr4(cr4 & ~X86_CR4_PGE);
>>>> -            barrier();
>>>> -            write_cr4(cr4);
>>>> +            if ( !cpu_has_invpcid )
>>>> +            {
>>>> +                unsigned long cr4 = read_cr4();
>>>> +
>>>> +                write_cr4(cr4 & ~X86_CR4_PGE);
>>>> +                barrier();
>>>> +                write_cr4(cr4);
>>>> +            }
>>>> +            else
>>>> +            {
>>>> +                /*
>>>> +                 * Using invpcid to flush all mappings works
>>>> +                 * regardless of whether PCID is enabled or not.
>>>> +                 * It is faster than read-modify-write CR4.
>>>> +                 */
>>>> +                invpcid_flush_all();
>>>> +            }
>>>
>>> As just validly indicated by Jürgen, this is where my comment I
>>> gave to one of his patches actually belongs: This is correct for
>>> FLUSH_TLB_GLOBAL, but goes too far for FLUSH_TLB.
>>
>> And again it was so even before this patch.
> 
> Not exactly - "before this patch" should include the state things were in before
> 32-bit code got removed. And that's where we had a proper separation between
> flushes including and excluding global entries. And now that we regain that
> ability, we should leverage it.

Already working on it in my XPTI speed-up series.

BTW: are you already working on rebasing your XPTI speed up series to
current staging? I'd like my series to use your series as a base unless
you are telling me you won't be able to resend your series soon.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> Juergen Gross <jgross@suse.com> 03/12/18 2:10 PM >>>
>BTW: are you already working on rebasing your XPTI speed up series to
>current staging? I'd like my series to use your series as a base unless
>you are telling me you won't be able to resend your series soon.

I hope to be able to get to this later this week.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 12/03/18 14:13, Jan Beulich wrote:
>>>> Juergen Gross <jgross@suse.com> 03/12/18 2:10 PM >>>
>> BTW: are you already working on rebasing your XPTI speed up series to
>> current staging? I'd like my series to use your series as a base unless
>> you are telling me you won't be able to resend your series soon.
> 
> I hope to be able to get to this later this week.

Okay, thanks.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
> BTW: are you already working on rebasing your XPTI speed up series to
> current staging? I'd like my series to use your series as a base unless
> you are telling me you won't be able to resend your series soon.

Coming back to my reply here yesterday - then again I'm a little
reluctant to send out a new version that has no changes other
than being re-based, when there were no comments by Andrew
on most of the remaining patches yet (and in the one case
where there were comments, I'm afraid I continue to disagree,
but I'll see about moving that patch last in the series).

Plus staging is broken right now anyway (but of course we know
now which patch to revert if need be).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 13/03/18 09:07, Jan Beulich wrote:
>>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
>> BTW: are you already working on rebasing your XPTI speed up series to
>> current staging? I'd like my series to use your series as a base unless
>> you are telling me you won't be able to resend your series soon.
> 
> Coming back to my reply here yesterday - then again I'm a little
> reluctant to send out a new version that has no changes other
> than being re-based, when there were no comments by Andrew
> on most of the remaining patches yet (and in the one case
> where there were comments, I'm afraid I continue to disagree,
> but I'll see about moving that patch last in the series).

In order to move forward, would you prefer me to base my patches on
current staging and put your patches on top of my series later? I just
don't want to rebase my series on a moving target...


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 13.03.18 at 10:27, <jgross@suse.com> wrote:
> On 13/03/18 09:07, Jan Beulich wrote:
>>>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
>>> BTW: are you already working on rebasing your XPTI speed up series to
>>> current staging? I'd like my series to use your series as a base unless
>>> you are telling me you won't be able to resend your series soon.
>> 
>> Coming back to my reply here yesterday - then again I'm a little
>> reluctant to send out a new version that has no changes other
>> than being re-based, when there were no comments by Andrew
>> on most of the remaining patches yet (and in the one case
>> where there were comments, I'm afraid I continue to disagree,
>> but I'll see about moving that patch last in the series).
> 
> In order to move forward, would you prefer me to base my patches on
> current staging and put your patches on top of my series later? I just
> don't want to rebase my series on a moving target...

Well, I'm similarly not intending to re-base onto a series still under
development / review. That's really the bad thing with deadlines:
If we didn't freeze the tree at a given time, but at the point where
previously agreed features and other non-bug-fix changes have
landed, we wouldn't have such an ordering problem right now (or
to be precise the ordering issue would still be there, but neither
of us would be at risk of their changes not making it).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 13/03/18 10:35, Jan Beulich wrote:
>>>> On 13.03.18 at 10:27, <jgross@suse.com> wrote:
>> On 13/03/18 09:07, Jan Beulich wrote:
>>>>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
>>>> BTW: are you already working on rebasing your XPTI speed up series to
>>>> current staging? I'd like my series to use your series as a base unless
>>>> you are telling me you won't be able to resend your series soon.
>>>
>>> Coming back to my reply here yesterday - then again I'm a little
>>> reluctant to send out a new version that has no changes other
>>> than being re-based, when there were no comments by Andrew
>>> on most of the remaining patches yet (and in the one case
>>> where there were comments, I'm afraid I continue to disagree,
>>> but I'll see about moving that patch last in the series).
>>
>> In order to move forward, would you prefer me to base my patches on
>> current staging and put your patches on top of my series later? I just
>> don't want to rebase my series on a moving target...
> 
> Well, I'm similarly not intending to re-base onto a series still under
> development / review. That's really the bad thing with deadlines:
> If we didn't freeze the tree at a given time, but at the point where
> previously agreed features and other non-bug-fix changes have
> landed, we wouldn't have such an ordering problem right now (or
> to be precise the ordering issue would still be there, but neither
> of us would be at risk of their changes not making it).

Understandable.

Could you then please repost at least patch 3? It has been approved
by Andrew and just needs the formal R-b: after rebasing.

Andrew, do you think you can find some time commenting on Jan's
other patches of his Meltdown series?

Speaking with my release manager hat on I'd _really_ like to have all
Meltdown/Spectre related patches minimizing the performance penalties
in 4.11.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 13.03.18 at 10:48, <jgross@suse.com> wrote:
> On 13/03/18 10:35, Jan Beulich wrote:
>>>>> On 13.03.18 at 10:27, <jgross@suse.com> wrote:
>>> On 13/03/18 09:07, Jan Beulich wrote:
>>>>>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
>>>>> BTW: are you already working on rebasing your XPTI speed up series to
>>>>> current staging? I'd like my series to use your series as a base unless
>>>>> you are telling me you won't be able to resend your series soon.
>>>>
>>>> Coming back to my reply here yesterday - then again I'm a little
>>>> reluctant to send out a new version that has no changes other
>>>> than being re-based, when there were no comments by Andrew
>>>> on most of the remaining patches yet (and in the one case
>>>> where there were comments, I'm afraid I continue to disagree,
>>>> but I'll see about moving that patch last in the series).
>>>
>>> In order to move forward, would you prefer me to base my patches on
>>> current staging and put your patches on top of my series later? I just
>>> don't want to rebase my series on a moving target...
>> 
>> Well, I'm similarly not intending to re-base onto a series still under
>> development / review. That's really the bad thing with deadlines:
>> If we didn't freeze the tree at a given time, but at the point where
>> previously agreed features and other non-bug-fix changes have
>> landed, we wouldn't have such an ordering problem right now (or
>> to be precise the ordering issue would still be there, but neither
>> of us would be at risk of their changes not making it).
> 
> Understandable.
> 
> Could you then please repost at least patch 3? It has been approved
> by Andrew and just needs the formal R-b: after rebasing.

Well, the thing with that patch is that from what I've been able
to tell so far its re-basing will consist of dropping it, moving its
sole remaining hunk (the altinstruction_nop assembler macro)
into what has been patch 4. None of the uses of the macro in
patch 3 should be needed anymore after Andrew's changes.
Or actually I think the macro won't be needed as a standalone
one anymore at all, as there's only a single place where it's
used, and hence it would likely better be folded into there (the
ALTERNATIVE_NOP one).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] x86: use invpcid to do global flushing

[Andrew Cooper]

On 13/03/2018 10:22, Jan Beulich wrote:
>>>> On 13.03.18 at 10:48, <jgross@suse.com> wrote:
>> On 13/03/18 10:35, Jan Beulich wrote:
>>>>>> On 13.03.18 at 10:27, <jgross@suse.com> wrote:
>>>> On 13/03/18 09:07, Jan Beulich wrote:
>>>>>>>> On 12.03.18 at 14:10, <jgross@suse.com> wrote:
>>>>>> BTW: are you already working on rebasing your XPTI speed up series to
>>>>>> current staging? I'd like my series to use your series as a base unless
>>>>>> you are telling me you won't be able to resend your series soon.
>>>>> Coming back to my reply here yesterday - then again I'm a little
>>>>> reluctant to send out a new version that has no changes other
>>>>> than being re-based, when there were no comments by Andrew
>>>>> on most of the remaining patches yet (and in the one case
>>>>> where there were comments, I'm afraid I continue to disagree,
>>>>> but I'll see about moving that patch last in the series).
>>>> In order to move forward, would you prefer me to base my patches on
>>>> current staging and put your patches on top of my series later? I just
>>>> don't want to rebase my series on a moving target...
>>> Well, I'm similarly not intending to re-base onto a series still under
>>> development / review. That's really the bad thing with deadlines:
>>> If we didn't freeze the tree at a given time, but at the point where
>>> previously agreed features and other non-bug-fix changes have
>>> landed, we wouldn't have such an ordering problem right now (or
>>> to be precise the ordering issue would still be there, but neither
>>> of us would be at risk of their changes not making it).
>> Understandable.
>>
>> Could you then please repost at least patch 3? It has been approved
>> by Andrew and just needs the formal R-b: after rebasing.
> Well, the thing with that patch is that from what I've been able
> to tell so far its re-basing will consist of dropping it, moving its
> sole remaining hunk (the altinstruction_nop assembler macro)
> into what has been patch 4. None of the uses of the macro in
> patch 3 should be needed anymore after Andrew's changes.
> Or actually I think the macro won't be needed as a standalone
> one anymore at all, as there's only a single place where it's
> used, and hence it would likely better be folded into there (the
> ALTERNATIVE_NOP one).

I'm sorry - I realise I'm very behind on reviews atm.  If you've got the
series rebased, then it will be easier to review this version than the
old version.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel