Re: [RFC PATCH] x86/p2m-pt: do type recalculations with p2m read lock

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.04.2023 16:27, Roger Pau Monné wrote:
> On Mon, Apr 03, 2023 at 02:39:08PM +0200, Jan Beulich wrote:
>> On 03.04.2023 12:14, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>> @@ -486,9 +486,6 @@ static int cf_check do_recalc(struct p2m_domain *p2m, 
>>> unsigned long gfn)
>>>          p2m_type_t ot, nt;
>>>          unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
>>>  
>>> -        if ( !valid_recalc(l1, e) )
>>> -            P2M_DEBUG("bogus recalc leaf at d%d:%lx:%u\n",
>>> -                      p2m->domain->domain_id, gfn, level);
>>>          ot = p2m_flags_to_type(l1e_get_flags(e));
>>>          nt = p2m_recalc_type_range(true, ot, p2m, gfn & mask, gfn | ~mask);
>>>          if ( nt != ot )
>>
>> I'm afraid I neither understand why you make this change, nor why you
>> then leave the other use of valid_recalc() in place.
> 
> The message can be bogus if we allow concurrent do_recalc(), and I
> did miss the previous one.
> 
> I missed the one at the top.  Originally I wanted to send the RFC with
> just changing the lock to read mode, but then I though I might as
> well fix that (now bogus) print message.
> 
>>> @@ -538,9 +535,9 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>>       */
>>>      ASSERT(!altp2m_active(current->domain));
>>>  
>>> -    p2m_lock(p2m);
>>> +    p2m_read_lock(p2m);
>>>      rc = do_recalc(p2m, PFN_DOWN(gpa));
>>> -    p2m_unlock(p2m);
>>> +    p2m_read_unlock(p2m);
>>>  
>>>      return rc;
>>>  }
>>
>> How can this be safe, when do_recalc() involves p2m_next_level(), which
>> may install new (intermediate) page tables?
> 
> Oh, great, didn't realize it was capable of doing so, it's more hidden
> than in the EPT case.  Seems like this will only happen if a superpage
> needs to be split because a lower order frame is being used as an
> ioreq server page.
> 
> Do you think it would be safe to try to attempt to perform the recalc
> with the read lock only and fallback to the write lock if there's a
> need to call p2m_next_level()?

Yes, that ought to be okay.

> Do you agree it might be possible to do the recalc with just the read
> lock if it's updating of PTE type / recalc flags only?

Technically this looks to be possible, yes. Question is whether we do
ourselves much good by introducing such a special case of permitting
a certain kind of writes with the lock only held in read mode. The
latest when we find a second (more or less similar) use case thing
are likely to become difficult.

Jan