Re: [PATCH v2 2/5] x86/HVM: split restore state checking from state loading

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2023 23:08, Andrew Cooper wrote:
> On 16/11/2023 1:46 pm, 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[]).
> 
> Pointless copying is best avoided, but it would mean that we either need
> to enforce proper alignment within the buffer (hard, but at least it's
> page aligned to start with), or __pack all of the structures so they get
> an alignment of 1.

Ugly, when they're part of the public interface.

> Not that I expect things to break in practice, but UB is UB and in some
> copious free time it might be nice to re-activate the unaligned checking
> in UBSAN on x86.

The C99 standard only ever mentions "alignment appropriate for its type".
I didn't even find any explicit mentioning of UB there. My understanding
is that it's all down to the psABI. That, in turn, allows for unaligned
accesses: "Misaligned data accesses are slower than aligned accesses but
otherwise behave identically. The only exceptions are ..."

>> --- a/xen/arch/x86/hvm/save.c
>> +++ b/xen/arch/x86/hvm/save.c
>> @@ -291,9 +369,8 @@ int hvm_load(struct domain *d, hvm_domai
>>      if ( !hdr )
>>          return -ENODATA;
>>  
>> -    rc = arch_hvm_load(d, hdr);
>> -    if ( rc )
>> -        return rc;
>> +    ASSERT(!arch_hvm_check(d, hdr));
> 
> You're normally the proponent of not having side effects in ASSERT()s
> like this.

The function could be marked pure, if it didn't log a message. I don't
consider this logging a true side effect here. And I truly want the
function call eliminated in release builds (i.e. I wouldn't want this
to become "if() ASSERT_UNREACHABLE();").

> But our caller did this anyway, so why re-assert it here?

One of the callers did, the other (hvm_copy_context_and_params()) didn't
but still ought to meet the assumption.

Jan