| Message ID | 1458839997-6597-1-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/03/16 17:19, Paul Durrant wrote: > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int vector) > * 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 != 0 ) > domain_crash(v->domain); > > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; > + /* Increment the value so that zero is always invalid. */ > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; Can you add a comment to viridan_cpu that a variable named vector actually holds "vector + 1" ? It can also be changed to an unsigned value. > *va |= 1u; > } > > diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h > index fbd1c6a..fc8e8f9 100644 > --- a/xen/include/public/arch-x86/hvm/save.h > +++ b/xen/include/public/arch-x86/hvm/save.h > @@ -588,7 +588,8 @@ 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; > + uint16_t apic_assist_vector; An explicit uint16_t[3] _pad; please. ~Andrew
>>> On 24.03.16 at 18:19, <paul.durrant@citrix.com> wrote: > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int vector) > * 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 != 0 ) > domain_crash(v->domain); > > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; > + /* Increment the value so that zero is always invalid. */ > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; From an APIC perspective, aren't vectors below 0x10 invalid anyway? I.e. can't zero serve the "none" indication just as much as -1 did without this new biasing? Or otherwise I'd still expect ... > @@ -829,13 +830,21 @@ 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); > > + /* > + * Guests that were saved on a host that did not use APIC assist > + * will clearly never have a pending assist, so reading the zero > + * value for apic_assist_vector from the zero extended save record > + * will always be correct. > + */ > + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; ... the bias to get accounted for here, instead of quite a bit of other code getting adjusted, and the meaning of the "vector" field getting slightly mis-used. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 29 March 2016 09:42 > To: Paul Durrant > Cc: xen-devel@lists.xenproject.org > Subject: Re: [PATCH] x86/hvm/viridian: save APIC assist vector > > >>> On 24.03.16 at 18:19, <paul.durrant@citrix.com> wrote: > > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int > vector) > > * 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 != 0 ) > > domain_crash(v->domain); > > > > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; > > + /* Increment the value so that zero is always invalid. */ > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; > > From an APIC perspective, aren't vectors below 0x10 invalid > anyway? I.e. can't zero serve the "none" indication just as much > as -1 did without this new biasing? Or otherwise I'd still expect ... > I can do that, if you're happy with it. If I'm going to do it this way though I will also put in an explicit check to make sure APIC assist is not used for vectors < 0x16. Paul > > @@ -829,13 +830,21 @@ 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); > > > > + /* > > + * Guests that were saved on a host that did not use APIC assist > > + * will clearly never have a pending assist, so reading the zero > > + * value for apic_assist_vector from the zero extended save record > > + * will always be correct. > > + */ > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; > > ... the bias to get accounted for here, instead of quite a bit of > other code getting adjusted, and the meaning of the "vector" > field getting slightly mis-used. > > Jan
> -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 24 March 2016 17:58 > To: Paul Durrant; xen-devel@lists.xenproject.org > Cc: Jan Beulich > Subject: Re: [Xen-devel] [PATCH] x86/hvm/viridian: save APIC assist vector > > On 24/03/16 17:19, Paul Durrant wrote: > > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int > vector) > > * 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 != 0 ) > > domain_crash(v->domain); > > > > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; > > + /* Increment the value so that zero is always invalid. */ > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; > > Can you add a comment to viridan_cpu that a variable named vector > actually holds "vector + 1" ? > > It can also be changed to an unsigned value. > > > *va |= 1u; > > } > > > > diff --git a/xen/include/public/arch-x86/hvm/save.h > b/xen/include/public/arch-x86/hvm/save.h > > index fbd1c6a..fc8e8f9 100644 > > --- a/xen/include/public/arch-x86/hvm/save.h > > +++ b/xen/include/public/arch-x86/hvm/save.h > > @@ -588,7 +588,8 @@ 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; > > + uint16_t apic_assist_vector; > > An explicit uint16_t[3] _pad; please. > Given that I'm going to drop the biasing technique, I'll drop this to a uint8_t and add appropriate padding as requested. Paul > ~Andrew
> -----Original Message----- > From: Paul Durrant > Sent: 29 March 2016 09:57 > To: 'Jan Beulich' > Cc: xen-devel@lists.xenproject.org > Subject: RE: [PATCH] x86/hvm/viridian: save APIC assist vector > > > -----Original Message----- > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: 29 March 2016 09:42 > > To: Paul Durrant > > Cc: xen-devel@lists.xenproject.org > > Subject: Re: [PATCH] x86/hvm/viridian: save APIC assist vector > > > > >>> On 24.03.16 at 18:19, <paul.durrant@citrix.com> wrote: > > > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, > int > > vector) > > > * 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 != 0 ) > > > domain_crash(v->domain); > > > > > > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; > > > + /* Increment the value so that zero is always invalid. */ > > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; > > > > From an APIC perspective, aren't vectors below 0x10 invalid > > anyway? I.e. can't zero serve the "none" indication just as much > > as -1 did without this new biasing? Or otherwise I'd still expect ... > > > > I can do that, if you're happy with it. If I'm going to do it this way though I will > also put in an explicit check to make sure APIC assist is not used for vectors < > 0x16. Obviously I meant 0x10 there ;-) Paul > > Paul > > > > @@ -829,13 +830,21 @@ 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); > > > > > > + /* > > > + * Guests that were saved on a host that did not use APIC assist > > > + * will clearly never have a pending assist, so reading the zero > > > + * value for apic_assist_vector from the zero extended save record > > > + * will always be correct. > > > + */ > > > + v->arch.hvm_vcpu.viridian.apic_assist.vector = > ctxt.apic_assist_vector; > > > > ... the bias to get accounted for here, instead of quite a bit of > > other code getting adjusted, and the meaning of the "vector" > > field getting slightly mis-used. > > > > Jan
>>> On 29.03.16 at 10:57, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 29 March 2016 09:42 >> To: Paul Durrant >> Cc: xen-devel@lists.xenproject.org >> Subject: Re: [PATCH] x86/hvm/viridian: save APIC assist vector >> >> >>> On 24.03.16 at 18:19, <paul.durrant@citrix.com> wrote: >> > @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int >> vector) >> > * 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 != 0 ) >> > domain_crash(v->domain); >> > >> > - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; >> > + /* Increment the value so that zero is always invalid. */ >> > + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; >> >> From an APIC perspective, aren't vectors below 0x10 invalid >> anyway? I.e. can't zero serve the "none" indication just as much >> as -1 did without this new biasing? Or otherwise I'd still expect ... >> > > I can do that, if you're happy with it. If I'm going to do it this way > though I will also put in an explicit check to make sure APIC assist is not > used for vectors < 0x16. As long as you mean 0x10 or 16, yes (at once allowing the fields to be uint8_t, as would be customary for vectors). I had actually half written a comment to that effect on the original patch, until I realized that the only harm caused by using a bogus low vector would be to the guest itself. Jan
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 410320c..8e858d2 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; } @@ -293,10 +292,11 @@ void viridian_start_apic_assist(struct vcpu *v, int vector) * 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 != 0 ) domain_crash(v->domain); - v->arch.hvm_vcpu.viridian.apic_assist.vector = vector; + /* Increment the value so that zero is always invalid. */ + v->arch.hvm_vcpu.viridian.apic_assist.vector = vector + 1; *va |= 1u; } @@ -311,8 +311,8 @@ int viridian_complete_apic_assist(struct vcpu *v) if ( *va & 1u ) return -1; /* 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; + vector = v->arch.hvm_vcpu.viridian.apic_assist.vector - 1; + v->arch.hvm_vcpu.viridian.apic_assist.vector = 0; return vector; } @@ -325,7 +325,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 +806,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 +830,21 @@ 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); + /* + * Guests that were saved on a host that did not use APIC assist + * will clearly never have a pending assist, so reading the zero + * value for apic_assist_vector from the zero extended save record + * will always be correct. + */ + v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector; + return 0; } diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index fbd1c6a..fc8e8f9 100644 --- a/xen/include/public/arch-x86/hvm/save.h +++ b/xen/include/public/arch-x86/hvm/save.h @@ -588,7 +588,8 @@ 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; + uint16_t apic_assist_vector; }; 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. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/hvm/viridian.c | 27 ++++++++++++++++++--------- xen/include/public/arch-x86/hvm/save.h | 3 ++- 2 files changed, 20 insertions(+), 10 deletions(-)