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

From: Isaila Alexandru <aisaila@bitdefender.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>,
	xen-devel@lists.xen.org
Subject: Re: [PATCH v14 08/11] x86/hvm: Add handler for save_one funcs
Date: Tue, 31 Jul 2018 16:45:49 +0300	[thread overview]
Message-ID: <1533044749.21125.27.camel@bitdefender.com> (raw)
In-Reply-To: <5B6064F902000078001D977E@prv1-mh.provo.novell.com>

On Ma, 2018-07-31 at 07:32 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 31.07.18 at 14:55, <aisaila@bitdefender.com> wrote:
> > On Ma, 2018-07-31 at 06:34 -0600, Jan Beulich wrote:
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > On 25.07.18 at 14:14, <aisaila@bitdefender.com> wrote:
> > > > --- 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)?
> > Both LAPIC save functions have for for (vcpu) so the look like a
> > save_one function already, no need to do anything here.
> Quite the opposite - presence of a loop over all vCPU-s clearly
> says they're not save-one functions. Otherwise you wouldn't
> have found the need to touch the functions the way you do in
> patch 10.
> 
> > 
> > > 
> > > > 
> > > > @@ -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.
> > This was done so we can add NULL in the places that do not have
> > save_one functions.
> ??? (I cannot connect your response to my remark.)
> 
If there is &_save_one then it will not compile if there is any call
with a NULL.

hpet.c: In function ‘__hvm_register_HPET_save_and_restore’:
/home/aisaila/work/xen/xen/include/asm/hvm/save.h:126:25: error: lvalue
required as unary ‘&’ operand
                         &_save_one,                                   
    \
                         ^
hpet.c:643:1: note: in expansion of macro ‘HVM_REGISTER_SAVE_RESTORE’
 HVM_REGISTER_SAVE_RESTORE(HPET, hpet_save, NULL, hpet_load, 1,
HVMSR_PER_DOM);

Alex

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