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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.10.2022 12:02, Andrew Cooper wrote:
> 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.

Can do.

>> 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.

I'm afraid I can't derive from ...

> 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.

... this why the statement isn't accurate. What both functions are trying
to do is reclaim pages from in-use shadows. None can exist without vCPU-s.
Yet still shadow mode has been getting enabled before vCPU-s are allocated.

>> ---
>> 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.

First: Both the change as done by the patch as well as the alternative
pointed out fulfill this requirement, afaict at least. Hence what you
say doesn't make clear whether you agree with the change as done or
whether you'd prefer the alternative (and if so, why).

Then the two functions do about the same thing, just with prealloc
having an early exit condition (once having reached the intended
count of available pages). Therefore ...

> 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.

... I think the two functions want to remain as closely in sync as
possible (I'm afraid I didn't fully have this in mind when writing
the remark - it should have been worded a little differently); really
I think it would be best if the duplicate code was folded. Hence to
me leaving alone that function (which is what I understand you
suggest) is not a good option, yet as explained in the post-commit-
message remark not replacing the assertion by an if() would have an
unwanted consequence.

Jan