Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 13/03/2018 14:20, Vitaly Kuznetsov wrote:
> vmcs_writel() I guess ... and, just to make sure I follow your
> suggestion, this is for x86_64 only, right? x86_32 does
> 
> vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));
> 
> and I think it needs to stay.

Yes.

> (personally, I'm rather for exporting save_fsgs(), dropping
> savesegment() and getting all we need from current to avoid propagating
> assumptions but I'm flexible)

Yes, that's fine too, as long as it's not mix and match. :)

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 13/03/2018 14:20, Vitaly Kuznetsov wrote:
> vmcs_writel() I guess ... and, just to make sure I follow your
> suggestion, this is for x86_64 only, right? x86_32 does
> 
> vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));
> 
> and I think it needs to stay.

Yes.

> (personally, I'm rather for exporting save_fsgs(), dropping
> savesegment() and getting all we need from current to avoid propagating
> assumptions but I'm flexible)

Yes, that's fine too, as long as it's not mix and match. :)

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Paolo Bonzini  writes:

> On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>>  
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>> +void save_current_fsgs(void)
>> +{
>> +save_fsgs(current);
>> +}
>> +EXPORT_SYMBOL_GPL(save_current_fsgs);
>
> We don't really need save_fsgs in KVM though.  We already do the
> savesegment ourselves, and we know Intel CPUs don't have
> X86_BUG_NULL_SEG.  So this:
>
> savesegment(fs, vmx->host_state.fs_sel);
> if (!(vmx->host_state.fs_sel & 7)) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
>   ...
>
> could probably become simply:
>
> savesegment(fs, vmx->host_state.fs_sel);
>   /*
>* When FSGSBASE extensions are enabled, this will have to use
>* RD{FS,GS}BASE instead of accessing current, and the
>* corresponding WR{FS,GS}BASE should be done unconditionally,
>* even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>*/
> if (vmx->host_state.fs_sel <= 3) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);

vmcs_writel() I guess ... and, just to make sure I follow your
suggestion, this is for x86_64 only, right? x86_32 does

vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));

and I think it needs to stay.

(personally, I'm rather for exporting save_fsgs(), dropping
savesegment() and getting all we need from current to avoid propagating
assumptions but I'm flexible)

-- 
  Vitaly

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Paolo Bonzini  writes:

> On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>>  
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>> +void save_current_fsgs(void)
>> +{
>> +save_fsgs(current);
>> +}
>> +EXPORT_SYMBOL_GPL(save_current_fsgs);
>
> We don't really need save_fsgs in KVM though.  We already do the
> savesegment ourselves, and we know Intel CPUs don't have
> X86_BUG_NULL_SEG.  So this:
>
> savesegment(fs, vmx->host_state.fs_sel);
> if (!(vmx->host_state.fs_sel & 7)) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
>   ...
>
> could probably become simply:
>
> savesegment(fs, vmx->host_state.fs_sel);
>   /*
>* When FSGSBASE extensions are enabled, this will have to use
>* RD{FS,GS}BASE instead of accessing current, and the
>* corresponding WR{FS,GS}BASE should be done unconditionally,
>* even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>*/
> if (vmx->host_state.fs_sel <= 3) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);

vmcs_writel() I guess ... and, just to make sure I follow your
suggestion, this is for x86_64 only, right? x86_32 does

vmcs_writel(HOST_FS_BASE, segment_base(vmx->host_state.fs_sel));

and I think it needs to stay.

(personally, I'm rather for exporting save_fsgs(), dropping
savesegment() and getting all we need from current to avoid propagating
assumptions but I'm flexible)

-- 
  Vitaly

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 18:00, Andy Lutomirski wrote:
>> Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
>> save_fsgs?  (Or if not I'm missing what the comment should be about).
> It could be in save_fsgs(), I guess.  The main point is to make it
> clear to readers of the code in save_fsgs(), the legacy helpers, etc
> that there's another piece of code in KVM that makes the same set of
> somewhat problematic assumptions and that will need updating for
> FSGSBASE.

Okay, that's a good idea.

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 18:00, Andy Lutomirski wrote:
>> Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
>> save_fsgs?  (Or if not I'm missing what the comment should be about).
> It could be in save_fsgs(), I guess.  The main point is to make it
> clear to readers of the code in save_fsgs(), the legacy helpers, etc
> that there's another piece of code in KVM that makes the same set of
> somewhat problematic assumptions and that will need updating for
> FSGSBASE.

Okay, that's a good idea.

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 4:18 PM, Paolo Bonzini  wrote:
> On 12/03/2018 17:13, Andy Lutomirski wrote:
>>>
>>> savesegment(fs, vmx->host_state.fs_sel);
>>> /*
>>>  * When FSGSBASE extensions are enabled, this will have to use
>>>  * RD{FS,GS}BASE instead of accessing current, and the
>>>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>>>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>>>  */
>>> if (vmx->host_state.fs_sel <= 3) {
>>> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
>>> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
>>> vmx->host_state.fs_reload_needed = 0;
>>> } else {
>>> vmcs_write16(HOST_FS_SELECTOR, 0);
>>> vmcs_write16(HOST_FS_BASE, 0);
>>> vmx->host_state.fs_reload_needed = 1;
>>> }
>>> savesegment(gs, vmx->host_state.gs_sel);
>>> ...
>>>
>>> ?
>>>
>> Hmm, probably, although this still gets the case where the user writes
>> 0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.
>>
>> I'm okay with this variant as long as you add a comment to
>> save_..._legacy pointing at the KVM code.
>
> Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
> save_fsgs?  (Or if not I'm missing what the comment should be about).

It could be in save_fsgs(), I guess.  The main point is to make it
clear to readers of the code in save_fsgs(), the legacy helpers, etc
that there's another piece of code in KVM that makes the same set of
somewhat problematic assumptions and that will need updating for
FSGSBASE.

I'm moderately confident that someone from Intel is currently working
on FSGSBASE, but all I've seen lately is a bunch of kbuild bot errors
:(

--Amdy

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 4:18 PM, Paolo Bonzini  wrote:
> On 12/03/2018 17:13, Andy Lutomirski wrote:
>>>
>>> savesegment(fs, vmx->host_state.fs_sel);
>>> /*
>>>  * When FSGSBASE extensions are enabled, this will have to use
>>>  * RD{FS,GS}BASE instead of accessing current, and the
>>>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>>>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>>>  */
>>> if (vmx->host_state.fs_sel <= 3) {
>>> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
>>> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
>>> vmx->host_state.fs_reload_needed = 0;
>>> } else {
>>> vmcs_write16(HOST_FS_SELECTOR, 0);
>>> vmcs_write16(HOST_FS_BASE, 0);
>>> vmx->host_state.fs_reload_needed = 1;
>>> }
>>> savesegment(gs, vmx->host_state.gs_sel);
>>> ...
>>>
>>> ?
>>>
>> Hmm, probably, although this still gets the case where the user writes
>> 0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.
>>
>> I'm okay with this variant as long as you add a comment to
>> save_..._legacy pointing at the KVM code.
>
> Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
> save_fsgs?  (Or if not I'm missing what the comment should be about).

It could be in save_fsgs(), I guess.  The main point is to make it
clear to readers of the code in save_fsgs(), the legacy helpers, etc
that there's another piece of code in KVM that makes the same set of
somewhat problematic assumptions and that will need updating for
FSGSBASE.

I'm moderately confident that someone from Intel is currently working
on FSGSBASE, but all I've seen lately is a bunch of kbuild bot errors
:(

--Amdy

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 17:13, Andy Lutomirski wrote:
>>
>> savesegment(fs, vmx->host_state.fs_sel);
>> /*
>>  * When FSGSBASE extensions are enabled, this will have to use
>>  * RD{FS,GS}BASE instead of accessing current, and the
>>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>>  */
>> if (vmx->host_state.fs_sel <= 3) {
>> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
>> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
>> vmx->host_state.fs_reload_needed = 0;
>> } else {
>> vmcs_write16(HOST_FS_SELECTOR, 0);
>> vmcs_write16(HOST_FS_BASE, 0);
>> vmx->host_state.fs_reload_needed = 1;
>> }
>> savesegment(gs, vmx->host_state.gs_sel);
>> ...
>>
>> ?
>>
> Hmm, probably, although this still gets the case where the user writes
> 0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.
> 
> I'm okay with this variant as long as you add a comment to
> save_..._legacy pointing at the KVM code.

Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
save_fsgs?  (Or if not I'm missing what the comment should be about).

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 17:13, Andy Lutomirski wrote:
>>
>> savesegment(fs, vmx->host_state.fs_sel);
>> /*
>>  * When FSGSBASE extensions are enabled, this will have to use
>>  * RD{FS,GS}BASE instead of accessing current, and the
>>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>>  */
>> if (vmx->host_state.fs_sel <= 3) {
>> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
>> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
>> vmx->host_state.fs_reload_needed = 0;
>> } else {
>> vmcs_write16(HOST_FS_SELECTOR, 0);
>> vmcs_write16(HOST_FS_BASE, 0);
>> vmx->host_state.fs_reload_needed = 1;
>> }
>> savesegment(gs, vmx->host_state.gs_sel);
>> ...
>>
>> ?
>>
> Hmm, probably, although this still gets the case where the user writes
> 0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.
> 
> I'm okay with this variant as long as you add a comment to
> save_..._legacy pointing at the KVM code.

Why in save_..._legacy?  If it is about FSGSBASE, shouldn't it be in
save_fsgs?  (Or if not I'm missing what the comment should be about).

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 4:03 PM, Paolo Bonzini  wrote:
> On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>>
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>> +void save_current_fsgs(void)
>> +{
>> + save_fsgs(current);
>> +}
>> +EXPORT_SYMBOL_GPL(save_current_fsgs);
>
> We don't really need save_fsgs in KVM though.  We already do the
> savesegment ourselves, and we know Intel CPUs don't have
> X86_BUG_NULL_SEG.  So this:
>
> savesegment(fs, vmx->host_state.fs_sel);
> if (!(vmx->host_state.fs_sel & 7)) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
> ...
>
> could probably become simply:
>
> savesegment(fs, vmx->host_state.fs_sel);
> /*
>  * When FSGSBASE extensions are enabled, this will have to use
>  * RD{FS,GS}BASE instead of accessing current, and the
>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>  */
> if (vmx->host_state.fs_sel <= 3) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmcs_write16(HOST_FS_BASE, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
> ...
>
> ?
>

Hmm, probably, although this still gets the case where the user writes
0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.

I'm okay with this variant as long as you add a comment to
save_..._legacy pointing at the KVM code.

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 4:03 PM, Paolo Bonzini  wrote:
> On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>>
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>> +void save_current_fsgs(void)
>> +{
>> + save_fsgs(current);
>> +}
>> +EXPORT_SYMBOL_GPL(save_current_fsgs);
>
> We don't really need save_fsgs in KVM though.  We already do the
> savesegment ourselves, and we know Intel CPUs don't have
> X86_BUG_NULL_SEG.  So this:
>
> savesegment(fs, vmx->host_state.fs_sel);
> if (!(vmx->host_state.fs_sel & 7)) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
> ...
>
> could probably become simply:
>
> savesegment(fs, vmx->host_state.fs_sel);
> /*
>  * When FSGSBASE extensions are enabled, this will have to use
>  * RD{FS,GS}BASE instead of accessing current, and the
>  * corresponding WR{FS,GS}BASE should be done unconditionally,
>  * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
>  */
> if (vmx->host_state.fs_sel <= 3) {
> vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
> vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
> vmx->host_state.fs_reload_needed = 0;
> } else {
> vmcs_write16(HOST_FS_SELECTOR, 0);
> vmcs_write16(HOST_FS_BASE, 0);
> vmx->host_state.fs_reload_needed = 1;
> }
> savesegment(gs, vmx->host_state.gs_sel);
> ...
>
> ?
>

Hmm, probably, although this still gets the case where the user writes
0 to %fs wrong.  Of course, save_fsgs() also gets that wrong.

I'm okay with this variant as long as you add a comment to
save_..._legacy pointing at the KVM code.

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>  
> +/*
> + * Currently, the only way for processes to change their FS/GS base is to 
> call
> + * ARCH_SET_FS/GS prctls and these reflect changes they make in task->thread.
> + * There are, however, additional considerations:
> + *
> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
> zeroes
> + * the base and on some it doesn't, we need to check for that
> + * (see save_base_legacy()).
> + *
> + * When FSGSBASE extensions are enabled userspace processes will be able to
> + * change their FS/GS bases without kernel intervention. save_fsgs() will
> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
> + * instructions.
> + */
> +void save_current_fsgs(void)
> +{
> + save_fsgs(current);
> +}
> +EXPORT_SYMBOL_GPL(save_current_fsgs);

We don't really need save_fsgs in KVM though.  We already do the
savesegment ourselves, and we know Intel CPUs don't have
X86_BUG_NULL_SEG.  So this:

savesegment(fs, vmx->host_state.fs_sel);
if (!(vmx->host_state.fs_sel & 7)) {
vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
vmx->host_state.fs_reload_needed = 0;
} else {
vmcs_write16(HOST_FS_SELECTOR, 0);
vmx->host_state.fs_reload_needed = 1;
}
savesegment(gs, vmx->host_state.gs_sel);
...

could probably become simply:

savesegment(fs, vmx->host_state.fs_sel);
/*
 * When FSGSBASE extensions are enabled, this will have to use
 * RD{FS,GS}BASE instead of accessing current, and the
 * corresponding WR{FS,GS}BASE should be done unconditionally,
 * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
 */
if (vmx->host_state.fs_sel <= 3) {
vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
vmx->host_state.fs_reload_needed = 0;
} else {
vmcs_write16(HOST_FS_SELECTOR, 0);
vmcs_write16(HOST_FS_BASE, 0);
vmx->host_state.fs_reload_needed = 1;
}
savesegment(gs, vmx->host_state.gs_sel);
...

?

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Paolo Bonzini]

On 12/03/2018 15:02, Vitaly Kuznetsov wrote:
>  
> +/*
> + * Currently, the only way for processes to change their FS/GS base is to 
> call
> + * ARCH_SET_FS/GS prctls and these reflect changes they make in task->thread.
> + * There are, however, additional considerations:
> + *
> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
> zeroes
> + * the base and on some it doesn't, we need to check for that
> + * (see save_base_legacy()).
> + *
> + * When FSGSBASE extensions are enabled userspace processes will be able to
> + * change their FS/GS bases without kernel intervention. save_fsgs() will
> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
> + * instructions.
> + */
> +void save_current_fsgs(void)
> +{
> + save_fsgs(current);
> +}
> +EXPORT_SYMBOL_GPL(save_current_fsgs);

We don't really need save_fsgs in KVM though.  We already do the
savesegment ourselves, and we know Intel CPUs don't have
X86_BUG_NULL_SEG.  So this:

savesegment(fs, vmx->host_state.fs_sel);
if (!(vmx->host_state.fs_sel & 7)) {
vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
vmx->host_state.fs_reload_needed = 0;
} else {
vmcs_write16(HOST_FS_SELECTOR, 0);
vmx->host_state.fs_reload_needed = 1;
}
savesegment(gs, vmx->host_state.gs_sel);
...

could probably become simply:

savesegment(fs, vmx->host_state.fs_sel);
/*
 * When FSGSBASE extensions are enabled, this will have to use
 * RD{FS,GS}BASE instead of accessing current, and the
 * corresponding WR{FS,GS}BASE should be done unconditionally,
 * even if fs_reload_needed (resp. gs_ldt_reload_needed) is 1.
 */
if (vmx->host_state.fs_sel <= 3) {
vmcs_write16(HOST_FS_SELECTOR, vmx->host_state.fs_sel);
vmcs_write16(HOST_FS_BASE, current->thread.fsbase);
vmx->host_state.fs_reload_needed = 0;
} else {
vmcs_write16(HOST_FS_SELECTOR, 0);
vmcs_write16(HOST_FS_BASE, 0);
vmx->host_state.fs_reload_needed = 1;
}
savesegment(gs, vmx->host_state.gs_sel);
...

?

Paolo

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Andy Lutomirski  writes:

> On Mon, Mar 12, 2018 at 2:02 PM, Vitaly Kuznetsov  wrote:
>> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
>> the context is pretty well defined. Read MSR_FS_BASE from
>> current->thread.fsbase after calling save_fsgs() which takes care of
>> X86_BUG_NULL_SEG case now and will do RD[FG,GS]BASE when FSGSBASE
>> extensions are exposed to userspace (currently they are not).
>>
>> Signed-off-by: Vitaly Kuznetsov 
>> ---
>>  arch/x86/include/asm/processor.h |  3 +++
>>  arch/x86/kernel/process_64.c | 20 
>>  arch/x86/kvm/vmx.c   |  4 +++-
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h 
>> b/arch/x86/include/asm/processor.h
>> index b0ccd4847a58..006352b85ba3 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -410,6 +410,9 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
>>  DECLARE_PER_CPU(char *, irq_stack_ptr);
>>  DECLARE_PER_CPU(unsigned int, irq_count);
>>  extern asmlinkage void ignore_sysret(void);
>> +
>> +/* Save actual FS/GS selectors and bases to current->thread */
>> +void save_current_fsgs(void);
>>  #else  /* X86_64 */
>>  #ifdef CONFIG_CC_STACKPROTECTOR
>>  /*
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 9eb448c7859d..eb907fefe02e 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -205,6 +205,26 @@ static __always_inline void save_fsgs(struct 
>> task_struct *task)
>> save_base_legacy(task, task->thread.gsindex, GS);
>>  }
>>
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>
> This is all a very complicated way to say "while a process is running,
> current->thread.fsbase and current->thread.gsbase may not match the
> corresponding CPU registers.  KVM wants an efficient way to save and
> restore FSBASE and GSBASE."
>

Well, I though it is not really obvious why "current->thread.fsbase and
current->thread.gsbase may not match the corresponding CPU registers"
:-) but save_base_legacy() is really nearby so I'll trim my comment in
v2.

> And how about changing this to:
>
> #if IS_ENABLED(CONFIG_KVM)
> void save_fsgs_for_kvm(void)
> {
> save_fsgs(current);
> }
> EXPORT_SYMBOL_GPL(save_fsgs_for_kvm);
>

Sure. Actually, there is nothing KVM-specific in this function but KVM
will probably be the only user for the time being.

-- 
  Vitaly

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Vitaly Kuznetsov]

Andy Lutomirski  writes:

> On Mon, Mar 12, 2018 at 2:02 PM, Vitaly Kuznetsov  wrote:
>> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
>> the context is pretty well defined. Read MSR_FS_BASE from
>> current->thread.fsbase after calling save_fsgs() which takes care of
>> X86_BUG_NULL_SEG case now and will do RD[FG,GS]BASE when FSGSBASE
>> extensions are exposed to userspace (currently they are not).
>>
>> Signed-off-by: Vitaly Kuznetsov 
>> ---
>>  arch/x86/include/asm/processor.h |  3 +++
>>  arch/x86/kernel/process_64.c | 20 
>>  arch/x86/kvm/vmx.c   |  4 +++-
>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/processor.h 
>> b/arch/x86/include/asm/processor.h
>> index b0ccd4847a58..006352b85ba3 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -410,6 +410,9 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
>>  DECLARE_PER_CPU(char *, irq_stack_ptr);
>>  DECLARE_PER_CPU(unsigned int, irq_count);
>>  extern asmlinkage void ignore_sysret(void);
>> +
>> +/* Save actual FS/GS selectors and bases to current->thread */
>> +void save_current_fsgs(void);
>>  #else  /* X86_64 */
>>  #ifdef CONFIG_CC_STACKPROTECTOR
>>  /*
>> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
>> index 9eb448c7859d..eb907fefe02e 100644
>> --- a/arch/x86/kernel/process_64.c
>> +++ b/arch/x86/kernel/process_64.c
>> @@ -205,6 +205,26 @@ static __always_inline void save_fsgs(struct 
>> task_struct *task)
>> save_base_legacy(task, task->thread.gsindex, GS);
>>  }
>>
>> +/*
>> + * Currently, the only way for processes to change their FS/GS base is to 
>> call
>> + * ARCH_SET_FS/GS prctls and these reflect changes they make in 
>> task->thread.
>> + * There are, however, additional considerations:
>> + *
>> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
>> zeroes
>> + * the base and on some it doesn't, we need to check for that
>> + * (see save_base_legacy()).
>> + *
>> + * When FSGSBASE extensions are enabled userspace processes will be able to
>> + * change their FS/GS bases without kernel intervention. save_fsgs() will
>> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
>> + * instructions.
>> + */
>
> This is all a very complicated way to say "while a process is running,
> current->thread.fsbase and current->thread.gsbase may not match the
> corresponding CPU registers.  KVM wants an efficient way to save and
> restore FSBASE and GSBASE."
>

Well, I though it is not really obvious why "current->thread.fsbase and
current->thread.gsbase may not match the corresponding CPU registers"
:-) but save_base_legacy() is really nearby so I'll trim my comment in
v2.

> And how about changing this to:
>
> #if IS_ENABLED(CONFIG_KVM)
> void save_fsgs_for_kvm(void)
> {
> save_fsgs(current);
> }
> EXPORT_SYMBOL_GPL(save_fsgs_for_kvm);
>

Sure. Actually, there is nothing KVM-specific in this function but KVM
will probably be the only user for the time being.

-- 
  Vitaly

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 2:02 PM, Vitaly Kuznetsov  wrote:
> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
> the context is pretty well defined. Read MSR_FS_BASE from
> current->thread.fsbase after calling save_fsgs() which takes care of
> X86_BUG_NULL_SEG case now and will do RD[FG,GS]BASE when FSGSBASE
> extensions are exposed to userspace (currently they are not).
>
> Signed-off-by: Vitaly Kuznetsov 
> ---
>  arch/x86/include/asm/processor.h |  3 +++
>  arch/x86/kernel/process_64.c | 20 
>  arch/x86/kvm/vmx.c   |  4 +++-
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index b0ccd4847a58..006352b85ba3 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -410,6 +410,9 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
>  DECLARE_PER_CPU(char *, irq_stack_ptr);
>  DECLARE_PER_CPU(unsigned int, irq_count);
>  extern asmlinkage void ignore_sysret(void);
> +
> +/* Save actual FS/GS selectors and bases to current->thread */
> +void save_current_fsgs(void);
>  #else  /* X86_64 */
>  #ifdef CONFIG_CC_STACKPROTECTOR
>  /*
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 9eb448c7859d..eb907fefe02e 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -205,6 +205,26 @@ static __always_inline void save_fsgs(struct task_struct 
> *task)
> save_base_legacy(task, task->thread.gsindex, GS);
>  }
>
> +/*
> + * Currently, the only way for processes to change their FS/GS base is to 
> call
> + * ARCH_SET_FS/GS prctls and these reflect changes they make in task->thread.
> + * There are, however, additional considerations:
> + *
> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
> zeroes
> + * the base and on some it doesn't, we need to check for that
> + * (see save_base_legacy()).
> + *
> + * When FSGSBASE extensions are enabled userspace processes will be able to
> + * change their FS/GS bases without kernel intervention. save_fsgs() will
> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
> + * instructions.
> + */

This is all a very complicated way to say "while a process is running,
current->thread.fsbase and current->thread.gsbase may not match the
corresponding CPU registers.  KVM wants an efficient way to save and
restore FSBASE and GSBASE."

And how about changing this to:

#if IS_ENABLED(CONFIG_KVM)
void save_fsgs_for_kvm(void)
{
save_fsgs(current);
}
EXPORT_SYMBOL_GPL(save_fsgs_for_kvm);

--Andy

Re: [PATCH 1/3] x86/kvm/vmx: read MSR_FS_BASE from current->thread

[Andy Lutomirski]

On Mon, Mar 12, 2018 at 2:02 PM, Vitaly Kuznetsov  wrote:
> vmx_save_host_state() is only called from kvm_arch_vcpu_ioctl_run() so
> the context is pretty well defined. Read MSR_FS_BASE from
> current->thread.fsbase after calling save_fsgs() which takes care of
> X86_BUG_NULL_SEG case now and will do RD[FG,GS]BASE when FSGSBASE
> extensions are exposed to userspace (currently they are not).
>
> Signed-off-by: Vitaly Kuznetsov 
> ---
>  arch/x86/include/asm/processor.h |  3 +++
>  arch/x86/kernel/process_64.c | 20 
>  arch/x86/kvm/vmx.c   |  4 +++-
>  3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/processor.h 
> b/arch/x86/include/asm/processor.h
> index b0ccd4847a58..006352b85ba3 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -410,6 +410,9 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
>  DECLARE_PER_CPU(char *, irq_stack_ptr);
>  DECLARE_PER_CPU(unsigned int, irq_count);
>  extern asmlinkage void ignore_sysret(void);
> +
> +/* Save actual FS/GS selectors and bases to current->thread */
> +void save_current_fsgs(void);
>  #else  /* X86_64 */
>  #ifdef CONFIG_CC_STACKPROTECTOR
>  /*
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 9eb448c7859d..eb907fefe02e 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -205,6 +205,26 @@ static __always_inline void save_fsgs(struct task_struct 
> *task)
> save_base_legacy(task, task->thread.gsindex, GS);
>  }
>
> +/*
> + * Currently, the only way for processes to change their FS/GS base is to 
> call
> + * ARCH_SET_FS/GS prctls and these reflect changes they make in task->thread.
> + * There are, however, additional considerations:
> + *
> + * There is X86_BUG_NULL_SEG: on some CPUs writing '0' to FS/GS selectors 
> zeroes
> + * the base and on some it doesn't, we need to check for that
> + * (see save_base_legacy()).
> + *
> + * When FSGSBASE extensions are enabled userspace processes will be able to
> + * change their FS/GS bases without kernel intervention. save_fsgs() will
> + * have to be updated to actually read FS and GS bases with RD[FG,GS]BASE
> + * instructions.
> + */

This is all a very complicated way to say "while a process is running,
current->thread.fsbase and current->thread.gsbase may not match the
corresponding CPU registers.  KVM wants an efficient way to save and
restore FSBASE and GSBASE."

And how about changing this to:

#if IS_ENABLED(CONFIG_KVM)
void save_fsgs_for_kvm(void)
{
save_fsgs(current);
}
EXPORT_SYMBOL_GPL(save_fsgs_for_kvm);

--Andy