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.vall...@cloud.com>
Reviewed-by: Roger PAu Monné <roger....@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.
