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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


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?

>>          break;
>>      default:
>>          return -EINVAL;
>>      }
>>
>
> [...]
>
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index a6496b7..dc41f93 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -71,4 +71,14 @@ void altp2m_flush(struct domain *d);
>>  int altp2m_destroy_by_id(struct domain *d,
>>                           unsigned int idx);
>>
>> +/* Set memory access attributes of the gfn in the altp2m view. If
>> the altp2m
>> + * view does not contain the particular entry, copy it first from
>> the hostp2m.
>> + *
>> + * Currently supports memory attribute adoptions of only one (4K)
>> page. */
>
> Coding style:
>
> /*
>  *  Foo
>  *  Bar
>  */
>

Thank you.

>> +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?

>> +
>> +/* Modify an altp2m view's entry or its attributes. */
>> +int modify_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>> +                        paddr_t gpa, paddr_t maddr, unsigned int level,
>> +                        p2m_type_t t, p2m_access_t a);
>> +
>>  /* Clean & invalidate caches corresponding to a region of guest
>> address space */
>>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>>
>> @@ -303,6 +314,16 @@ static inline int get_page_and_type(struct
>> page_info *page,
>>  /* get host p2m table */
>>  #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>>
>> +static inline bool_t p2m_is_hostp2m(const struct p2m_domain *p2m)
>> +{
>> +    return p2m->p2m_class == p2m_host;
>> +}
>> +
>> +static inline bool_t p2m_is_altp2m(const struct p2m_domain *p2m)
>> +{
>> +    return p2m->p2m_class == p2m_alternate;
>> +}
>> +
>
> Why those helpers are only added here? They should be at the same
> place you define the p2m_class.
>

They are used here for the first time. But ok, I can do that.

>>  /* vm_event and mem_access are supported on any ARM guest */
>>  static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
>>  {
>>
>

Best regards,
~Sergej


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