[Xen-devel] 答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[David Wang <DavidWang@xxxxxxxxxxx>]

Hi Jan,
Thank you for reply. Maybe My description is not clear. Please allow me to 
explain it again.
Multiple CPU may be share an channel(according to  hpet_get_channel(), it is 
possible ). When  one of  them get the lock of 
channel(hpet_broadcast_exit()->hpet_detach_channel()->spin_lock_irq(&ch->lock)) 
, others shouldn't rewrite/clear the ch->cpumask in hpet_broadcast_exit(). This 
lead to errors.
For example:
CPU zero and CPU one share an channel by executing hpet_get_channel() 
respectively and  ch->cpumask of channel be set to 0x3.
Next, CPU zero  execute hpet_broadcast_exit()->cpumask_clear_cpu() and the 
ch->cpumask is 0x2.  
CPU zero execute hpet_broadcast_exit()->hpet_detach_channel()->cpumask_empty() 
and it get a false.  
After that the next moment, CPU one  execute 
hpet_broadcast_exit()->cpumask_clear_cpu(). That set the ch->cpumask to 0.
When CPU zero execute hpet_detach_channel()->cpumask_first(), ch->cpu would be 
set to nr_cpu_ids for ch->cpumask being 0.
An assertion would happen through hpet_detach_channel() 
->set_channel_irq_affinity() -> cpumask_of() ->cpumask_check();
 
I think the cause leading to assertion is that  cpu rewrite  shared zone when 
other is reading. I've tried two ways.  The CPU must get the lock of channel 
before  executing cpumask_clear_cpu() as patch v1.  Another way of resolving it 
is "a variable hold the value of  ch->cpumask at the beginning of 
hpet_detach_channel() as patch v2" .

"how about eliminating the cpumask_empty() call in favor of just the 
cpumask_first()"  
Do you mean to delete  the cpumask_empty() and leave the cpumask_first()? 

I don't know if  i express clearly.   Best wish and look forward to your reply !

.
发件人: Jan Beulich <JBeulich@xxxxxxxx>
发送时间: 2018年4月18日 23:49
收件人: David Wang
抄送: xen-devel; Fiona Li(BJ-RD)
主题: Re: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)
    
>>> On 18.04.18 at 11:25, <Davidwang@xxxxxxxxxxx> wrote:
> From: David Wang <davidwang@xxxxxxxxxxx>
> 
> For the ch->cpumask be cleared by other cpu, cpumask_first() called by
> hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
> set_channel_irq_affinity() when cpumask_of() check cpu.
> Fix this by using a local variable.

The fix isn't to use a local variable, introducing a local variable is only a
vehicle for addressing the bug. Also I'm afraid I still can't make much
sense of the first sentence; it only is that now I know what you want
to fix.

> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -509,15 +509,18 @@ static void hpet_attach_channel(unsigned int cpu,
>  static void hpet_detach_channel(unsigned int cpu,
>                                  struct hpet_event_channel *ch)
>  {
> +    cpumask_t cpumask;

No, certainly not. We don't want variables of that type on the stack.
Recall that in v1 review I wrote "how about eliminating the
cpumask_empty() call in favor of just the cpumask_first()". The local
variable to introduce is to hold the result of cpumask_first().

Jan


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