| Message ID | 20181120160325.75415-1-liran.alon@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Update shared MSRs to be saved/restored on MSR_EFER.LMA changes | expand |
On Tue, Nov 20, 2018 at 8:03 AM, Liran Alon <liran.alon@oracle.com> wrote: > When guest transitions from/to long-mode by modifying MSR_EFER.LMA, > the list of shared MSRs to be saved/restored on guest<->host > transitions is updated (See vmx_set_efer() call to setup_msrs()). > > On every entry to guest, vcpu_enter_guest() calls > vmx_prepare_switch_to_guest(). This function should also take care > of setting the shared MSRs to be saved/restored. However, the > function does nothing in case we are already running with loaded > guest state (vmx->loaded_cpu_state != NULL). > > This means that even when guest modifies MSR_EFER.LMA which results > in updating the list of shared MSRs, it isn't being taken into account > by vmx_prepare_switch_to_guest() because it happens while we are > running with loaded guest state. > > To fix above mentioned issue, add a flag to mark that the list of > shared MSRs has been updated and modify vmx_prepare_switch_to_guest() > to set shared MSRs when running with host state *OR* list of shared > MSRs has been updated. > > Note that this issue was mistakenly introduced by commit > 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's > kernel_gs_base") because previously vmx_set_efer() always called > vmx_load_host_state() which resulted in vmx_prepare_switch_to_guest() to > set shared MSRs. > > Fixes: 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") > > Reported-by: Eyal Moscovici <eyal.moscovici@oracle.com> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> > Reviewed-by: Liam Merwick <liam.merwick@oracle.com> > Signed-off-by: Liran Alon <liran.alon@oracle.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On 20/11/18 19:20, Jim Mattson wrote: > On Tue, Nov 20, 2018 at 8:03 AM, Liran Alon <liran.alon@oracle.com> wrote: >> When guest transitions from/to long-mode by modifying MSR_EFER.LMA, >> the list of shared MSRs to be saved/restored on guest<->host >> transitions is updated (See vmx_set_efer() call to setup_msrs()). >> >> On every entry to guest, vcpu_enter_guest() calls >> vmx_prepare_switch_to_guest(). This function should also take care >> of setting the shared MSRs to be saved/restored. However, the >> function does nothing in case we are already running with loaded >> guest state (vmx->loaded_cpu_state != NULL). >> >> This means that even when guest modifies MSR_EFER.LMA which results >> in updating the list of shared MSRs, it isn't being taken into account >> by vmx_prepare_switch_to_guest() because it happens while we are >> running with loaded guest state. >> >> To fix above mentioned issue, add a flag to mark that the list of >> shared MSRs has been updated and modify vmx_prepare_switch_to_guest() >> to set shared MSRs when running with host state *OR* list of shared >> MSRs has been updated. >> >> Note that this issue was mistakenly introduced by commit >> 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's >> kernel_gs_base") because previously vmx_set_efer() always called >> vmx_load_host_state() which resulted in vmx_prepare_switch_to_guest() to >> set shared MSRs. >> >> Fixes: 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") >> >> Reported-by: Eyal Moscovici <eyal.moscovici@oracle.com> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> > Reviewed-by: Jim Mattson <jmattson@google.com> > Queued, thanks. Paolo
> On 20 Nov 2018, at 20:20, Jim Mattson <jmattson@google.com> wrote: > > On Tue, Nov 20, 2018 at 8:03 AM, Liran Alon <liran.alon@oracle.com> wrote: >> When guest transitions from/to long-mode by modifying MSR_EFER.LMA, >> the list of shared MSRs to be saved/restored on guest<->host >> transitions is updated (See vmx_set_efer() call to setup_msrs()). >> >> On every entry to guest, vcpu_enter_guest() calls >> vmx_prepare_switch_to_guest(). This function should also take care >> of setting the shared MSRs to be saved/restored. However, the >> function does nothing in case we are already running with loaded >> guest state (vmx->loaded_cpu_state != NULL). >> >> This means that even when guest modifies MSR_EFER.LMA which results >> in updating the list of shared MSRs, it isn't being taken into account >> by vmx_prepare_switch_to_guest() because it happens while we are >> running with loaded guest state. >> >> To fix above mentioned issue, add a flag to mark that the list of >> shared MSRs has been updated and modify vmx_prepare_switch_to_guest() >> to set shared MSRs when running with host state *OR* list of shared >> MSRs has been updated. >> >> Note that this issue was mistakenly introduced by commit >> 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's >> kernel_gs_base") because previously vmx_set_efer() always called >> vmx_load_host_state() which resulted in vmx_prepare_switch_to_guest() to >> set shared MSRs. >> >> Fixes: 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") >> >> Reported-by: Eyal Moscovici <eyal.moscovici@oracle.com> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >> Signed-off-by: Liran Alon <liran.alon@oracle.com> > Reviewed-by: Jim Mattson <jmattson@google.com> Paolo, I saw this patch don’t have a Cc to stable tree. It seems that it should have one as-well as it also fixes an important regression issue. -Liran
On 28/11/18 17:43, Liran Alon wrote: > > >> On 20 Nov 2018, at 20:20, Jim Mattson <jmattson@google.com> wrote: >> >> On Tue, Nov 20, 2018 at 8:03 AM, Liran Alon <liran.alon@oracle.com> wrote: >>> When guest transitions from/to long-mode by modifying MSR_EFER.LMA, >>> the list of shared MSRs to be saved/restored on guest<->host >>> transitions is updated (See vmx_set_efer() call to setup_msrs()). >>> >>> On every entry to guest, vcpu_enter_guest() calls >>> vmx_prepare_switch_to_guest(). This function should also take care >>> of setting the shared MSRs to be saved/restored. However, the >>> function does nothing in case we are already running with loaded >>> guest state (vmx->loaded_cpu_state != NULL). >>> >>> This means that even when guest modifies MSR_EFER.LMA which results >>> in updating the list of shared MSRs, it isn't being taken into account >>> by vmx_prepare_switch_to_guest() because it happens while we are >>> running with loaded guest state. >>> >>> To fix above mentioned issue, add a flag to mark that the list of >>> shared MSRs has been updated and modify vmx_prepare_switch_to_guest() >>> to set shared MSRs when running with host state *OR* list of shared >>> MSRs has been updated. >>> >>> Note that this issue was mistakenly introduced by commit >>> 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's >>> kernel_gs_base") because previously vmx_set_efer() always called >>> vmx_load_host_state() which resulted in vmx_prepare_switch_to_guest() to >>> set shared MSRs. >>> >>> Fixes: 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") >>> >>> Reported-by: Eyal Moscovici <eyal.moscovici@oracle.com> >>> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >>> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> >>> Signed-off-by: Liran Alon <liran.alon@oracle.com> >> Reviewed-by: Jim Mattson <jmattson@google.com> > > Paolo, I saw this patch don’t have a Cc to stable tree. It seems that it should have one as-well as it also fixes an important regression issue. Ok, I wasn't sure if this is a regression. I'll propose it after Linus pulls. Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 4555077d69ce..aa6afbd20c08 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -984,6 +984,7 @@ struct vcpu_vmx { struct shared_msr_entry *guest_msrs; int nmsrs; int save_nmsrs; + bool guest_msrs_dirty; unsigned long host_idt_base; #ifdef CONFIG_X86_64 u64 msr_host_kernel_gs_base; @@ -2897,6 +2898,20 @@ static void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu) vmx->req_immediate_exit = false; + /* + * Note that guest MSRs to be saved/restored can also be changed + * when guest state is loaded. This happens when guest transitions + * to/from long-mode by setting MSR_EFER.LMA. + */ + if (!vmx->loaded_cpu_state || vmx->guest_msrs_dirty) { + vmx->guest_msrs_dirty = false; + for (i = 0; i < vmx->save_nmsrs; ++i) + kvm_set_shared_msr(vmx->guest_msrs[i].index, + vmx->guest_msrs[i].data, + vmx->guest_msrs[i].mask); + + } + if (vmx->loaded_cpu_state) return; @@ -2957,11 +2972,6 @@ static void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu) vmcs_writel(HOST_GS_BASE, gs_base); host_state->gs_base = gs_base; } - - for (i = 0; i < vmx->save_nmsrs; ++i) - kvm_set_shared_msr(vmx->guest_msrs[i].index, - vmx->guest_msrs[i].data, - vmx->guest_msrs[i].mask); } static void vmx_prepare_switch_to_host(struct vcpu_vmx *vmx) @@ -3436,6 +3446,7 @@ static void setup_msrs(struct vcpu_vmx *vmx) move_msr_up(vmx, index, save_nmsrs++); vmx->save_nmsrs = save_nmsrs; + vmx->guest_msrs_dirty = true; if (cpu_has_vmx_msr_bitmap()) vmx_update_msr_bitmap(&vmx->vcpu);
When guest transitions from/to long-mode by modifying MSR_EFER.LMA, the list of shared MSRs to be saved/restored on guest<->host transitions is updated (See vmx_set_efer() call to setup_msrs()). On every entry to guest, vcpu_enter_guest() calls vmx_prepare_switch_to_guest(). This function should also take care of setting the shared MSRs to be saved/restored. However, the function does nothing in case we are already running with loaded guest state (vmx->loaded_cpu_state != NULL). This means that even when guest modifies MSR_EFER.LMA which results in updating the list of shared MSRs, it isn't being taken into account by vmx_prepare_switch_to_guest() because it happens while we are running with loaded guest state. To fix above mentioned issue, add a flag to mark that the list of shared MSRs has been updated and modify vmx_prepare_switch_to_guest() to set shared MSRs when running with host state *OR* list of shared MSRs has been updated. Note that this issue was mistakenly introduced by commit 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") because previously vmx_set_efer() always called vmx_load_host_state() which resulted in vmx_prepare_switch_to_guest() to set shared MSRs. Fixes: 678e315e78a7 ("KVM: vmx: add dedicated utility to access guest's kernel_gs_base") Reported-by: Eyal Moscovici <eyal.moscovici@oracle.com> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> Reviewed-by: Liam Merwick <liam.merwick@oracle.com> Signed-off-by: Liran Alon <liran.alon@oracle.com> --- arch/x86/kvm/vmx.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)