Re: [Xen-devel] [PATCH v18 12/13] x86/hvm: Remove redundant save functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.09.18 at 15:14, <aisaila@xxxxxxxxxxxxxxx> wrote:
> This patch removes the redundant save functions and renames the
> save_one* to save. It then changes the domain param to vcpu in the
> save funcs and adapts print messages in order to match the format of the
> other save related messages.
> 
> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
> 
> ---
> Changes since V17:
>       - Refit HVM_REGISTER_SAVE_RESTORE(CPU)
>       - Add const to the added struct domain *d

How does this match up with ...

> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -516,16 +516,17 @@ static const struct hvm_mmio_ops hpet_mmio_ops = {
>  };
>  
>  
> -static int hpet_save(struct domain *d, hvm_domain_context_t *h)
> +static int hpet_save(struct vcpu *v, hvm_domain_context_t *h)
>  {
> +    struct domain *d = v->domain;

... this and at least the vPIT and vRTC cases as well? It's not clear to me
why const couldn't be added here; it pretty clearly can't be added in the
vPIT case, but then in the vRTC it looks to be possible again. Especially if
you give a broad/unspecific statement as above, please make sure you've
also done the conversion uniformly.

> @@ -148,14 +145,14 @@ int hvm_save_one(struct domain *d, unsigned int 
> typecode, unsigned int instance,
>           !hvm_sr_handlers[typecode].save )
>          return -EINVAL;
>  
> +    if ( instance >= d->max_vcpus )
> +        return -ENOENT;

Excuse me, but how many more times do I need to point out that this
is wrong? Once again for a single-vCPU guest saving the second PIC
instance will become impossible with this. Just to re-iterate: The check
above has to be limited to just HVMSR_PER_VCPU kind records.

>      ctxt.size = hvm_sr_handlers[typecode].size;
> -    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
> -        ctxt.size *= d->max_vcpus;
>      ctxt.data = xmalloc_bytes(ctxt.size);
>      if ( !ctxt.data )
>          return -ENOMEM;
>  
> -    if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
> +    if ( (rv = hvm_sr_handlers[typecode].save(d->vcpu[instance], &ctxt)) != 
> 0 )

And you need to use d->vcpu[0] for all others here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel