[v6,02/11] x86/vlapic: Move lapic migration checks to the check hooks

Commit Message

Alejandro Vallejo Oct. 1, 2024, 12:37 p.m. UTC

While doing this, factor out checks common to architectural and hidden
state.

Signed-off-by: Alejandro Vallejo <[e-mail]>
Reviewed-by: Roger Pau Monné <[e-mail]>
--
Last reviewed in the topology series v3. Fell under the cracks.

  https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/
---
 xen/arch/x86/hvm/vlapic.c | 84 ++++++++++++++++++++++++++-------------
 1 file changed, 56 insertions(+), 28 deletions(-)

Comments

Jan Beulich Oct. 8, 2024, 3:41 p.m. UTC | #1

On 01.10.2024 14:37, Alejandro Vallejo wrote:
> While doing this, factor out checks common to architectural and hidden
> state.
> 
> Signed-off-by: Alejandro Vallejo <[e-mail]>
> Reviewed-by: Roger Pau Monné <[e-mail]>
> --
> Last reviewed in the topology series v3. Fell under the cracks.
> 
>   https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/

It's not the 1st patch in the series, and I can't spot anywhere that it is
made clear that this one can go in ahead of patch 1. I may have overlooked
something in the long-ish cover letter.

Jan

Alejandro Vallejo Oct. 9, 2024, 4:11 p.m. UTC | #2

On Tue Oct 8, 2024 at 4:41 PM BST, Jan Beulich wrote:
> On 01.10.2024 14:37, Alejandro Vallejo wrote:
> > While doing this, factor out checks common to architectural and hidden
> > state.
> > 
> > Signed-off-by: Alejandro Vallejo <[e-mail]>
> > Reviewed-by: Roger Pau Monné <[e-mail]>
> > --
> > Last reviewed in the topology series v3. Fell under the cracks.
> > 
> >   https://lore.kernel.org/xen-devel/ZlhP11Vvk6c1Ix36@macbook/
>
> It's not the 1st patch in the series, and I can't spot anywhere that it is
> made clear that this one can go in ahead of patch 1. I may have overlooked
> something in the long-ish cover letter.
>
> Jan

Patch 1 is independent of almost everything. It merely needs to go in before
the final patch to avoid turning it into a bugfix. I put it on front under the
expectation that it wouldn't be very contentious. In retrospect, this one ought
to have taken its place, indeed.

Cheers,
Alejandro

Patch

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 992355e511cd..101902cff889 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1571,60 +1571,88 @@  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",
+        dprintk(XENLOG_G_ERR, "HVM restore: dom%d has no vCPU %u\n",
                 d->domain_id, vcpuid);
         return -EINVAL;
     }
-    s = vcpu_vlapic(v);
+
+    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 = 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];
+    struct vlapic *s = vcpu_vlapic(v);
 
     if ( hvm_load_entry_zeroextend(LAPIC, h, &s->hw) != 0 )
+    {
+        ASSERT_UNREACHABLE();
         return -EINVAL;
+    }
 
     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;
+        ASSERT_UNREACHABLE();
 
     s->loaded.id = vlapic_get_reg(s, APIC_ID);
     s->loaded.ldr = vlapic_get_reg(s, APIC_LDR);
@@ -1641,9 +1669,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)