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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.18 at 14:02, <aisaila@xxxxxxxxxxxxxxx> wrote:
> On Fri, 2018-09-07 at 03:43 -0600, Jan Beulich wrote:
>> > > > 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
>> 
>> 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.
> 
> Sorry for the misunderstanding of this matter but in v16 I had
> 
>  +    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU &&
>  +        instance >= d->max_vcpus )
>  +        return -ENOENT;
>  +    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_DOM )
>  +        instance = 0;
> 
> and this solved the save call for per_dom having instance = 0.
> Later, in v17 I dropped the PER_DOM if and in v18 I dropped the &&
> PER_VCPU. 
> 
> Now the question is how exactly should I go with this?

PER_VCPU instances need to be bounded. PER_DOM ones need
to be left alone (and vCPU 0 used for access).

>> >      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.
>> 
> I can add a switch (kind) here and for the PER_VCPU case to call
> save(d->vcpu[instance]) then for PER_DOM case call save(d->vcpu[0]). 
> 
> Is this a good way to go?

Perhaps, but I think you'd be better off latching the subject vCPU
into a local variable.

Jan



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