Re: [RFC 14/22] xen/arm: p2m: Re-implement p2m_cache_flush using p2m_get_entry

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 05/09/16 22:13, Stefano Stabellini wrote:
> On Thu, 28 Jul 2016, Julien Grall wrote:
>> The function p2m_cache_flush can be re-implemented using the generic
>> function p2m_get_entry by iterating over the range and using the mapping
>> order given by the callee.
>>
>> As the current implementation, no preemption is implemented, although
>> the comment in the current code claimed it. As the function is called by
>> a DOMCTL with a region of 1GB maximum, I think the preemption can be
>> left unimplemented for now.
>>
>> Finally drop the operation CACHEFLUSH in apply_one_level as nobody is
>> using it anymore. Note that the function could have been dropped in one
>> go at the end, however I find easier to drop the operations one by one
>> avoiding a big deletion in the patch that convert the last operation.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>     The loop pattern will be very for the reliquish function. It might
>>     be possible to extract it in a separate function.
>> ---
>>  xen/arch/arm/p2m.c | 67 +++++++++++++++++++++++++++---------------------------
>>  1 file changed, 34 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 9a9c85c..e7697bb 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -722,7 +722,6 @@ enum p2m_operation {
>>      INSERT,
>>      REMOVE,
>>      RELINQUISH,
>> -    CACHEFLUSH,
>>      MEMACCESS,
>>  };
>>
>> @@ -978,36 +977,6 @@ static int apply_one_level(struct domain *d,
>>           */
>>          return P2M_ONE_PROGRESS;
>>
>> -    case CACHEFLUSH:
>> -        if ( !p2m_valid(orig_pte) )
>> -        {
>> -            *addr = (*addr + level_size) & level_mask;
>> -            return P2M_ONE_PROGRESS_NOP;
>> -        }
>> -
>> -        if ( level < 3 && p2m_table(orig_pte) )
>> -            return P2M_ONE_DESCEND;
>> -
>> -        /*
>> -         * could flush up to the next superpage boundary, but would
>> -         * need to be careful about preemption, so just do one 4K page
>> -         * now and return P2M_ONE_PROGRESS{,_NOP} so that the caller will
>> -         * continue to loop over the rest of the range.
>> -         */
>> -        if ( p2m_is_ram(orig_pte.p2m.type) )
>> -        {
>> -            unsigned long offset = paddr_to_pfn(*addr & ~level_mask);
>> -            flush_page_to_ram(orig_pte.p2m.base + offset);
>> -
>> -            *addr += PAGE_SIZE;
>> -            return P2M_ONE_PROGRESS;
>> -        }
>> -        else
>> -        {
>> -            *addr += PAGE_SIZE;
>> -            return P2M_ONE_PROGRESS_NOP;
>> -        }
>> -
>>      case MEMACCESS:
>>          if ( level < 3 )
>>          {
>> @@ -1555,12 +1524,44 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
>>  {
>>      struct p2m_domain *p2m = &d->arch.p2m;
>>      gfn_t end = gfn_add(start, nr);
>> +    p2m_type_t t;
>> +    unsigned int order;
>>
>>      start = gfn_max(start, p2m->lowest_mapped_gfn);
>>      end = gfn_min(end, p2m->max_mapped_gfn);
>>
>> -    return apply_p2m_changes(d, CACHEFLUSH, start, nr, INVALID_MFN,
>> -                             0, p2m_invalid, d->arch.p2m.default_access);
>> +    /* XXX: Should we use write lock here? */
>
> Good question. As the p2m is left unchanged by this function, I think
> that the read lock is sufficient.

It is what I thought. I will replace the todo by a comment explain why 
read-lock is used here.

>
>
>> +    p2m_read_lock(p2m);
>> +
>> +    for ( ; gfn_x(start) < gfn_x(end); start = gfn_add(start, 1UL << order) )
>> +    {
>> +        mfn_t mfn = p2m_get_entry(p2m, start, &t, NULL, &order);
>> +
>> +        /* Skip hole and non-RAM page */
>> +        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(t) )
>> +        {
>> +            /*
>> +             * the order corresponds to the order of the mapping in the
>> +             * page table. so we need to align the gfn before
>> +             * incrementing.
>> +             */
>> +            start = _gfn(gfn_x(start) & ~((1UL << order) - 1));
>> +            continue;
>> +        }
>> +
>> +        /*
>> +         * Could flush up to the next superpage boundary, but we would
>> +         * need to be careful about preemption, so just do one 4K page
>> +         * now.
>
> I think that even without preemption you should implement flushing up to
> the next superpage boundary (but not beyond "end"). You can still do it
> 4K at a time, but only call p2m_get_entry once per "order". Could be a
> decent performance improvement as cacheflush is a performance critical
> hypercall.

Good point. I will give a look for the next version.

Regards,

-- 
Julien Grall

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