Re: [PATCH][4.17?] x86: also zap secondary time area handles during soft reset

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 13, 2022 at 08:48:21AM +0200, Jan Beulich wrote:
> Just like domain_soft_reset() properly zaps runstate area handles, the
> secondary time area ones also need discarding to prevent guest memory
> corruption once the guest is re-started.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> To avoid another for_each_vcpu() here, domain_soft_reset() could also
> be made call a new arch_vcpu_soft_reset() out of its already present
> loop. Yet that would make the change less isolated.
> 
> In domain_soft_reset() I wonder whether, just like done here, the
> zapping of runstate area handles and vCPU info mappings wouldn't better
> be done after all operations which can fail. But perhaps for this to
> matter the domain is left in too inconsistent a state anyway if the
> function fails ...

We would need some kind of recovery anyway, so given the current code
and lack of recovery it doesn't seem to matter much.

> However, at the very least I wonder whether x86'es
> restriction to HVM shouldn't leave PV guests undisturbed if a soft-reset
> was attempted on them. Right now they not only have state partially
> clobbered, but (if the arch function is reached) they would be crashed
> unconditionally.

It's a toolstack initiated operation by a domctl, so I'm fine with
saying that it's up for the toolstack to prevent soft resets from
being attempted against PV domains.  Would be nice to reject the
operation earlier on the hypervisor, maybe by moving
arch_domain_soft_reset() earlier in domain_soft_reset() so that we
can return without crashing?

In any case it's unlikely for a domain that was attempting a soft
reset to survive the hypervisor rejecting the operation, so it doesn't
matter much whether the domain is crashed by Xen or left as-is I would
think.

Thanks, Roger.