On Saturday 04 September 2010 03:30:56 Dong, Eddie wrote:
> Christoph Egger wrote:
> > On Friday 03 September 2010 10:12:08 Dong, Eddie wrote:
> >> The fundamental argument of whether we should convert vendor
> >> specific code into vendor neutral code is not solved yet.
> >> I guess I don;t need to review rest of the code due to this :) I
> >> strongly suggest we remove those unnecessary wrapper for readibilty,
> >> flexibility and performance.
> >
> > I am sort of surprised that you raise the same things again that have
> > been discussed in the last round. Please correct me if I am wrong:
> > This sounds to me that the statements/explanations from Keir, Tim and
> > me are not clear
> > to you. Please feel free to ask whatever is unclear / questionable to
> > you.
>
> So you are actually not asking for my review comments, though you
> explicitly called it.
Well, when you say so :)
> I am not convinced a wrapper for wrapping, which is just make it like C
> style as you said, should be taken just because somebody have called it
> out.
I wouldn't call it a wrapper. I call as the implementation of an abstraction.
And regarding to what Tim said:
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01044.html
Is there a particular reason not to do so apart from the LOC as you brought
up in http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01255.html
(As Tim said in
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01256.html
it's the duplication of logic that counts.)
> 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.
This requires a lot of accesses from/to the vmcb/vmcs unless you have
a lazy switching technique, do you ?
>
> 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 ?
>
> +/* 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).
Christoph
--
---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
|
Home