Re: [Xen-devel] [PATCH v7] x86/emulate: Send vm_event from emulate

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On 27.08.2019 11:26, Jan Beulich wrote:
> On 20.08.2019 22:11, Andrew Cooper wrote:
>> On 30/07/2019 15:54, Jan Beulich wrote:
>>>> @@ -622,14 +622,22 @@ static void *hvmemul_map_linear_addr(
>>>>                 }
>>>>
>>>>                 if ( p2mt == p2m_ioreq_server )
>>>> -            {
>>>> -                err = NULL;
>>>>                     goto out;
>>>> -            }
>>>>
>>>>                 ASSERT(p2mt == p2m_ram_logdirty || 
>>>> !p2m_is_readonly(p2mt));
>>>> +
>>>> +            if ( curr->arch.vm_event &&
>>>> +                 curr->arch.vm_event->send_event &&
>>>> +                 hvm_emulate_send_vm_event(addr, gfn, pfec) )
>>>> +                err = ERR_PTR(~X86EMUL_RETRY);
>>>>             }
>>>>         }
>>>> +    /* Check if any vm_event was sent */
>>>> +    if ( err )
>>>> +        goto out;
>>>>
>>>>         /* Entire access within a single frame? */
>>>>         if ( nr_frames == 1 )
>>> First of all I have to apologize: In earlier replies I referred
>>> to update_map_err(). I notice only now that this is a still
>>> pending change of mine, which Andrew continues to object to,
>>> while I continue to think it (in one form or another) is needed:
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01250.html 
>>>
>>>
>>> Given the unpatched code, I think your change is correct, but
>>> quite possibly your earlier variant was, too. But since the
>>> unpatched code is imo wrong, I'd prefer if the VM event side
>>> change was put on top of the fixed code, in order to not further
>>> complicate the actual fix (which we may also want to backport).
>>>
>>> Andrew, as to that old pending patch, I'm afraid I haven't been
>>> convinced in the slightest by your argumentation, regardless of
>>> the actual behavior of the XTF test you've created.
>>
>> So what?  You want your change taken anyway despite evidence that it is
>> wrong?
>>
>>>   There are
>>> two fundamental points you've not addressed during the earlier
>>> discussion:
>>> 1) For a guest behavior should be entirely transparent as far as
>>> 2nd level translation goes, unless the _only_ issue results from
>>> it. That's because on bare hardware there simply is no 2nd level
>>> translation.
>>> 2) Somewhat related, consider the case of the guest handling the
>>> #PF on the second half of the access by a means which makes the
>>> reason for the 2nd stage "fault" go away, or not recur. In that
>>> case we've wrongly (i.e. at least needlessly) dealt with the 2nd
>>> stage "fault".
>>
>> For both of these, do you actually have an example where you believe
>> Xen's logic currently goes wrong?  All I see, looking though the
>> threads, is unsubstantiated claims that the current behaviour is wrong.
> 
> Hmm, I thought we're both still recalling the case this started from:
> ballooned-out page handling kicking in when the guest expects a page
> fault (based on its own page tables).
> 
>>> I am, btw, not convinced that the behavior as you've observed it
>>> is actually "correct" in the sense of "sensible".
>>
>> You're entitled to the believe that this isn't sensible if you wish.
>>
>> It doesn't make it relevant to the argument.  Relevant arguments would
>> be identifying, a bug in my XTF test, or counterexample where the CPUs
>> do an opposite thing, or a passage in a spec which make a statement
>> supporting your claim.
>>
>> As far as I am concerned, it is perfectly sensible and logical
>> behaviour.  To complete an instruction which straddles a page boundary,
>> it is necessary to have both translations available in the TLB, which
>> requires two EPT-walks to have already completed correctly.
>>
>> SDM Vol 3 28.2.3.3 is very clear on the matter.  All translations to the
>> ultimate physical addresses get established first (I.e. the TLB fills
>> complete) before any access rights get considered.  This means that
>> ordering of #PF and EPT misconfig/violation is complicated by their dual
>> nature for failures.
>>
>> In reality, I think the current code in Xen will get the priority of
>> second and first stage access right fault inverted, but its a damn sight
>> closer to how the CPU behaves than the proposed patch, which would get
>> first staged access rights mixed up with second stage translation faults.
> 
> I consider your position as perfectly valid to take. It's just that, as
> in so many other cases, it's not the only valid one (imo). You judge
> from observed behavior, which is fine. You don't, however, address my
> argument of there not being 2nd stage translation at all from guest
> pov: The change made results in the expected behavior if there was no
> 2nd stage translation. And it is my view of virtualization that the
> goal should be to provide guest visible behavior matching the
> unvirtualized case as much as possible.
> 

Hi Jan, Andrew,

Is there a way we can go on with this issue?

Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel