Re: [Xen-devel] [PATCH 6/6] x86/hvm: Implement hvmemul_write() using real mappings

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.07.17 at 19:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/07/17 17:06, Jan Beulich wrote:
>>>>> On 03.07.17 at 17:07, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 22/06/17 10:06, Jan Beulich wrote:
>>>>> +        *mfn++ = _mfn(page_to_mfn(page));
>>>>> +        frame++;
>>>>> +
>>>>> +        if ( p2m_is_discard_write(p2mt) )
>>>>> +        {
>>>>> +            err = ERR_PTR(~(long)X86EMUL_OKAY);
>>>>> +            goto out;
>>>> If one page is discard-write and the other isn't, this will end up
>>>> being wrong.
>>> Straddled accesses are always a grey area, and discard-write is an extra
>>> special case which only exists inside Xen.  Discard-write means that the
>>> guest knows that it shouldn't write there at all.
>> Is it the case that the guest knows? Iirc this type had been
>> introduced for introspection tool use.
> 
> This is for Intel GVT-g (c/s 1c02cce0ed).

Oh, I've mixed this up with the hvmemul_write_discard() & Co,
which ...

> Introspection would go very wrong if selective writes had no effect.

... are introspection specific, so I'm afraid you're not entirely right
with this statement.

>> Plus - not having the parameters const means you can't
>> allocate and initialize something in one go, and then store or
>> pass around a pointer to it which is const-qualified to make
>> clear the contents of that memory block aren't supposed to be
>> further modified (until de-allocation).
> 
> I don't see this as a bad thing.  You can't do it with malloc()/free().

And wrongly so, imo.

>>>>> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
>>>> There upsides and downsides to requiring the caller to pass in the
>>>> same values as to map(): You can do more correctness checking
>>>> here, but you also risk the caller using the wrong values (perhaps
>>>> because of a meanwhile updated local variable). While I don't
>>>> outright object to this approach, personally I'd prefer minimal
>>>> inputs here, and the code deriving everything from hvmemul_ctxt.
>>> I'm not sure exactly how we might wish to extend this logic.  Are we
>>> ever going to want more than one active mapping at once (perhaps rep
>>> movs emulation across two ram regions)?
>> I could imagine even more than two regions - from an abstract
>> perspective it may be possible / helpful to have a mapping of
>> each piece of memory a single instruction accesses, along the
>> lines of minimal number of architecturally guaranteed TLB
>> entries on ia64 in order to execute any possible x86 insn.
> 
> A further problem I've just realised is the use of multiple ->write()
> calls for a single instruction in x86_emulate().  The preceding writes
> obviously can't get backed out in the case of the second write taking a
> fault, which will cause the emulator to exhibit similar
> non-architectural behaviour as this patch is trying to fix.

I'm not convinced hardware guarantees all cases where we use
multiple writes to be carried out in one step. I'm relatively sure
multiple pushes (for far calls) are being carried out one by one.
Just that exception checks likely are being done once up front
(albeit I'm less certain about #PF, whereas for all segment
related things this would seem natural). Same for SGDT/SIDT,
which I think are the only non-push multi-write insns.

And of course the same issue exists for multiple segment register
writes.

>>> The other reason is that in the release builds, even if we stored the
>>> pointer in hvmemul_ctxt, we still cant determine which unmapping
>>> function to use without linear and size.
>> I don't understand - all you pass on is "mapping". And whether to
>> unmap a single or two pages could be told from hvmemul_ctxt->mfn[1]
>> (not) being INVALID_MFN.
> 
> That isn't safe outside of the debug builds.
> 
> You could in principle use mfn[1] != 0 in release builds, if it weren't
> for the fact that hvmemul_ctxt could be used for multiple ->write()
> calls.  In the case of a straddled write followed by a non-straddled
> write, the second unmap() would evaluate incorrectly.

I'd of course imply unmap() to set ->mfn[] to INVALID_MFN.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel