Avi Kivity <avi@xxxxxxxxxx> wrote on 20/10/2009 06:56:39:
> From:
>
> Avi Kivity <avi@xxxxxxxxxx>
>
> To:
>
> Orit Wasserman/Haifa/IBM@IBMIL
>
> Cc:
>
> kvm@xxxxxxxxxxxxxxx, Ben-Ami Yassour1/Haifa/IBM@IBMIL, Abel Gordon/
> Haifa/IBM@IBMIL, Muli Ben-Yehuda/Haifa/IBM@IBMIL,
> aliguori@xxxxxxxxxx, mdday@xxxxxxxxxx
>
> Date:
>
> 20/10/2009 06:56
>
> Subject:
>
> Re: [PATCH 5/5] Nested VMX patch 5 implements vmlaunch and vmresume
>
> On 10/15/2009 11:41 PM, oritw@xxxxxxxxxx wrote:
> > From: Orit Wasserman<oritw@xxxxxxxxxx>
> >
> > ---
> > arch/x86/kvm/vmx.c | 1173 ++++++++++++++++++++++++++++++++++++++
> ++++++++++++--
> > 1 files changed, 1148 insertions(+), 25 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 6a4c252..e814029 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -209,6 +209,7 @@ struct __attribute__ ((__packed__)) level_state {
> > struct vmcs *vmcs;
> > int cpu;
> > int launched;
> > + bool first_launch;
> > };
> >
> > struct nested_vmx {
> > @@ -216,6 +217,12 @@ struct nested_vmx {
> > bool vmxon;
> > /* What is the location of the vmcs l1 keeps for l2? (in
> level1 gpa) */
> > u64 vmptr;
> > + /* Are we running nested guest */
> > + bool nested_mode;
> > + /* L1 requested VMLAUNCH or VMRESUME but we didn't run L2 yet */
> > + bool nested_run_pending;
> > + /* flag indicating if there was a valid IDT after exiting from l2
*/
> > + bool nested_valid_idt;
> >
>
> Did you mean valid_idt_vectoring_info?
yes.
>
> No need to prefix everything with nested_ inside nested_vmx.
OK.
>
> > +void prepare_vmcs_12(struct kvm_vcpu *vcpu)
> > +{
> > + struct shadow_vmcs *l2_shadow_vmcs =
> > + get_shadow_vmcs(vcpu);
> > +
> > + l2_shadow_vmcs->guest_es_selector = vmcs_read16(GUEST_ES_SELECTOR);
> > + l2_shadow_vmcs->guest_cs_selector = vmcs_read16(GUEST_CS_SELECTOR);
> > + l2_shadow_vmcs->guest_ss_selector = vmcs_read16(GUEST_SS_SELECTOR);
> > + l2_shadow_vmcs->guest_ds_selector = vmcs_read16(GUEST_DS_SELECTOR);
> > + l2_shadow_vmcs->guest_fs_selector = vmcs_read16(GUEST_FS_SELECTOR);
> > + l2_shadow_vmcs->guest_gs_selector = vmcs_read16(GUEST_GS_SELECTOR);
> > + l2_shadow_vmcs->guest_ldtr_selector = vmcs_read16
(GUEST_LDTR_SELECTOR);
> > + l2_shadow_vmcs->guest_tr_selector = vmcs_read16(GUEST_TR_SELECTOR);
> > +
> > + l2_shadow_vmcs->tsc_offset = vmcs_read64(TSC_OFFSET);
> > + l2_shadow_vmcs->guest_physical_address =
> > + vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > + l2_shadow_vmcs->vmcs_link_pointer = vmcs_read64(VMCS_LINK_POINTER);
> >
>
> Physical addresses need translation, no?
If you are referring to GUEST_PHYSICAL_ADDRESS than there is no need for
translation for L1.
It need to stay in L2 physical address.
>
> > + l2_shadow_vmcs->guest_cr0 = vmcs_readl(GUEST_CR0);
> > +
> > + l2_shadow_vmcs->guest_cr4 = vmcs_readl(GUEST_CR4);
> >
>
> We don't allow the guest to modify these, so no need to read them. If
> you do, you need to remove the bits that we modify.
You are correct for example CR0.TS , it will be fixed in the next set of
patches.
>
> > +
> > +int load_vmcs_common(struct shadow_vmcs *src)
> > +{
> > +
> > + vmcs_write64(VMCS_LINK_POINTER, src->vmcs_link_pointer);
> >
>
> Why load this?
At the moment it is not used , maybe in the future.
I can add a check to see if it was changed.
>
> > + vmcs_write64(GUEST_IA32_DEBUGCTL, src->guest_ia32_debugctl);
> >
>
> I think some features there are dangerous.
I will look into it.
>
> > + vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, src->
vm_entry_msr_load_count);
> >
>
> Need to verify? Also need to validate the loaded MSRs and run them
> through kvm_set_msr() instead of letting the cpu do it.
I will add the checks.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
