| Message ID | 1459243805-2150-1-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.03.16 at 11:30, <paul.durrant@citrix.com> wrote: > If any vcpu has a pending APIC assist when the domain is suspended > then the vector needs to be saved. If this is not done then it's > possible for the vector to remain pending in the vlapic ISR > indefinitely after resume. > > This patch adds code to save the APIC assist vector value in the > viridian vcpu save record. This means that the record is now zero- > extended on load and, because this implies a loaded value of > zero means nothing is pending (for backwards compatibility with > hosts not implementing APIC assist), the rest of the viridian APIC > assist code is adjusted to treat a zero value in this way. A > check has therefore been added to viridian_start_apic_assist() to > prevent the enlightenment being used for vectors < 0x10 (which > are illegal for an APIC). > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Are you then also looking into the domain page leaks which seem likely to stem from the earlier APIC assist change? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 29 March 2016 10:53 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2] x86/hvm/viridian: save APIC assist vector > > >>> On 29.03.16 at 11:30, <paul.durrant@citrix.com> wrote: > > If any vcpu has a pending APIC assist when the domain is suspended > > then the vector needs to be saved. If this is not done then it's > > possible for the vector to remain pending in the vlapic ISR > > indefinitely after resume. > > > > This patch adds code to save the APIC assist vector value in the > > viridian vcpu save record. This means that the record is now zero- > > extended on load and, because this implies a loaded value of > > zero means nothing is pending (for backwards compatibility with > > hosts not implementing APIC assist), the rest of the viridian APIC > > assist code is adjusted to treat a zero value in this way. A > > check has therefore been added to viridian_start_apic_assist() to > > prevent the enlightenment being used for vectors < 0x10 (which > > are illegal for an APIC). > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Are you then also looking into the domain page leaks which seem > likely to stem from the earlier APIC assist change? > Yes, I'm going to look at them. Not sure why the vcpu shutdown code is not releasing things. Paul > Jan
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Paul Durrant > Sent: 29 March 2016 10:58 > To: Jan Beulich > Cc: xen-devel@lists.xenproject.org > Subject: Re: [Xen-devel] [PATCH v2] x86/hvm/viridian: save APIC assist > vector > > > -----Original Message----- > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: 29 March 2016 10:53 > > To: Paul Durrant > > Cc: xen-devel@lists.xenproject.org > > Subject: Re: [PATCH v2] x86/hvm/viridian: save APIC assist vector > > > > >>> On 29.03.16 at 11:30, <paul.durrant@citrix.com> wrote: > > > If any vcpu has a pending APIC assist when the domain is suspended > > > then the vector needs to be saved. If this is not done then it's > > > possible for the vector to remain pending in the vlapic ISR > > > indefinitely after resume. > > > > > > This patch adds code to save the APIC assist vector value in the > > > viridian vcpu save record. This means that the record is now zero- > > > extended on load and, because this implies a loaded value of > > > zero means nothing is pending (for backwards compatibility with > > > hosts not implementing APIC assist), the rest of the viridian APIC > > > assist code is adjusted to treat a zero value in this way. A > > > check has therefore been added to viridian_start_apic_assist() to > > > prevent the enlightenment being used for vectors < 0x10 (which > > > are illegal for an APIC). > > > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > > > Are you then also looking into the domain page leaks which seem > > likely to stem from the earlier APIC assist change? > > > > Yes, I'm going to look at them. Not sure why the vcpu shutdown code is not > releasing things. > Ah. It appears to doesn't get called when the domain is shut down, that's why. I guess the viridian_vcpu_deinit() call needs relocating then. Paul > Paul > > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 29.03.16 at 11:30, <paul.durrant@citrix.com> wrote: > @@ -806,7 +808,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > for_each_vcpu( d, v ) { > struct hvm_viridian_vcpu_context ctxt; > > - ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; > + ctxt.apic_assist_msr = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; > + ctxt.apic_assist_vector = v->arch.hvm_vcpu.viridian.apic_assist.vector; > > if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) != 0 ) > return 1; While this went in already, I'm afraid there's further work needed: There's an information leak here (since the padding field doesn't get zeroed), and ... > @@ -829,13 +832,15 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > return -EINVAL; > } > > - if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > + if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > return -EINVAL; > > - v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist; > + v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr; > if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled ) > initialize_apic_assist(v); > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; > + > return 0; > } ... the padding field doesn't get checked to be zero here, preventing us from later on assigning meaning to any parts of it. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 30 March 2016 07:19 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2] x86/hvm/viridian: save APIC assist vector > > >>> On 29.03.16 at 11:30, <paul.durrant@citrix.com> wrote: > > @@ -806,7 +808,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, > hvm_domain_context_t *h) > > for_each_vcpu( d, v ) { > > struct hvm_viridian_vcpu_context ctxt; > > > > - ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; > > + ctxt.apic_assist_msr = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; > > + ctxt.apic_assist_vector = v- > >arch.hvm_vcpu.viridian.apic_assist.vector; > > > > if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) != 0 ) > > return 1; > > While this went in already, I'm afraid there's further work needed: > There's an information leak here (since the padding field doesn't get > zeroed), and ... > Yes, sorry I didn't notice that the struct was not zeroed out before writing to it. > > @@ -829,13 +832,15 @@ static int viridian_load_vcpu_ctxt(struct domain > *d, hvm_domain_context_t *h) > > return -EINVAL; > > } > > > > - if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > > + if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) > > return -EINVAL; > > > > - v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist; > > + v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr; > > if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled ) > > initialize_apic_assist(v); > > > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; > > + > > return 0; > > } > > ... the padding field doesn't get checked to be zero here, > preventing us from later on assigning meaning to any parts of it. > Ok. I'll add that. Paul > Jan
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 410320c..dceed2c 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -252,7 +252,6 @@ static void initialize_apic_assist(struct vcpu *v) if ( viridian_feature_mask(v->domain) & HVMPV_apic_assist ) { v->arch.hvm_vcpu.viridian.apic_assist.va = va; - v->arch.hvm_vcpu.viridian.apic_assist.vector = -1; return; } @@ -288,12 +287,15 @@ void viridian_start_apic_assist(struct vcpu *v, int vector) if ( !va ) return; + if ( vector < 0x10 ) + return; + /* * If there is already an assist pending then something has gone * wrong and the VM will most likely hang so force a crash now * to make the problem clear. */ - if ( v->arch.hvm_vcpu.viridian.apic_assist.vector >= 0 ) + if ( v->arch.hvm_vcpu.viridian.apic_assist.vector ) domain_crash(v->domain); v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; @@ -306,13 +308,13 @@ int viridian_complete_apic_assist(struct vcpu *v) int vector; if ( !va ) - return -1; + return 0; if ( *va & 1u ) - return -1; /* Interrupt not yet processed by the guest. */ + return 0; /* Interrupt not yet processed by the guest. */ vector = v->arch.hvm_vcpu.viridian.apic_assist.vector; - v->arch.hvm_vcpu.viridian.apic_assist.vector = -1; + v->arch.hvm_vcpu.viridian.apic_assist.vector = 0; return vector; } @@ -325,7 +327,7 @@ void viridian_abort_apic_assist(struct vcpu *v) return; *va &= ~1u; - v->arch.hvm_vcpu.viridian.apic_assist.vector = -1; + v->arch.hvm_vcpu.viridian.apic_assist.vector = 0; } static void update_reference_tsc(struct domain *d, bool_t initialize) @@ -806,7 +808,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) for_each_vcpu( d, v ) { struct hvm_viridian_vcpu_context ctxt; - ctxt.apic_assist = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; + ctxt.apic_assist_msr = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw; + ctxt.apic_assist_vector = v->arch.hvm_vcpu.viridian.apic_assist.vector; if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) != 0 ) return 1; @@ -829,13 +832,15 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) return -EINVAL; } - if ( hvm_load_entry(VIRIDIAN_VCPU, h, &ctxt) != 0 ) + if ( hvm_load_entry_zeroextend(VIRIDIAN_VCPU, h, &ctxt) != 0 ) return -EINVAL; - v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist; + v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr; if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled ) initialize_apic_assist(v); + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; + return 0; } diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index f36eff7..e2f4450 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1189,7 +1189,7 @@ int vlapic_has_pending_irq(struct vcpu *v) * comparing with the IRR. */ vector = viridian_complete_apic_assist(v); - if ( vector != -1 ) + if ( vector ) vlapic_clear_vector(vector, &vlapic->regs->data[APIC_ISR]); isr = vlapic_find_highest_isr(vlapic); diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index fbd1c6a..8d73b51 100644 --- a/xen/include/public/arch-x86/hvm/save.h +++ b/xen/include/public/arch-x86/hvm/save.h @@ -588,7 +588,9 @@ struct hvm_viridian_domain_context { DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context); struct hvm_viridian_vcpu_context { - uint64_t apic_assist; + uint64_t apic_assist_msr; + uint8_t apic_assist_vector; + uint8_t _pad[7]; }; DECLARE_HVM_SAVE_TYPE(VIRIDIAN_VCPU, 17, struct hvm_viridian_vcpu_context);
If any vcpu has a pending APIC assist when the domain is suspended then the vector needs to be saved. If this is not done then it's possible for the vector to remain pending in the vlapic ISR indefinitely after resume. This patch adds code to save the APIC assist vector value in the viridian vcpu save record. This means that the record is now zero- extended on load and, because this implies a loaded value of zero means nothing is pending (for backwards compatibility with hosts not implementing APIC assist), the rest of the viridian APIC assist code is adjusted to treat a zero value in this way. A check has therefore been added to viridian_start_apic_assist() to prevent the enlightenment being used for vectors < 0x10 (which are illegal for an APIC). Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> --- v2: - don't use biasing - add missing padding to save record --- xen/arch/x86/hvm/viridian.c | 23 ++++++++++++++--------- xen/arch/x86/hvm/vlapic.c | 2 +- xen/include/public/arch-x86/hvm/save.h | 4 +++- 3 files changed, 18 insertions(+), 11 deletions(-)