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
