[Xen-devel] Re: [PATCH 00/13] Nested Virtualization: Overview

[Christoph Egger <Christoph.Egger@xxxxxxx>]

Thank you very much for giving me the valuable information I am asking for, 
finally.

> >> Freely (at any time) preload/post-store of VMCS fields is very hard
> >> because VMX only provide access to current VMCS (because it is CPU
> >> register), while SVM may be able to access all VMCBs given that it
> >> is in memory. I can't say it is impossible (one can solve it with
> >> further limitation/complexity), however enforcing those conversion
> >> simply put trickies to VMX code to limiting the time of
> >> pre-load/post-load, and the additional cost of VMCS access.
> >
> > When the VCPU switches between host and guest mode then you need to
> > save the l1 guest state, restore the l2 guest state and vice versa.
>
> No. Switching from L1 to L2 guest, it is a simple restore of L2 guest state
> (VMCLEAR/VMPTRLD). VMX doesn't save L1 state,

So the function hooks prepare4vmentry is pretty cheap and the 'hostsave'
function hook is empty.

> while SVM does require the save of L1 state per my understanding.

SVM does not per architecture. It is the implementation I have taken over from
KVM. It will be replaced with an implementation that only requires a VMSAVE
to save the L1 state. But that won't change the patch series fundamentally as
this change touches SVM code only.

> Intel process can hold VMCS in processor for performance.
>
> Switching from L2 to L1, we need to convert (or store) some VMCS
> information from physical to virtual VMCS.

That is what can be put into the prepare4vmexit function hook.

> But it is limited and only covers the "guest state" and exit information.
> Load of L1 state may be as simple as VMPTRLD (of course it may modify some
> VMCS field upon different situation).

That is what can be put into the 'hostrestore' function hook.

> > This requires a lot of accesses from/to the vmcb/vmcs  unless you have
> > a lazy switching technique, do you ?
>
> Yes, the Intel hardware already did lazy switching thru VMPTRLD.
> And the access of VMCS is depending on the L1 guest modification, only
> dirty VMCS fields needs to be updated.

Sounds like you need a 'shadow vmcs' that holds the l2 guest state.

> In the majority case, the VM exit from L2 guest will be handled by root VMM
> directly.

Same on SVM. root VMM handles everything L1 guest does not intercept plus some
special intercepts such as interrupts, nmi's, page fault/nested page faults.

> One example is external interrupt, which doesn't need to access 
> rest of VMCS fields except the reason, but the wrapper makes the access a
> must, which I can't agree.

Which conversion function do you mean by 'wrapper' here ? Why does it require
additional VMCS field accesses ?

Can you explain this in detail, please ?


> >> Another portion of so called common code are actually SVM code only.
> >> Here are part of them:
> >>
> >>
> >>
> >> +
> >> +static enum nestedhvm_vmexits
> >> +nestedhvm_vmexit_msr(unsigned long *msr_bitmap, uint32_t msr,
> >> bool_t write) +{ + bool_t enabled;
> >> +  unsigned long *msr_bit = NULL;
> >> +
> >> +  /*
> >> +   * See AMD64 Programmers Manual, Vol 2, Section 15.10 +  *
> >> (MSR-Bitmap Address). +     */
> >> +  if ( msr <= 0x1fff )
> >> +          msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
> >> +  else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> >> +          msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
> >> +  else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
> >> +          msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
> >
> > Why does above code snippet not work on Intel CPUs ?
>
> It is said even "Intel processor" is following AMD64 manual, isn;t it?
> msr_bitmap in Intel doesn't have a fixed bitmap position, rather it scan
> the entire table to decide which MSR to automatically save/restore for
> performance given that we only put single digital MSRs for that purpose.
> BTW, Intel does implement MSRs large than 0x1fff such as 0x107D0.
> Preassuming their usage model for now is risky.

Ok, I will think about this.

> >> +/* Virtual GIF */
> >> +int
> >> +nestedsvm_vcpu_clgi(struct vcpu *v)
> >> +{
> >> +  if (!nestedhvm_enabled(v->domain)) {
> >> +          hvm_inject_exception(TRAP_invalid_op, 0, 0);
> >> +          return -1;
> >> +  }
> >> +
> >> +  if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >> +          return 0;
> >> +
> >> +  /* clear gif flag */
> >> +  vcpu_nestedhvm(v).nh_gif = 0;
> >> +  local_event_delivery_disable(); /* mask events for PV drivers */
> >> +  return 0; +}
> >> +
> >> +int
> >> +nestedsvm_vcpu_stgi(struct vcpu *v)
> >> +{
> >> +  if (!nestedhvm_enabled(v->domain)) {
> >> +          hvm_inject_exception(TRAP_invalid_op, 0, 0);
> >> +          return -1;
> >> +  }
> >> +
> >> +  /* Always set the GIF to make hvm_interrupt_blocked work. */
> >> +  vcpu_nestedhvm(v).nh_gif = 1;
> >> +
> >> +  if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >> +          return 0;
> >> +
> >> +  local_event_delivery_enable(); /* unmask events for PV drivers */
> >> +  return 0; +}
> >> +
> >
> > The reason to leave this in the generic code is what Keir stated out
> > as feedback to the second patch series:
> > http://lists.xensource.com/archives/html/xen-devel/2010-06/msg00280.html
> > (What was patch #10 there is  patch #8 in latest patch series).
>
> While, I didn;t read in this way. Even the function itself is wrapped, they
> should go to SVM tree if not go together with the caller.
>
> Anyway, to me given that nested SVM & VMX is still on the very beginning of
> development, I can only say yes to those wrappers that have clear
> understanding to both side.

Good to know. See my offer below.

> I would rather leave those uncertain wrappers to future, once the basic
> shape of nested virtualization is good and stable enough, i.e. lightweight
> wrapper.  We have plenty of performance work ahead such as virtual VTd
> support, enhanced PV driver for nested etc. Excessive wrapper is simple a
> burden to nested VMX developers for those future features.
>
> Qing will post his patch today or tomorrow for your reference if you want.

Thanks. May I take code from there and add into my patch series ?

> C++ abstracts better than C, but C++ never replaces C in places like Linux.

It's not necessary. When you truely understand the concept behind interface
and implementation you can do that in any language you know.

> BTW, can you add SR-IOV test into your patch test system to avoid
> regression?

Yes.

> Thx, Eddie



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel