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.
> 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.
Having looked over everything yet again, I stand by my very first
conclusion of that change being incorrect.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
