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....@citrix.com> >> Signed-off-by: Jan Beulich <jbeul...@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[]). > > 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
