On Jan 28, 2016 7:56 AM, "Jan Beulich" <jbeul...@suse.com> wrote:
>
> >>> On 28.01.16 at 15:42, <tleng...@novetta.com> wrote:
> > On Jan 28, 2016 6:38 AM, "Jan Beulich" <jbeul...@suse.com> wrote:
> >> >>> On 27.01.16 at 21:06, <tleng...@novetta.com> wrote:
> >> > --- a/xen/arch/x86/mm/p2m.c
> >> > +++ b/xen/arch/x86/mm/p2m.c
> >> > @@ -1572,7 +1572,9 @@ void p2m_mem_access_emulate_check(struct vcpu
*v,
> >> > bool_t violation = 1;
> >> > const struct vm_event_mem_access *data = &rsp->u.mem_access;
> >> >
> >> > - if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access)
> > == 0 )
> >> > + if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
> >> > + altp2m_active(v->domain) ?
> > vcpu_altp2m(v).p2midx : 0,
> >> > + &access) == 0 )
> >>
> >> This looks to be a behavioral change beyond what title and
> >> description say, and it's not clear whether that's actually the
> >> behavior everyone wants.
> >
> > I'm fairly comfident its exactly the expected behavior when one uses
> > mem_access in altp2m tables and emulation. Right now because the lack of
> > this AFAIK emulation would not work correctly with altp2m. But Razvan
> > probably can chime in as he uses this path actively.
>
> But this should then be mentioned in the description.
>
> >> > + /* altp2m view 0 is treated as the hostp2m */
> >> > + if ( altp2m_idx )
> >> > + {
> >> > + if ( altp2m_idx >= MAX_ALTP2M ||
> >> > + d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> >> > + return -EINVAL;
> >> > +
> >> > + p2m = d->arch.altp2m_p2m[altp2m_idx];
> >> > + }
> >> > + else
> >> > + p2m = hp2m;
> >>
> >> And I don't see why you need separate p2m and hp2m local
> >> variables.
> >
> > It was also used above for returning default access, while p2m is the
> > actually active one here.
>
> But all that could be done with just the one variable that was already
> there.
>
> Jan
Sure, SGTM.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
