Re: [PATCH][4.17] x86/shadow: drop (replace) bogus assertions

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 14/10/2022 09:49, Jan Beulich wrote:
> The addition of a call to shadow_blow_tables() from shadow_teardown()
> has resulted in the "no vcpus" related assertion becoming triggerable:
> If domain_create() fails with at least one page successfully allocated
> in the course of shadow_enable(), or if domain_create() succeeds and
> the domain is then killed without ever invoking XEN_DOMCTL_max_vcpus.

It warrants pointing out that are unit tests in the tree which do
exactly this.

> The assertion's comment was bogus anyway: Shadow mode has been getting
> enabled before allocation of vCPU-s for quite some time.

I agree with the principle of what you're saying, but I don't think it's
accurate.

Shadow (vs hap) has always been part of domain create.  But we've always
had a problem where we need to wait for a shadow op to allocate some
shadow memory before various domain-centric operations can proceed.

As with the ARM GICv2 issues, we do want to address this and let
domain_create() (or some continuable version of it) allocate the bare
minimum shadow pool size, which simplifies a load of other codepaths.

>  Convert the
> assertion to a conditional: As long as there are no vCPU-s, there's
> nothing to blow away.
>
> Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool preemptively")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> A similar assertion/comment pair exists in _shadow_prealloc(); the
> comment is similarly bogus, and the assertion could in principle trigger
> e.g. when shadow_alloc_p2m_page() is called early enough. Replace those
> at the same time by a similar early return, here indicating failure to
> the caller (which will generally lead to the domain being crashed in
> shadow_prealloc()).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> While in shadow_blow_tables() the option exists to simply remove the
> assertion without adding a new conditional (the two loops simply will
> do nothing), the same isn't true for _shadow_prealloc(): There we
> would then trigger the ASSERT_UNREACHABLE() near the end of the
> function.

IMO, blow_tables() has no business caring about vcpus.  It should be
idempotent, and ideally wants to be left in a state where it doesn't
need modifying when the aformentioned create changes happen.

For prealloc(), I'd argue that it shouldn't care, but as this is a
bugfix to an XSA, leaving it in this form for now is the safer course of
action.  Whomever cleans up the create path can do the work to ensure
that all prealloc() paths are safe before vcpus are allocated.

~Andrew