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é <[email protected]> >>>>>> Signed-off-by: Jan Beulich <[email protected]> >>>>>> --- >>>>>> 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? > 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. Andrew - before I go and undo the v2 changes, what is your view? Jan
