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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 09, 2024 at 11:58:48AM +0100, Jan Beulich wrote:
> 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:
> >>>> @@ -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.

I was going to suggest to add the pure attribute to the function, but
it does have side effects as it prints to the console.

> 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.

Why do you mention the messages appearing twice?  Won't the first
check call return error and thus should prevent the caller from
attempting to load the state?

Thanks, Roger.