Re: [Xen-devel] [PATCH v1b 1/9] x86/IRQ: deal with move-in-progress state in fixup_irqs()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.05.19 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, May 06, 2019 at 01:15:59AM -0600, Jan Beulich wrote:
>> >>> On 03.05.19 at 16:10, <JBeulich@xxxxxxxx> wrote:
>> >>>> On 03.05.19 at 11:19, <roger.pau@xxxxxxxxxx> wrote:
>> >> On Mon, Apr 29, 2019 at 09:40:14AM -0600, Jan Beulich wrote:
>> >>> --- unstable.orig/xen/arch/x86/irq.c     
>> >>> +++ unstable/xen/arch/x86/irq.c
>> >>> @@ -242,6 +242,20 @@ void destroy_irq(unsigned int irq)
>> >>>      xfree(action);
>> >>>  }
>> >>>  
>> >>> +static void release_old_vec(struct irq_desc *desc)
>> >>> +{
>> >>> +    unsigned int vector = desc->arch.old_vector;
>> >>> +
>> >>> +    desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
>> >>> +    cpumask_clear(desc->arch.old_cpu_mask);
>> >>> +
>> >>> +    if ( desc->arch.used_vectors )
>> >> 
>> >> Wouldn't it be better to clean the bitmap when vector !=
>> >> IRQ_VECTOR_UNASSIGNED?
>> > 
>> > No code path does / should call into here without the need to
>> > actually release the previous vector.
>> > 
>> >> I haven't checked all the callers, but I don't think it's valid to
>> >> call release_old_vec with desc->arch.old_vector ==
>> >> IRQ_VECTOR_UNASSIGNED, in which case I would add an ASSERT.
>> > 
>> > Well, yes, I probably could. However, as much as I'm in
>> > favor of ASSERT()s, I don't think it makes sense to ASSERT()
>> > basically every bit of expected state. In the end there would
>> > otherwise be more ASSERT()s than actual code.
>> 
>> Actually, upon second thought - let me add this, but then in an
>> even more strict form: Certain very low and very high numbered
>> vectors are illegal here as well, and we may then be able to use
>> the same validation helper elsewhere (in particular also for the
>> check that you've found to be wrong in _clear_irq_vector()).
> 
> Thanks, that LGTM.

And FTR - it _does_ trigger. I'm still struggling to explain why.
The only place where ->arch.move_in_progress gets set is
in _assign_irq_vector(), and the check I've put there for
debugging purposes doesn't trigger, i.e. the vectors put there
into ->arch.old_vector are valid.

Jan



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