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é <[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?
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.
