Re: [Xen-devel] [PATCH v2 18/25] arm/altp2m: Add HVMOP_altp2m_set_mem_access.

[Julien Grall <julien.grall@xxxxxxx>]

On 06/08/2016 12:03, Sergej Proskurin wrote:
> Hi Julien,
Hello Sergej,

> On 08/04/2016 04:19 PM, Julien Grall wrote:
> > Hello Sergej,
> >
> > On 01/08/16 18:10, Sergej Proskurin wrote:
> > >  int p2m_alloc_table(struct p2m_domain *p2m)
> > >  {
> > >      unsigned int i;
> > > @@ -1920,7 +1948,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t
> > > gfn, uint32_t nr,
> > >                          uint32_t start, uint32_t mask,
> > > xenmem_access_t access,
> > >                          unsigned int altp2m_idx)
> > >  {
> > > -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > > +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *ap2m = NULL;
> > >      p2m_access_t a;
> > >      long rc = 0;
> > >
> > > @@ -1939,33 +1967,60 @@ long p2m_set_mem_access(struct domain *d,
> > > gfn_t gfn, uint32_t nr,
> > >  #undef ACCESS
> > >      };
> > >
> > > +    /* altp2m view 0 is treated as the hostp2m */
> > > +    if ( altp2m_idx )
> > > +    {
> > > +        if ( altp2m_idx >= MAX_ALTP2M ||
> > > +             d->arch.altp2m_vttbr[altp2m_idx] == INVALID_VTTBR )
> > > +            return -EINVAL;
> > > +
> > > +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> > > +    }
> > > +
> > >      switch ( access )
> > >      {
> > >      case 0 ... ARRAY_SIZE(memaccess) - 1:
> > >          a = memaccess[access];
> > >          break;
> > >      case XENMEM_access_default:
> > > -        a = p2m->default_access;
> > > +        a = hp2m->default_access;
> > Why the default_access is set the host p2m and not the alt p2m?
> Currently, we don't have a way of manually setting/getting the
> default_access in altp2m. Maybe it would make sense to extend the
> interface by explicitly setting default_access of the individual views.
> As I am thinking about that, it would benefit the entire architecture as
> the current propagate change operation simply flushes the altp2m views
> and expects them to be lazily filled with hostp2m's entries. Because of
> this, I believe this would render the altp2m functionality obsolete if
> the system would try to change entries in the hostp2m while altp2m was
> active. What do you think?
Sounds good. However, this needs to be documented in the code.

[...]

> > > +int altp2m_set_mem_access(struct domain *d,
> > > +                          struct p2m_domain *hp2m,
> > > +                          struct p2m_domain *ap2m,
> > > +                          p2m_access_t a,
> > > +                          gfn_t gfn);
> > > +
> > >  #endif /* __ASM_ARM_ALTP2M_H */
> > > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > > index 32326cb..9859ad1 100644
> > > --- a/xen/include/asm-arm/p2m.h
> > > +++ b/xen/include/asm-arm/p2m.h
> > > @@ -180,6 +180,17 @@ void p2m_dump_info(struct domain *d);
> > >  /* Look up the MFN corresponding to a domain's GFN. */
> > >  mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
> > >
> > > +/* Lookup the MFN, memory attributes, and page table level
> > > corresponding to a
> > > + * domain's GFN. */
> > > +mfn_t p2m_lookup_attr(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t,
> > > +                      unsigned int *level, unsigned int *mattr,
> > > +                      xenmem_access_t *xma);
> > I don't want to see a such interface expose outside of p2m. The
> > outside world may not know what means the level. And I don't
> > understand why you return "mattr" here.
> In the current implementation, mattr is indeed not needed anymore. Yet,
> I did want to hear your opinion first. So, I will gladly remove mattr
> from the prototype.
>
> Concerning the exposure of p2m_lookup_attr: Agreed. However, I am not
> sure how else we could get the required functionality from altp2m.c
> without duplicating big parts of the code. In the previous patch, you
> have mentioned that we should rather share code to get the required
> values. Now we do...
>
> Do you have another idea how we could solve this issue?
Please give a look to the functions p2m_get_entry and p2m_set_entry I 
introduced in [1].
Regards,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg02952.html
--
Julien Grall

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