Re: [Xen-devel] [PATCH V4 3/3] x86/altp2m: fix display frozen when switching to a new view early

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/9/18 4:19 PM, Razvan Cojocaru wrote:
> On 11/8/18 8:14 PM, George Dunlap wrote:
>> On 11/01/2018 02:45 PM, Razvan Cojocaru wrote:
>> ...here and...
>>
>>> +
>>>  int p2m_set_ioreq_server(struct domain *d,
>>>                           unsigned int flags,
>>>                           struct hvm_ioreq_server *s)
>>> @@ -994,12 +1033,12 @@ int p2m_change_type_one(struct domain *d, unsigned 
>>> long gfn_l,
>>>  }
>>>  
>>>  /* Modify the p2m type of a range of gfns from ot to nt. */
>>> -void p2m_change_type_range(struct domain *d, 
>>> -                           unsigned long start, unsigned long end,
>>> -                           p2m_type_t ot, p2m_type_t nt)
>>> +static void change_type_range(struct p2m_domain *p2m,
>>> +                              unsigned long start, unsigned long end,
>>> +                              p2m_type_t ot, p2m_type_t nt)
>>>  {
>>>      unsigned long gfn = start;
>>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>> +    struct domain *d = p2m->domain;
>>>      int rc = 0;
>>>  
>>>      ASSERT(ot != nt);
>>> @@ -1052,6 +1091,24 @@ void p2m_change_type_range(struct domain *d,
>>>      p2m_unlock(p2m);
>>>  }
>>>  
>>> +void p2m_change_type_range(struct domain *d,
>>> +                           unsigned long start, unsigned long end,
>>> +                           p2m_type_t ot, p2m_type_t nt)
>>> +{
>>> +    change_type_range(p2m_get_hostp2m(d), start, end, ot, nt);
>>> +
>>> +#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) )
>>> +                change_type_range(d->arch.altp2m_p2m[i], start, end, ot, 
>>> nt);
>>> +    }
>>> +#endif
>>> +}
>>> +
>>
>> ...here you grab & release each lock separately, inside the update
>> function.  memory_type_changed is probably more or less idempotent, so
>> won't matter if two different calls race; but it seems likely that if
>> two p2m_change_type_range() calls happen concurrently, the various
>> altp2ms will get different results.  Is it worth refactoring both of
>> these so that, like change_entry_type_global, you hold the host p2m lock
>> while you change the individual altp2m locks?
> 
> I have changed the code to do all the modifications under hostp2m lock,
> and on changing the resolution on a host with three altp2ms I get a
> "Watchdog timer detects that CPU3 is stuck!" hypervisor crash:

Apologies, this is clearly my fault. I've missed a copy-paste problem
that went wrong - I have a p2m_lock() where there should have been a
p2m_unlock().

I'm sorry for the noise.


Thanks,
Razvan

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