thr3ads.net - Xen devel - [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific) [Oct 2010]

If this information is useful, please help other people find it:
Share via:

Christoph Egger

2010-Oct-15 13:07 UTC

[Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

-- 
---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@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger

2010-Nov-12 18:44 UTC

head link

[Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

-- 
---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@lists.xensource.com
http://lists.xensource.com/xen-devel

Tim Deegan

2010-Nov-16 15:06 UTC

head link

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

At 18:44 +0000 on 12 Nov (1289587459), Christoph Egger
wrote:> diff -r 3bfc06e2e41a -r b18448601670 xen/arch/x86/hvm/svm/nestedsvm.c
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -24,6 +24,30 @@
>  #include <asm/hvm/svm/nestedsvm.h>
>  #include <asm/hvm/svm/svmdebug.h>
>  #include <asm/paging.h> /* paging_mode_hap */
> +#include <asm/event.h> /* for local_event_delivery_(en|dis)able */
> +
> +static int
> +nestedsvm_vcpu_clgi(struct vcpu *v)
> +{
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +
> +    /* clear gif flag */
> +    svm->ns_gif = 0;
> +    local_event_delivery_disable(); /* mask events for PV drivers */
This function, and the stgi one below, can only operate safely on
current; if you want to keep the argument for performance then maybe
ASSERT(v == current) for sanity.
> +    return 0;
Also, since they only ever return 0, please make them return void
instead. 
> +}
> +
> +static int
> +nestedsvm_vcpu_stgi(struct vcpu *v)
> +{
> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> +
> +    /* Always set the GIF to make hvm_interrupt_blocked work. */
> +    svm->ns_gif = 1;
> +
> +    local_event_delivery_enable(); /* unmask events for PV drivers */
> +    return 0;
> +}
> +void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
> +{
> +    int ret;
> +    unsigned int inst_len;
> +
> +    if ( !nestedhvm_enabled(v->domain) ) {
> +        hvm_inject_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE,
0);
> +        return;
> +    }
> +
> +    if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 )
> +        return;
> +
> +    ret = nestedsvm_vcpu_stgi(v);
> +    if (ret)
> +        /* On failure, nestedsvm_vcpu_stgi injected an exception,
> +         * almost a #GP or #UD.
No, it never fails and always returns 0. :)  Likewise below.

Cheers,

Tim.
> +         */
> +       return;
> +
> +    __update_guest_eip(regs, inst_len);
> +}
> +
> +void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
> +{
> +    int ret;
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    unsigned int inst_len;
> +
> +    if ( !nestedhvm_enabled(v->domain) ) {
> +        hvm_inject_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE,
0);
> +        return;
> +    }
> +
> +    if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 )
> +        return;
> +
> +    ret = nestedsvm_vcpu_clgi(v);
> +    if (ret)
> +        /* On failure, nestedsvm_vcpu_clgi injected an exception,
> +         * almost a #GP or #UD.
> +         */
> +        return;
> +
> +    /* After a CLGI no interrupts should come */
> +    vmcb->vintr.fields.irq = 0;
> +    vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
> +
> +    __update_guest_eip(regs, inst_len);
> +}
-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Keir Fraser

2010-Nov-16 15:45 UTC

head link

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

On 16/11/2010 15:06, "Tim Deegan" <Tim.Deegan@citrix.com> wrote:
>> +static int
>> +nestedsvm_vcpu_clgi(struct vcpu *v)
>> +{
>> +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
>> +
>> +    /* clear gif flag */
>> +    svm->ns_gif = 0;
>> +    local_event_delivery_disable(); /* mask events for PV drivers */
> 
> This function, and the stgi one below, can only operate safely on
> current; if you want to keep the argument for performance then maybe
> ASSERT(v == current) for sanity.
I don''t believe in the performance win. Functions which act correctly
only
on current should not be passed a vcpu parameter.

 -- Keir



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

Christoph Egger

2010-Dec-02 17:47 UTC

head link

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

On Tuesday 16 November 2010 16:06:45 Tim Deegan wrote:> At 18:44 +0000 on 12 Nov (1289587459), Christoph Egger wrote:
> > diff -r 3bfc06e2e41a -r b18448601670 xen/arch/x86/hvm/svm/nestedsvm.c
> > --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> > +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> > @@ -24,6 +24,30 @@
> >  #include <asm/hvm/svm/nestedsvm.h>
> >  #include <asm/hvm/svm/svmdebug.h>
> >  #include <asm/paging.h> /* paging_mode_hap */
> > +#include <asm/event.h> /* for local_event_delivery_(en|dis)able
*/
> > +
> > +static int
> > +nestedsvm_vcpu_clgi(struct vcpu *v)
> > +{
> > +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> > +
> > +    /* clear gif flag */
> > +    svm->ns_gif = 0;
> > +    local_event_delivery_disable(); /* mask events for PV drivers */
>
> This function, and the stgi one below, can only operate safely on
> current; if you want to keep the argument for performance then maybe
> ASSERT(v == current) for sanity.
No, it''s not about performance. It''s about consistency.
Do you want me to remove the parameter?
> > +    return 0;
>
> Also, since they only ever return 0, please make them return void
> instead.
Fixed.
>
> > +}
> > +
> > +static int
> > +nestedsvm_vcpu_stgi(struct vcpu *v)
> > +{
> > +    struct nestedsvm *svm = &vcpu_nestedsvm(v);
> > +
> > +    /* Always set the GIF to make hvm_interrupt_blocked work. */
> > +    svm->ns_gif = 1;
> > +
> > +    local_event_delivery_enable(); /* unmask events for PV drivers */
> > +    return 0;
> > +}
> >
> > +void svm_vmexit_do_stgi(struct cpu_user_regs *regs, struct vcpu *v)
> > +{
> > +    int ret;
> > +    unsigned int inst_len;
> > +
> > +    if ( !nestedhvm_enabled(v->domain) ) {
> > +        hvm_inject_exception(TRAP_invalid_op,
HVM_DELIVER_NO_ERROR_CODE,
> > 0); +        return;
> > +    }
> > +
> > +    if ( (inst_len = __get_instruction_length(v, INSTR_STGI)) == 0 )
> > +        return;
> > +
> > +    ret = nestedsvm_vcpu_stgi(v);
> > +    if (ret)
> > +        /* On failure, nestedsvm_vcpu_stgi injected an exception,
> > +         * almost a #GP or #UD.
>
> No, it never fails and always returns 0. :)  Likewise below.
Both fixed.

Christoph
>
> Cheers,
>
> Tim.
>
> > +         */
> > +       return;
> > +
> > +    __update_guest_eip(regs, inst_len);
> > +}
> > +
> > +void svm_vmexit_do_clgi(struct cpu_user_regs *regs, struct vcpu *v)
> > +{
> > +    int ret;
> > +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > +    unsigned int inst_len;
> > +
> > +    if ( !nestedhvm_enabled(v->domain) ) {
> > +        hvm_inject_exception(TRAP_invalid_op,
HVM_DELIVER_NO_ERROR_CODE,
> > 0); +        return;
> > +    }
> > +
> > +    if ( (inst_len = __get_instruction_length(v, INSTR_CLGI)) == 0 )
> > +        return;
> > +
> > +    ret = nestedsvm_vcpu_clgi(v);
> > +    if (ret)
> > +        /* On failure, nestedsvm_vcpu_clgi injected an exception,
> > +         * almost a #GP or #UD.
> > +         */
> > +        return;
> > +
> > +    /* After a CLGI no interrupts should come */
> > +    vmcb->vintr.fields.irq = 0;
> > +    vmcb->general1_intercepts &= ~GENERAL1_INTERCEPT_VINTR;
> > +
> > +    __update_guest_eip(regs, inst_len);
> > +}


-- 
---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@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Oct 2010 - [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

[Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

[Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)

Re: [Xen-devel] [PATCH 10/13] Nested Virtualization: interrupts (svm specific)