On 27.07.2022 21:06, Andrew Cooper wrote:
> On 27/07/2022 13:53, Jan Beulich wrote:
>> On 27.07.2022 14:46, Andrew Cooper wrote:
>>> On 26/07/2022 17:04, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>>> @@ -132,7 +132,8 @@ delete_fl1_shadow_status(struct domain *
>>>> SHADOW_PRINTK("gfn=%"SH_PRI_gfn", type=%08x, smfn=%"PRI_mfn"\n",
>>>> gfn_x(gfn), SH_type_fl1_shadow, mfn_x(smfn));
>>>> ASSERT(mfn_to_page(smfn)->u.sh.head);
>>>> - shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn);
>>>> + if ( !shadow_hash_delete(d, gfn_x(gfn), SH_type_fl1_shadow, smfn) )
>>>> + ASSERT_UNREACHABLE();
>>> I'd recommend crashing the domain here too. I know the we've already
>>> got the return value we want, but this represents corruption in the
>>> shadow pagetable metadata, and I highly doubt it is safe to let the
>>> guest continue to execute in such circumstances.
>> I'm happy to add or convert, but please clarify: DYM in addition to
>> the assertion or in place of it?
>
> Erm pass.
>
> We still don't have a sensible construct for ASSERT_OR_DOMAIN_CRASH(),
> or a well-thought-through piece of guidance.
>
> Probably keep the ASSERT() ? Almost all state checking in the shadow
> code is via asserts.
Meanwhile I was actually leaning towards dropping the assertion. The
goal is to catch attention when things go wrong. A suddenly dying
domain surely ought to be as noticable as an assertion triggering.
> Mostly what I'm concerned by is it feeling weird to have one path which
> only domain crashes, and one path which only asserts.
I'm afraid I've not been successful in determining which other path it
is you're referring to. If I knew, I might be up for converting that
as well (not in this same patch, perhaps). I don't think you mean
set_shadow_status(), since there we can't validly ASSERT(), as the
condition is (in principle) guest controllable.
Jan
