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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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