| Message ID | f87f27de6fadf896bf4d50c8a37e2b9b7201afb2.1716976271.git.alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
On Wed, May 29, 2024 at 03:32:30PM +0100, Alejandro Vallejo wrote: > While doing this, factor out checks common to architectural and hidden state. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Reviewed-by: Roger PAu Monné <roger.pau@citrix.com> With the BUG() possibly replaced with ASSERT_UNREACHABLE(), > --- > v3: > * Moved from v2/patch3. > * Added check hook for the architectural state as well. > * Use domain_vcpu() rather than the previous open coded checks for vcpu range. > --- > xen/arch/x86/hvm/vlapic.c | 81 +++++++++++++++++++++++++-------------- > 1 file changed, 53 insertions(+), 28 deletions(-) > > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 9cfc82666ae5..a0df62b5ec0a 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -1553,60 +1553,85 @@ static void lapic_load_fixup(struct vlapic *vlapic) > v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); > } > > -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > -{ > - unsigned int vcpuid = hvm_load_instance(h); > - struct vcpu *v; > - struct vlapic *s; > > +static int lapic_check_common(const struct domain *d, unsigned int vcpuid) > +{ > if ( !has_vlapic(d) ) > return -ENODEV; > > /* Which vlapic to load? */ > - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) > + if ( !domain_vcpu(d, vcpuid) ) > { > dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", > d->domain_id, vcpuid); The message here is kind of misleading as printing apic%u makes it look like it's an APIC ID, but it's a vCPU ID. It would be best to just print: "HVM restore: dom%d has no vCPU %u\n". > return -EINVAL; > } > - s = vcpu_vlapic(v); > > - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > + return 0; > +} > + > +static int cf_check lapic_check_hidden(const struct domain *d, > + hvm_domain_context_t *h) > +{ > + unsigned int vcpuid = hvm_load_instance(h); > + struct hvm_hw_lapic s; > + int rc; > + > + if ( (rc = lapic_check_common(d, vcpuid)) ) > + return rc; Nit: I don't like much to assign values inside of conditions, I would rather do: int rc = lapic_check_common(d, vcpuid); if ( rc ) return rc; > + > + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) != 0 ) > + return -ENODATA; > + > + /* EN=0 with EXTD=1 is illegal */ > + if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) == > + APIC_BASE_EXTD ) > return -EINVAL; > > + return 0; > +} > + > +static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) > +{ > + unsigned int vcpuid = hvm_load_instance(h); > + struct vcpu *v = d->vcpu[vcpuid]; Not sure whether it's worth using domain_vcpu() here. We have already checked the vCPU is valid. > + struct vlapic *s = vcpu_vlapic(v); > + > + if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) > + BUG(); I would use { ASSERT_UNREACHABLE(); return -EINVAL; } here, there's IMO no strict reason to panic on non-debug builds. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 9cfc82666ae5..a0df62b5ec0a 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1553,60 +1553,85 @@ static void lapic_load_fixup(struct vlapic *vlapic) v, vlapic->loaded.id, vlapic->loaded.ldr, good_ldr); } -static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) -{ - unsigned int vcpuid = hvm_load_instance(h); - struct vcpu *v; - struct vlapic *s; +static int lapic_check_common(const struct domain *d, unsigned int vcpuid) +{ if ( !has_vlapic(d) ) return -ENODEV; /* Which vlapic to load? */ - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) + if ( !domain_vcpu(d, vcpuid) ) { dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", d->domain_id, vcpuid); return -EINVAL; } - s = vcpu_vlapic(v); - if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) + return 0; +} + +static int cf_check lapic_check_hidden(const struct domain *d, + hvm_domain_context_t *h) +{ + unsigned int vcpuid = hvm_load_instance(h); + struct hvm_hw_lapic s; + int rc; + + if ( (rc = lapic_check_common(d, vcpuid)) ) + return rc; + + if ( hvm_load_entry_zeroextend(LAPIC, h, &s) != 0 ) + return -ENODATA; + + /* EN=0 with EXTD=1 is illegal */ + if ( (s.apic_base_msr & (APIC_BASE_ENABLE | APIC_BASE_EXTD)) == + APIC_BASE_EXTD ) return -EINVAL; + return 0; +} + +static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h) +{ + unsigned int vcpuid = hvm_load_instance(h); + struct vcpu *v = d->vcpu[vcpuid]; + struct vlapic *s = vcpu_vlapic(v); + + if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 ) + BUG(); + s->loaded.hw = 1; if ( s->loaded.regs ) lapic_load_fixup(s); - if ( !(s->hw.apic_base_msr & APIC_BASE_ENABLE) && - unlikely(vlapic_x2apic_mode(s)) ) - return -EINVAL; - hvm_update_vlapic_mode(v); return 0; } -static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h) +static int cf_check lapic_check_regs(const struct domain *d, + hvm_domain_context_t *h) { unsigned int vcpuid = hvm_load_instance(h); - struct vcpu *v; - struct vlapic *s; + int rc; - if ( !has_vlapic(d) ) - return -ENODEV; + if ( (rc = lapic_check_common(d, vcpuid)) ) + return rc; - /* Which vlapic to load? */ - if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL ) - { - dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no apic%u\n", - d->domain_id, vcpuid); - return -EINVAL; - } - s = vcpu_vlapic(v); + if ( !hvm_get_entry(LAPIC_REGS, h) ) + return -ENODATA; + + return 0; +} + +static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h) +{ + unsigned int vcpuid = hvm_load_instance(h); + struct vcpu *v = d->vcpu[vcpuid]; + struct vlapic *s = vcpu_vlapic(v); if ( hvm_load_entry(LAPIC_REGS, h, s->regs) != 0 ) - return -EINVAL; + BUG(); s->loaded.id = vlapic_get_reg(s, APIC_ID); s->loaded.ldr = vlapic_get_reg(s, APIC_LDR); @@ -1623,9 +1648,9 @@ static int cf_check lapic_load_regs(struct domain *d, hvm_domain_context_t *h) return 0; } -HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL, +HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, lapic_check_hidden, lapic_load_hidden, 1, HVMSR_PER_VCPU); -HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL, +HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_check_regs, lapic_load_regs, 1, HVMSR_PER_VCPU); int vlapic_init(struct vcpu *v)
While doing this, factor out checks common to architectural and hidden state. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v3: * Moved from v2/patch3. * Added check hook for the architectural state as well. * Use domain_vcpu() rather than the previous open coded checks for vcpu range. --- xen/arch/x86/hvm/vlapic.c | 81 +++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 28 deletions(-)