Re: [Xen-devel] [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,  wrote:
>> On 13/03/18 10:35, Jan Beulich wrote:
>> On 13.03.18 at 10:27,  wrote:
 On 13/03/18 09:07, Jan Beulich wrote:
 On 12.03.18 at 14:10,  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

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

[Jan Beulich]

>>> On 13.03.18 at 10:48,  wrote:
> On 13/03/18 10:35, Jan Beulich wrote:
> On 13.03.18 at 10:27,  wrote:
>>> On 13/03/18 09:07, Jan Beulich wrote:
>>> On 12.03.18 at 14:10,  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: [Xen-devel] [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,  wrote:
>> On 13/03/18 09:07, Jan Beulich wrote:
>> On 12.03.18 at 14:10,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 13.03.18 at 10:27,  wrote:
> On 13/03/18 09:07, Jan Beulich wrote:
> On 12.03.18 at 14:10,  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: [Xen-devel] [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,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 12.03.18 at 14:10,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 12/03/18 14:13, Jan Beulich wrote:
 Juergen Gross  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> Juergen Gross  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Juergen Gross]

On 12/03/18 13:59, Jan Beulich wrote:
 Juergen Gross  03/09/18 7:05 PM >>>
>> On 09/03/18 16:29, Jan Beulich wrote:
>> On 05.03.18 at 10:50,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> Juergen Gross  03/09/18 7:05 PM >>>
>On 09/03/18 16:29, Jan Beulich wrote:
> On 05.03.18 at 10:50,  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: [Xen-devel] [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,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 10:50,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 14:31,  wrote:
> On Mon, Mar 05, 2018 at 06:24:37AM -0700, Jan Beulich wrote:
>> >>> On 05.03.18 at 14:11,  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: [Xen-devel] [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,  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: [Xen-devel] [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,  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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 14:11,  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: [Xen-devel] [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,  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,  wrote:
>>> Signed-off-by: Wei Liu 
>> 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

Re: [Xen-devel] [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,  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,  wrote:
>> Signed-off-by: Wei Liu 
> 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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 13:35,  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,  wrote:
> Signed-off-by: Wei Liu 
 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: [Xen-devel] [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,  wrote:
 Signed-off-by: Wei Liu 
>>> 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: [Xen-devel] [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,  wrote:
>>> Signed-off-by: Wei Liu 
>> 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: [Xen-devel] [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,  wrote:
>> Signed-off-by: Wei Liu 
> 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: [Xen-devel] [PATCH 2/2] x86: use invpcid to do global flushing

[Jan Beulich]

>>> On 05.03.18 at 10:50,  wrote:
> Signed-off-by: Wei Liu 

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: [Xen-devel] [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 

Reviewed-by: Juergen Gross 


Juergen

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