Re: [PATCH v3 2/6] x86/HVM: split restore state checking from state loading

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Dec 06, 2023 at 08:27:59AM +0100, Jan Beulich wrote:
> On 05.12.2023 16:55, Roger Pau Monné wrote:
> > On Tue, Dec 05, 2023 at 03:59:13PM +0100, Jan Beulich wrote:
> >> On 05.12.2023 15:29, Roger Pau Monné wrote:
> >>> On Tue, Dec 05, 2023 at 09:52:31AM +0100, Jan Beulich wrote:
> >>>> On 04.12.2023 18:27, Roger Pau Monné wrote:
> >>>>> On Tue, Nov 28, 2023 at 11:34:04AM +0100, Jan Beulich wrote:
> >>>>>> ..., at least as reasonably feasible without making a check hook
> >>>>>> mandatory (in particular strict vs relaxed/zero-extend length checking
> >>>>>> can't be done early this way).
> >>>>>>
> >>>>>> Note that only one of the two uses of hvm_load() is accompanied with
> >>>>>> hvm_check(). The other directly consumes hvm_save() output, which ought
> >>>>>> to be well-formed. This means that while input data related checks don't
> >>>>>> need repeating in the "load" function when already done by the "check"
> >>>>>> one (albeit assertions to this effect may be desirable), domain state
> >>>>>> related checks (e.g. has_xyz(d)) will be required in both places.
> >>>>>>
> >>>>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>> ---
> >>>>>> Do we really need all the copying involved in use of _hvm_read_entry()
> >>>>>> (backing hvm_load_entry()? Zero-extending loads are likely easier to
> >>>>>> handle that way, but for strict loads all we gain is a reduced risk of
> >>>>>> unaligned accesses (compared to simply pointing into h->data[]).
> >>>>>
> >>>>> See below, but I wonder whether the checks could be performed as part
> >>>>> of hvm_load() without having to introduce a separate handler and loop
> >>>>> over the context entries.
> >>>>
> >>>> Specifically not. State loading (in the longer run) would better not fail
> >>>> once started. (Imo it should have been this way from the beginning.) Only
> >>>> then will the vCPU still be in a predictable state even after a possible
> >>>> error.
> >>>
> >>> Looking at the callers, does such predictable state after failure
> >>> matter?
> >>>
> >>> One caller is an hypercall used by the toolstack at domain create,
> >>> failing can just lead to the domain being destroyed.  The other caller
> >>> is vm fork, which will also lead to the fork being destroyed if
> >>> context loading fails.
> >>>
> >>> Maybe I'm overlooking something.
> >>
> >> You don't (I think), but existing callers necessarily have to behave the
> >> way you describe. From an abstract perspective, though, failed state
> >> loading would better allow a retry. And really I thought that when you
> >> suggested to split checking from loading, you had exactly that in mind.
> > 
> > Not really TBH, because I didn't think that much on a possible
> > implementation when proposing it.
> 
> But what else did you think of then in terms of separating checking from
> loading?

Just calling the check and load functions inside of the same loop was
my initial thought.

> > Maybe a suitable compromise would be to reset the state to the initial
> > (at domain build) one on failure?
> 
> That's an option, sure.
> 
> > I do dislike the duplicated loops, as it seems like a lot of duplicate
> > boilerplate code, and I have fears of it going out of sync.
> 
> There's a certain risk, yes, but that exists similarly with the save and
> load sides imo.

Hm, yes, albeit I have the feeling those are not as similar as the
proposed check and load loops.

Thanks, Roger.