Re: [Xen-devel] [PATCH 2/2] altp2m: Implement p2m_get_mem_access for altp2m views

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Jan 28, 2016 6:38 AM, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>
> >>> On 27.01.16 at 21:06, <tlengyel@xxxxxxxxxxx> 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 = ""> > >
> > -Â Â Â Â 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.

>
> > @@ -1918,9 +1920,10 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> >Â Â* Get access type for a gfn.
> >Â Â* If gfn == INVALID_GFN, gets the default access type.
> >Â Â*/
> > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> > +Â Â Â Â Â Â Â Â Â Â Â Âxenmem_access_t *access)
> >Â {
> > -Â Â struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +Â Â struct p2m_domain *hp2m = p2m_get_hostp2m(d), *p2m = NULL;
> >Â Â Â p2m_type_t t;
> >Â Â Â p2m_access_t a;
> >Â Â Â mfn_t mfn;
> > @@ -1943,10 +1946,22 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >Â Â Â /* If request to get default access. */
> >Â Â Â if ( gfn_x(gfn) == INVALID_GFN )
> >Â Â Â {
> > -Â Â Â Â *access = memaccess[p2m->default_access];
> > +Â Â Â Â *access = memaccess[hp2m->default_access];
> >Â Â Â Â Â return 0;
> >Â Â Â }
>
> And following the above - why would this not use the altp2m's
> default access?

Altp2m default access is currently ignored. You can only set default access for hp2m.

>
> > +Â Â /* 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.

>
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
> >Â Â* Get access type for a gfn.
> >Â Â* If gfn == INVALID_GFN, gets the default access type.
> >Â Â*/
> > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned long altp2m_idx,
> > +Â Â Â Â Â Â Â Â Â Â Â Âxenmem_access_t *access);
>
> Same question as for patch 1 regarding the "unsigned long" here.
>
As stated on the other patch, it could be either for our purposes. Any benefit in using int rather then long?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel