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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.01.2024 11:26, Roger Pau Monné wrote:
> On Tue, Dec 19, 2023 at 04:24:02PM +0100, Jan Beulich wrote:
>> On 19.12.2023 15:36, Roger Pau Monné wrote:
>>> On Mon, Dec 18, 2023 at 03:39:55PM +0100, Jan Beulich wrote:
>>>> --- 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.
>>
>> We certainly can't until all checking was moved out of the load handlers.
>> And even then I think there are still cases where load might produce an
>> error. (In fact I would have refused a little more strongly to folding
>> the prior hvm_check() into hvm_load() if indeed a separate hvm_load()
>> could have ended up returning void in the long run.)
> 
> I see, _load could fail even if all the data provided was correct, for
> example because the hypervisor is OoM?

That's the primary hypothetical cause for such a failure, yes.

>>>> @@ -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?
>>
>> Possibly. In v4 I directly transformed what I had in v3:
>>
>>     ASSERT(!arch_hvm_check(d, hdr));
>>
>> I.e. it is now the call above plus ...
>>
>>>> +    if ( real )
>>>> +    {
>>>> +        struct vcpu *v;
>>>> +
>>>> +        ASSERT(!rc);
>>
>> ... this assertion. Really the little brother of the call site assertion
>> you're asking for (see above).
>>
>>>> +        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;
> 
> The issue I see with this is that when built with debug=n the call to
> arch_hvm_check() with real == true is useless, as the result is never
> evaluated - IOW: would be clearer to just avoid the call altogether.

Which, besides being imo slightly worse for then having two call sites,
puts me in a difficult position: It may not have been here, but on
another patch (but I think it was an earlier version of this one)
where Andrew commented on

    ASSERT(func());

as generally being a disliked pattern, for having a "side effect" in
the expression of an assertion. Plus the call isn't pointless even in
release builds, because of the log messages issued: Them appearing
twice in close succession might be a good hint of something fishy
going on.

Jan