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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> ---
>>>>>> 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