Re: [PATCH V2 3/3] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 10/26/18 5:47 PM, Jan Beulich wrote:
>>>> On 23.10.18 at 15:19, <rcojocaru@bitdefender.com> wrote:
>>  xen/arch/x86/mm/p2m-ept.c |  8 +++++
>>  xen/arch/x86/mm/p2m.c     | 83 +++++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 78 insertions(+), 13 deletions(-)
> 
> What about p2m-pt.c?

Thank you for the review!

I thought altp2m was an EPT-only tool, do you mean that I should also
iterate over possibly active altp2ms in p2m_pt_handle_deferred_changes()
there as well?

>> @@ -287,24 +286,47 @@ int p2m_is_logdirty_range(struct p2m_domain *p2m, unsigned long start,
>>      return 0;
>>  }
>>  
>> +static void _p2m_change_entry_type_global(struct p2m_domain *p2m,
> 
> Instead of prefixing another underscore, why don't you
> drop p2m_ from this static helper?

Of course, I'll do that.

>> +                                          p2m_type_t ot, p2m_type_t nt)
>> +{
>> +    p2m->change_entry_type_global(p2m, ot, nt);
>> +    p2m->global_logdirty = (nt == p2m_ram_logdirty);
>> +}
>> +
>>  void p2m_change_entry_type_global(struct domain *d,
>>                                    p2m_type_t ot, p2m_type_t nt)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>  
>>      ASSERT(ot != nt);
>>      ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
>>  
>> -    p2m_lock(p2m);
>> -    p2m->change_entry_type_global(p2m, ot, nt);
>> -    p2m->global_logdirty = (nt == p2m_ram_logdirty);
>> -    p2m_unlock(p2m);
>> +    p2m_lock(hostp2m);
>> +
>> +    _p2m_change_entry_type_global(p2m_get_hostp2m(d), ot, nt);
> 
> Use hostp2m here too?

Right, I should have. I'll change it.

>> +
>> +#ifdef CONFIG_HVM
>> +    if ( unlikely(altp2m_active(d)) )
>> +    {
>> +        unsigned int i;
>> +
>> +        for ( i = 0; i < MAX_ALTP2M; i++ )
>> +            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>> +            {
>> +                struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>> +
>> +                p2m_lock(p2m);
>> +                _p2m_change_entry_type_global(p2m, ot, nt);
>> +                p2m_unlock(p2m);
>> +            }
>> +    }
>> +#endif
>> +
>> +    p2m_unlock(hostp2m);
>>  }
>>  
>> -void p2m_memory_type_changed(struct domain *d)
>> +static void _p2m_memory_type_changed(struct p2m_domain *p2m)
> 
> Same as above.

Will change it.

>> @@ -313,6 +335,22 @@ void p2m_memory_type_changed(struct domain *d)
>>      }
>>  }
>>  
>> +void p2m_memory_type_changed(struct domain *d)
>> +{
>> +#ifdef CONFIG_HVM
>> +    if ( unlikely(altp2m_active(d)) )
>> +    {
>> +        unsigned int i;
>> +
>> +        for ( i = 0; i < MAX_ALTP2M; i++ )
>> +            if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>> +                _p2m_memory_type_changed(d->arch.altp2m_p2m[i]);
>> +    }
>> +#endif
>> +
>> +    _p2m_memory_type_changed(p2m_get_hostp2m(d));
>> +}
> 
> Is there a particular reason this one doesn't follow the host-then-
> alt pattern used above and elsewhere? If so, a comment should
> be attached. If not, using that same model would seem better.
> (Same then below for the range function.)

No reason, no. I'll make them consistent.


Thanks,
Razvan

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