Re: [PATCH] Add intr_mask() / intr_unmask() interface to mask / unmask individual interrupt sources

[Jason Thorpe <thorpej%me.com@localhost>]

> On Aug 11, 2019, at 12:04 PM, Maxime Villard <max%m00nbsd.net@localhost> wrote:
> 
> Le 11/08/2019 à 19:36, Jason Thorpe a écrit :
>> Anyone?  Bueller?
> 
> Small notes:
> 
> +	const u_long psl = x86_read_psl();
> +	x86_disable_intr();
> 
> Not sure, why disable interrupts? To prevent an interrupt between the
> is_mask_count and pic_hw* changes?

The is_mask_count field is nominally protected by cpu_lock ... but the vectors don't take that lock.  So I disable interrupts there to ensure the hw state and the mask_count is consistent from the PoV of the vectors.  I'll add some comments to explain it.

> +	if (mask) {
> +		source->is_mask_count++;
> +		KASSERT(source->is_mask_count != 0);
> +		(*pic->pic_hwmask)(pic, ih->ih_pin);
> 
> Seems like pic_hwmask only needs to be called when is_mask_count==0.

Yes, good catch.  I also noticed a few other places where I should be checking is_mask_count before calling pic_hwunmask, and also realized that I needed to add some code to handle the set_affinity flow ... it's complicated by the fact that an intr_mask()'d interrupt will always be "pending", so some care is needed.  Specifically, before waiting for "pending" to drain out, I now mark the interrupt sources as "distribution-pending" ... so when intr_unmask() comes along, it won't re-enable the source at the hardware if it sees that, but will still force processing of the source if it was marked as interrupt-pending.

> Note that XENINTRSTUB will also need the same changes.

Thanks, I'll take a look at those, too.

-- thorpej