1. Patchew
  2. Xen
  3. [PATCH v3 0/6] x86: Expose consistent topology to guests
  4. View patch

[PATCH v3 1/6] x86/vlapic: Move lapic migration checks to the check hooks

Alejandro Vallejo posted 6 patches 5 months, 4 weeks ago
There is a newer version of this series
[PATCH v3 1/6] x86/vlapic: Move lapic migration checks to the check hooks
Posted by Alejandro Vallejo 5 months, 4 weeks ago 
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(-)

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)
-- 
2.34.1
Re: [PATCH v3 1/6] x86/vlapic: Move lapic migration checks to the check hooks
Posted by Roger Pau Monné 5 months, 4 weeks ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.