Re: [PATCH v4 1/5] x86/HVM: split restore state checking from state loading

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Dec 18, 2023 at 03:39:55PM +0100, Jan Beulich wrote:
> ..., at least as reasonably feasible without making a check hook
> mandatory (in particular strict vs relaxed/zero-extend length checking
> can't be done early this way).
> 
> Note that only one of the two uses of "real" hvm_load() is accompanied
> with a "checking" one. The other directly consumes hvm_save() output,
> which ought to be well-formed. This means that while input data related
> checks don't need repeating in the "load" function when already done by
> the "check" one (albeit assertions to this effect may be desirable),
> domain state related checks (e.g. has_xyz(d)) will be required in both
> places.
> 
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Now that this re-arranges hvm_load() anyway, wouldn't it be better to
> down the vCPU-s ahead of calling arch_hvm_load() (which is now easy to
> arrange for)?

Seems OK to me.

> Do we really need all the copying involved in use of _hvm_read_entry()
> (backing hvm_load_entry()? Zero-extending loads are likely easier to
> handle that way, but for strict loads all we gain is a reduced risk of
> unaligned accesses (compared to simply pointing into h->data[]).

I do feel it's safer to copy the data so the checks are done against
what's loaded.  Albeit hvm_load() is already using hvm_get_entry().

> ---
> v4: Fold hvm_check() into hvm_load().
> v2: New.
> 
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -379,8 +379,12 @@ long arch_do_domctl(
>          if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 
> 0 )
>              goto sethvmcontext_out;
>  
> +        ret = hvm_load(d, false, &c);
> +        if ( ret )
> +            goto sethvmcontext_out;
> +
>          domain_pause(d);
> -        ret = hvm_load(d, &c);
> +        ret = hvm_load(d, true, &c);

Now that the check has been done ahead, do we want to somehow assert
that this cannot fail?  AIUI that's the expectation.

>          domain_unpause(d);
>  
>      sethvmcontext_out:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5397,7 +5397,7 @@ int hvm_copy_context_and_params(struct d
>      }
>  
>      c.cur = 0;
> -    rc = hvm_load(dst, &c);
> +    rc = hvm_load(dst, true, &c);
>  
>   out:
>      vfree(c.data);
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -30,7 +30,8 @@ static void arch_hvm_save(struct domain
>      d->arch.hvm.sync_tsc = rdtsc();
>  }
>  
> -static int arch_hvm_load(struct domain *d, const struct hvm_save_header *hdr)
> +static int arch_hvm_check(const struct domain *d,
> +                          const struct hvm_save_header *hdr)
>  {
>      uint32_t eax, ebx, ecx, edx;
>  
> @@ -55,6 +56,11 @@ static int arch_hvm_load(struct domain *
>                 "(%#"PRIx32") and restored on another (%#"PRIx32").\n",
>                 d->domain_id, hdr->cpuid, eax);
>  
> +    return 0;
> +}
> +
> +static void arch_hvm_load(struct domain *d, const struct hvm_save_header 
> *hdr)
> +{
>      /* Restore guest's preferred TSC frequency. */
>      if ( hdr->gtsc_khz )
>          d->arch.tsc_khz = hdr->gtsc_khz;
> @@ -66,13 +72,12 @@ static int arch_hvm_load(struct domain *
>  
>      /* VGA state is not saved/restored, so we nobble the cache. */
>      d->arch.hvm.stdvga.cache = STDVGA_CACHE_DISABLED;
> -
> -    return 0;
>  }
>  
>  /* List of handlers for various HVM save and restore types */
>  static struct {
>      hvm_save_handler save;
> +    hvm_check_handler check;
>      hvm_load_handler load;
>      const char *name;
>      size_t size;
> @@ -88,6 +93,7 @@ void __init hvm_register_savevm(uint16_t
>  {
>      ASSERT(typecode <= HVM_SAVE_CODE_MAX);
>      ASSERT(hvm_sr_handlers[typecode].save == NULL);
> +    ASSERT(hvm_sr_handlers[typecode].check == NULL);
>      ASSERT(hvm_sr_handlers[typecode].load == NULL);
>      hvm_sr_handlers[typecode].save = save_state;
>      hvm_sr_handlers[typecode].load = load_state;
> @@ -275,12 +281,10 @@ int hvm_save(struct domain *d, hvm_domai
>      return 0;
>  }
>  
> -int hvm_load(struct domain *d, hvm_domain_context_t *h)
> +int hvm_load(struct domain *d, bool real, hvm_domain_context_t *h)

Maybe the 'real' parameter should instead be an enum:

enum hvm_load_action {
    CHECK,
    LOAD,
};
int hvm_load(struct domain *d, enum hvm_load_action action,
             hvm_domain_context_t *h);

Otherwise a comment might be warranted about how 'real' affects the
logic in the function.

>  {
>      const struct hvm_save_header *hdr;
>      struct hvm_save_descriptor *desc;
> -    hvm_load_handler handler;
> -    struct vcpu *v;
>      int rc;
>  
>      if ( d->is_dying )
> @@ -291,50 +295,91 @@ int hvm_load(struct domain *d, hvm_domai
>      if ( !hdr )
>          return -ENODATA;
>  
> -    rc = arch_hvm_load(d, hdr);
> -    if ( rc )
> -        return rc;
> +    rc = arch_hvm_check(d, hdr);

Shouldn't this _check function only be called when real == false?

> +    if ( real )
> +    {
> +        struct vcpu *v;
> +
> +        ASSERT(!rc);
> +        arch_hvm_load(d, hdr);
>  
> -    /* Down all the vcpus: we only re-enable the ones that had state saved. 
> */
> -    for_each_vcpu(d, v)
> -        if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
> -            vcpu_sleep_nosync(v);
> +        /*
> +         * Down all the vcpus: we only re-enable the ones that had state
> +         * saved.
> +         */
> +        for_each_vcpu(d, v)
> +            if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
> +                vcpu_sleep_nosync(v);
> +    }
> +    else if ( rc )
> +        return rc;
>  
>      for ( ; ; )
>      {
> +        const char *name;
> +        hvm_load_handler load;
> +
>          if ( h->size - h->cur < sizeof(struct hvm_save_descriptor) )
>          {
>              /* Run out of data */
>              printk(XENLOG_G_ERR
>                     "HVM%d restore: save did not end with a null entry\n",
>                     d->domain_id);
> +            ASSERT(!real);
>              return -ENODATA;
>          }
>  
>          /* Read the typecode of the next entry  and check for the end-marker 
> */
>          desc = (struct hvm_save_descriptor *)(&h->data[h->cur]);
> -        if ( desc->typecode == 0 )
> +        if ( desc->typecode == HVM_SAVE_CODE(END) )
> +        {
> +            /* Reset cursor for hvm_load(, true, ). */
> +            if ( !real )
> +                h->cur = 0;
>              return 0;
> +        }
>  
>          /* Find the handler for this entry */
> -        if ( (desc->typecode > HVM_SAVE_CODE_MAX) ||
> -             ((handler = hvm_sr_handlers[desc->typecode].load) == NULL) )
> +        if ( desc->typecode >= ARRAY_SIZE(hvm_sr_handlers) ||
> +             !(name = hvm_sr_handlers[desc->typecode].name) ||
> +             !(load = hvm_sr_handlers[desc->typecode].load) )
>          {
>              printk(XENLOG_G_ERR "HVM%d restore: unknown entry typecode %u\n",
>                     d->domain_id, desc->typecode);

The message is not very accurate here, it does fail when the typecode
is unknown, but also fails when such typecode has no name or load
function setup.

Thanks, Roger.