Re: [PATCH v5 10/10] x86emul: support MCOMMIT

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/04/2020 16:09, Jan Beulich wrote:
> On 03.04.2020 17:00, Andrew Cooper wrote:
>> On 03/04/2020 09:00, Jan Beulich wrote:
>>> On 03.04.2020 01:47, Andrew Cooper wrote:
>>>> On 24/03/2020 12:37, Jan Beulich wrote:
>>>>> The dependency on a new EFER bit implies that we need to set that bit
>>>>> ourselves in order to be able to successfully invoke the insn.
>>>>>
>>>>> Also once again introduce the SVM related constants at this occasion.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> RFC: The exact meaning of the PM stating "any errors encountered by
>>>>>      those stores have been signaled to associated error logging
>>>>>      resources" is unclear. Depending on what this entails, blindly
>>>>>      enabling EFER.MCOMMIT may not be a good idea. Hence the RFC.
>>>> Not just that.  Its not safe for Xen to ever execute MCOMMIT for
>>>> emulation purposes.
>>> I.e. you're suggesting we mustn't even try to emulate it?
>> Sorry - that's not quite what I intended to mean.
>>
>>>> From what I can glean from the documentation, it is intended for
>>>> non-volatile RAM, but I can't find anything discussing the error handling.
>>>>
>>>> The fact the instruction can be intercepted in the first place hopefully
>>>> means that there must be something Xen can look at to get the real error
>>>> indicator.  However, the suggestion is that this will all be platform
>>>> specific.
>>>>
>>>>
>>>> The emulation problem comes from the fact that if Xen has any pending
>>>> writes to to NVRAM as part of the emulation path (or an interrupt for
>>>> that matter), an error intended for Xen would leak into guest context.
>>> I'm afraid all of this is guesswork until it becomes clear how
>>> exactly this error reporting is intended to work.
>> What I meant was that "emulating MCOMMIT can't involve executing an
>> MCOMMIT instruction".
> I still don't see why - the error recording is (presumably) not
> dependent upon the context in which the insn was issued.

And that is the problem.  This instruction has a 1-bit "everything ok"
vs "something went wrong" feedback.

We can't be telling a guest that something went wrong when in fact it
was Xen doing something unrelated which suffered the error.

>> In some future where we have combined intercept and emulation paths,
>> whatever ends up existing will still have to reach out to the error
>> banks directly to figure out what is going on.
> I.e. you're assuming there's going to be an architectural way to
> access those, rather than perhaps many platform specific ones?

I think we're going to have to wait and see what materialises.

~Andrew