Re: [PATCH] x86/HVM: tidy state on hvmemul_map_linear_addr()'s error path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 17:11, Paul Durrant wrote:
> On 08/02/2024 15:59, Jan Beulich wrote:
>> On 06.02.2024 13:06, Jan Beulich wrote:
>>> While in the vast majority of cases failure of the function will not
>>> be followed by re-invocation with the same emulation context, a few
>>> very specific insns - involving multiple independent writes, e.g. ENTER
>>> and PUSHA - exist where this can happen. Since failure of the function
>>> only signals to the caller that it ought to try an MMIO write instead,
>>> such failure also cannot be assumed to result in wholesale failure of
>>> emulation of the current insn. Instead we have to maintain internal
>>> state such that another invocation of the function with the same
>>> emulation context remains possible. To achieve that we need to reset MFN
>>> slots after putting page references on the error path.
>>>
>>> Note that all of this affects debugging code only, in causing an
>>> assertion to trigger (higher up in the function). There's otherwise no
>>> misbehavior - such a "leftover" slot would simply be overwritten by new
>>> contents in a release build.
>>>
>>> Also extend the related unmap() assertion, to further check for MFN 0.
>>>
>>> Fixes: 8cbd4fb0b7ea ("x86/hvm: implement hvmemul_write() using real 
>>> mappings")
>>> Reported.by: Manuel Andreas <manuel.andreas@xxxxxx>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Just noticed that I forgot to Cc Paul.
>>
>> Jan
>>
>>> ---
>>> While probably I could be convinced to omit the #ifndef, I'm really
>>> considering to extend the one in hvmemul_unmap_linear_addr(), to
>>> eliminate the zapping from release builds: Leaving MFN 0 in place is not
>>> much better than leaving a (presently) guest-owned one there. And we
>>> can't really put/leave INVALID_MFN there, as that would conflict with
>>> other debug checking.
> 
> Would it be worth defining a sentinel value for this purpose rather than 
> hardcoding _mfn(0)? (_mfn(0) seems like a reasonable sentinel... it's 
> just a question of having a #define for it).

Perhaps, but that's for a separate patch then.

> Either way...
> 
> Acked-by: Paul Durrant <paul@xxxxxxx>

Thanks.

Jan