Re: [Xen-devel] [PATCH RFC v1] x86/emulate: Send vm_event form emulate

[Roger Pau Monné <royger@xxxxxxxxxxx>]

On Thu, Jan 10, 2019 at 10:41 AM Alexandru Stefan ISAILA
<aisaila@xxxxxxxxxxxxxxx> wrote:
>
>
> >> +    if ( altp2m_active(current->domain) )
> >> +        p2m = p2m_get_altp2m(current);
> >> +    if ( !p2m )
> >> +        p2m = p2m_get_hostp2m(current->domain);
> >> +
> >> +    gfn_lock(p2m, gfn, 0);
> >> +    mfn = p2m->get_entry(p2m, gfn, &p2mt, &access, 0, NULL, NULL);
> >> +    gfn_unlock(p2m, gfn, 0);
> >
> > Don't you need to keep the gfn locked for at lest the duration of the
> > function? Or else what you put in the req struct below might not be
> > accurate by the time you write it. Maybe this is not relevant because
> > this req ends up queued in an async ring, so by the time the other end
> > reads the request the information might indeed have changed already.
>
> I don't think the gfn should be locked for that long. I followed the
> model from p2m_mem_access_check() and there the gfn is locked in
> p2m_get_mem_access() only to get the access. This is relevant because
> the event is synced.

What do you mean with the event is synced?

AFAICT what you do is correct. Keeping the gfn locked for longer is
not going to prevent the information on the ring from getting stale
anyway.

> >
> > Also, I'm wondering whether there's no helper to fetch the gfn entry
> > information from the p2m when using altp2m. The above construct (or
> > variations of it) must be widely used in altp2m code?
>
> I can change to p2m_get_mem_access() in the next version and lose some
> duplicated code here.

That seems sensible.

> >
> >> +
> >> +    if ( mfn_eq(mfn, INVALID_MFN) )
> >> +        return false;
> >> +
> >> +    switch ( access ) {
> >> +    case p2m_access_x:
> >> +    case p2m_access_rx:
> >> +        if ( pfec & PFEC_write_access )
> >> +            req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
> >> +        break;
> >
> > Newline.
> >
> >> +    case p2m_access_w:
> >> +    case p2m_access_rw:
> >> +        if ( pfec & PFEC_insn_fetch )
> >> +            req.u.mem_access.flags = MEM_ACCESS_X;
> >> +        break;
> >
> > Newline.
> >
> >> +    case p2m_access_r:
> >> +    case p2m_access_n:
> >> +        if ( pfec & PFEC_write_access )
> >> +            req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
> >> +        if ( pfec & PFEC_insn_fetch )
> >> +            req.u.mem_access.flags |= MEM_ACCESS_X;
> >> +        break;
> >
> > Newline.
> >
> >> +    default:
> >> +        return false;
> >> +    }
> >
> > I'm not sure the switch is needed, you can't have a PFEC_write_access
> > for example if the p2m type is p2m_access_w or p2m_access_rw, hence it
> > seems like it could be simplified to only take the pfec into account?
>
> It is possible to have PFEC_write_access and p2m_access_w but then there
> is no violation and the event will not be sent.

I'm not sure I follow. PFEC is a way to signal a page fault, hence it
would make no sense to for example get PFEC_write_access if the access
is p2m_access_w?

Thanks, Roger.

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