[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.