Re: [Xen-devel] [PATCH v14 08/11] x86/hvm: Add handler for save_one funcs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.07.18 at 14:14, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -85,16 +85,18 @@ int arch_hvm_load(struct domain *d, struct 
> hvm_save_header *hdr)
>  /* List of handlers for various HVM save and restore types */
>  static struct {
>      hvm_save_handler save;
> +    hvm_save_one_handler save_one;
>      hvm_load_handler load;
>      const char *name;
>      size_t size;
>      int kind;
> -} hvm_sr_handlers[HVM_SAVE_CODE_MAX + 1] = { {NULL, NULL, "<?>"}, };
> +} hvm_sr_handlers[HVM_SAVE_CODE_MAX + 1] = { {NULL, NULL, NULL, "<?>"}, };

This initializer looks flawed, and I'd appreciate if you could fix it as
you have to touch it anyway: It only sets .name of array entry 0
to a non-NULL string. Either this setting is not needed in the first
place (as looks to be the case), or you'll want to initialize all array
entries the same. Use the C99 (GNU extension in C89) for that
purpose. Perhaps simply dropping the initializer could be prereq
patch which could go in right away.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1576,9 +1576,9 @@ static int lapic_load_regs(struct domain *d, 
> hvm_domain_context_t *h)
>      return 0;
>  }
>  
> -HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, lapic_load_hidden,
> +HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_save_hidden, NULL, lapic_load_hidden,
>                            1, HVMSR_PER_VCPU);
> -HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, lapic_load_regs,
> +HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, NULL, lapic_load_regs,

These are per-vCPU as well - why do they get NULL inserted here,
rather than there being another (two) prereq patch(es)?

> --- a/xen/include/asm-x86/hvm/save.h
> +++ b/xen/include/asm-x86/hvm/save.h
> @@ -97,6 +97,8 @@ static inline uint16_t hvm_load_instance(struct 
> hvm_domain_context *h)
>   * restoring.  Both return non-zero on error. */
>  typedef int (*hvm_save_handler) (struct domain *d, 
>                                   hvm_domain_context_t *h);
> +typedef int (*hvm_save_one_handler)(struct  vcpu *v,
> +                                    hvm_domain_context_t *h);

I don't think "one" is a valid part of the name here: PIC has
multiple instances too (and eventually IO-APIC will), yet they're
not to be managed this way. I think you want to use "vcpu"
instead.

> @@ -114,12 +117,13 @@ void hvm_register_savevm(uint16_t typecode,
>  
>  /* Syntactic sugar around that function: specify the max number of
>   * saves, and this calculates the size of buffer needed */
> -#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num, _k)             \
> +#define HVM_REGISTER_SAVE_RESTORE(_x, _save, _save_one, _load, _num, _k)  \
>  static int __init __hvm_register_##_x##_save_and_restore(void)            \
>  {                                                                         \
>      hvm_register_savevm(HVM_SAVE_CODE(_x),                                \
>                          #_x,                                              \
>                          &_save,                                           \
> +                        _save_one,                                        \

While I generally appreciate the omission of the &, I'd
prefer if you added it for consistency with the neighboring
lines.

Jan



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