TLB flushing

TLB flushing

[Juergen Gross]

While hunting a strange bug in my PCID patch series hinting at some
TLB invalidation problem I discovered a piece of code looking rather
fishy to me.

Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
of FLUSH_TLB_GLOBAL?

While not being a problem in current code as both will flush all TLB
entries my series will change that by using invpcid to flush only the
non-global entries if FLUSH_TLB_GLOBAL wasn't set.

I can send a patch if anyone can confirm that using FLUSH_TLB only is
wrong.


Juergen

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

Re: TLB flushing

[Jan Beulich]

>>> On 20.03.18 at 09:50, <jgross@suse.com> wrote:
> While hunting a strange bug in my PCID patch series hinting at some
> TLB invalidation problem I discovered a piece of code looking rather
> fishy to me.
> 
> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
> of FLUSH_TLB_GLOBAL?
> 
> While not being a problem in current code as both will flush all TLB
> entries my series will change that by using invpcid to flush only the
> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
> 
> I can send a patch if anyone can confirm that using FLUSH_TLB only is
> wrong.

I think this shouldn't be a separate patch, but an integral part of the
one introducing the distinction between "all" and non-global flushes.
This is because
- right now it doesn't make a difference (we do "all" flushes anyway),
- back in the 32-bit days it didn't matter because guest mappings
  would never have been allowed to be global, and transient Xen
  mappings also would never have had the G bit set.
IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
this would need to become FLUSH_TLB_GLOBAL at the point the
kind of flush gets altered, while without it could remain at FLUSH_TLB.
Perhaps it is worthwhile to retain this distinction just for
documentation purposes (in case a future change wants to turn off
that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).

Jan


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

Re: TLB flushing

[Juergen Gross]

On 20/03/18 10:19, Jan Beulich wrote:
>>>> On 20.03.18 at 09:50, <jgross@suse.com> wrote:
>> While hunting a strange bug in my PCID patch series hinting at some
>> TLB invalidation problem I discovered a piece of code looking rather
>> fishy to me.
>>
>> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
>> of FLUSH_TLB_GLOBAL?
>>
>> While not being a problem in current code as both will flush all TLB
>> entries my series will change that by using invpcid to flush only the
>> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
>>
>> I can send a patch if anyone can confirm that using FLUSH_TLB only is
>> wrong.
> 
> I think this shouldn't be a separate patch, but an integral part of the
> one introducing the distinction between "all" and non-global flushes.
> This is because
> - right now it doesn't make a difference (we do "all" flushes anyway),
> - back in the 32-bit days it didn't matter because guest mappings
>   would never have been allowed to be global, and transient Xen
>   mappings also would never have had the G bit set.
> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
> this would need to become FLUSH_TLB_GLOBAL at the point the
> kind of flush gets altered, while without it could remain at FLUSH_TLB.

Really? Aren't global hypervisor mappings affected by this, too?

> Perhaps it is worthwhile to retain this distinction just for
> documentation purposes (in case a future change wants to turn off
> that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).

I think as long as FLUSH_TLB_GLOBAL is being used in the code
new_tlbflush_clock_period() should do so, too. In case there is no need
to do TLB flushes including global pages, FLUSH_TLB_GLOBAL can be
modified to do only non-global flushing (with a comment explaining why
this is secure).


Juergen

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

Re: TLB flushing

[Jan Beulich]

>>> On 20.03.18 at 10:29, <jgross@suse.com> wrote:
> On 20/03/18 10:19, Jan Beulich wrote:
>>>>> On 20.03.18 at 09:50, <jgross@suse.com> wrote:
>>> While hunting a strange bug in my PCID patch series hinting at some
>>> TLB invalidation problem I discovered a piece of code looking rather
>>> fishy to me.
>>>
>>> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
>>> of FLUSH_TLB_GLOBAL?
>>>
>>> While not being a problem in current code as both will flush all TLB
>>> entries my series will change that by using invpcid to flush only the
>>> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
>>>
>>> I can send a patch if anyone can confirm that using FLUSH_TLB only is
>>> wrong.
>> 
>> I think this shouldn't be a separate patch, but an integral part of the
>> one introducing the distinction between "all" and non-global flushes.
>> This is because
>> - right now it doesn't make a difference (we do "all" flushes anyway),
>> - back in the 32-bit days it didn't matter because guest mappings
>>   would never have been allowed to be global, and transient Xen
>>   mappings also would never have had the G bit set.
>> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
>> this would need to become FLUSH_TLB_GLOBAL at the point the
>> kind of flush gets altered, while without it could remain at FLUSH_TLB.
> 
> Really? Aren't global hypervisor mappings affected by this, too?

Yes and no. The timestamp here is needed to know whether to
flush when a page gets recycled. As long as G is never set on
guest controlled mappings for pages which may be recycled,
there's no issue. In particular, the G bits in the 1:1 mappings are
of no interest here (and in fact anything Xen maintains which no
guest can control), as those mappings never go away (or if they
did, e.g. when a page needs to be offlined for causing #MC, an
explicit global flush for that page would be required).

>> Perhaps it is worthwhile to retain this distinction just for
>> documentation purposes (in case a future change wants to turn off
>> that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).
> 
> I think as long as FLUSH_TLB_GLOBAL is being used in the code
> new_tlbflush_clock_period() should do so, too. In case there is no need
> to do TLB flushes including global pages, FLUSH_TLB_GLOBAL can be
> modified to do only non-global flushing (with a comment explaining why
> this is secure).

No - an explicit global flush request (as per above explanation)
must never be converted down to a non-global one. Such a
"conversion" would only be valid when CR4.PGE is clear at all
times (but then it's not really a conversion anymore).

Jan


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

Re: TLB flushing

[Juergen Gross]

On 20/03/18 10:56, Jan Beulich wrote:
>>>> On 20.03.18 at 10:29, <jgross@suse.com> wrote:
>> On 20/03/18 10:19, Jan Beulich wrote:
>>>>>> On 20.03.18 at 09:50, <jgross@suse.com> wrote:
>>>> While hunting a strange bug in my PCID patch series hinting at some
>>>> TLB invalidation problem I discovered a piece of code looking rather
>>>> fishy to me.
>>>>
>>>> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
>>>> of FLUSH_TLB_GLOBAL?
>>>>
>>>> While not being a problem in current code as both will flush all TLB
>>>> entries my series will change that by using invpcid to flush only the
>>>> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
>>>>
>>>> I can send a patch if anyone can confirm that using FLUSH_TLB only is
>>>> wrong.
>>>
>>> I think this shouldn't be a separate patch, but an integral part of the
>>> one introducing the distinction between "all" and non-global flushes.
>>> This is because
>>> - right now it doesn't make a difference (we do "all" flushes anyway),
>>> - back in the 32-bit days it didn't matter because guest mappings
>>>   would never have been allowed to be global, and transient Xen
>>>   mappings also would never have had the G bit set.
>>> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
>>> this would need to become FLUSH_TLB_GLOBAL at the point the
>>> kind of flush gets altered, while without it could remain at FLUSH_TLB.
>>
>> Really? Aren't global hypervisor mappings affected by this, too?
> 
> Yes and no. The timestamp here is needed to know whether to
> flush when a page gets recycled. As long as G is never set on
> guest controlled mappings for pages which may be recycled,
> there's no issue. In particular, the G bits in the 1:1 mappings are
> of no interest here (and in fact anything Xen maintains which no
> guest can control), as those mappings never go away (or if they
> did, e.g. when a page needs to be offlined for causing #MC, an
> explicit global flush for that page would be required).

I just verified that there is at least one problem in the hypervisor
TLB flushing logic: using invpcid it is possible to flush the non-global
entries only. If I do that in case of FLUSH_TLB_GLOBAL not being set I
get segfaults in user mode of the guest.


Juergen

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

TLB flushing

[Syed Rafiul Hussain]

Hi,

I've modified a page in kernel space as non-writable and did tlb flushing.
However, even after making the page non-writable and tlb flushing, I am
still able to write on that page. Has anyone faced similar kind of problem?
Is it a problem of tlb flushing? I would really appreciate if someone could
help me in this regard.


Thanks.

Syed Rafiul Hussain
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120710/8ea704e2/attachment-0001.html>

Re: TLB flushing

[Keir Fraser]

On 23/1/07 18:28, "Ky Srinivasan" <ksrinivasan@novell.com> wrote:

> Looking at the do_mmuext_op(), it looks like we are not handling shadow mode
> uniformly :
> For instance, for MMUEXT_INVLPG_LOCAL case, we deal with the case where the
> domain may be operating under shadow mode; however, for other flush options we
> don't deal with the case where the domain may be operating in shadow mode.
> What is the rationale for this?

Most (or perhaps even all) of the references to shadow mode in arch/x86/mm.c
are to handle PV guests running in shadow_mode_translate and/or
shadow_mode_refcounts. Those modes were never really a well-integrated or
-tested configuration for PV guests and are now deprecated (e.g., reference
to shadow mode in MMUEXT_INVLPG_LOCAL will be removed sometime soon). Really
most of arch/x86/mm.c needs to be renamed to arch/x86/mm/pv.c...

 -- Keir

TLB flushing

[Ky Srinivasan]

Looking at the do_mmuext_op(), it looks like we are not handling shadow mode uniformly :
For instance, for MMUEXT_INVLPG_LOCAL case, we deal with the case where the domain may be operating under shadow mode; however, for other flush options we don't deal with the case where the domain may be operating in shadow mode. What is the rationale for this?

Regards,

K. Y