Re: [PATCH] x86/ioapic: sanitize IO-APIC pins before enabling the local APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.07.2023 13:29, Roger Pau Monné wrote:
> On Thu, Jul 13, 2023 at 09:49:14AM +0200, Jan Beulich wrote:
>> One other thing I finally figured was confusing me in the
>> description of the patch; re-quoting that paragraph here:
>>
>> "Fix this by moving the masking of IO-APIC pins ahead of the enabling
>>  of the local APIC.  Note that before doing such masking Xen attempts
>>  to detect the pin where the legacy i8259 is connected, and that logic
>>  relies on the pin being unmasked, hence the logic is also moved ahead
>>  of enabling the local APIC."
>>
>> The last sentence saying "also" is kind of odd with the first one
>> already stating this very movement. To me it's therefore unclear what
>> exactly the second sentence is intended to be telling me. I guess you
>> want to express that together with the making the detection logic is
>> also moved (i.e. the entire function is called earlier), but I'm
>> afraid this isn't the only way to read that second sentence.
> 
> Yes, you are reading it as I was intending.  It's merely a logic that
> the i8259 detection is also done slightly earlier, as it relies on the
> RTE masking not being tampered with.
> 
> What about:
> 
> "Fix this by moving the masking of IO-APIC pins ahead of the enabling
>  of the local APIC.  Note that before doing such masking Xen attempts
>  to detect the pin where the legacy i8259 is connected, and that logic
>  relies on the pin being unmasked, hence the logic is also moved ahead
>  of enabling the local APIC."
> 
> "Move the masking of the IO-APIC pins ahead of the setup of the local
>  APIC.  This has the side effect of also moving the detection of the
>  pin where the i8259 is connected, as it must be done before masking
>  any pins."
> 
> Is the aboce any better?

It is, thanks.

>  Would you rather prefer me to drop any
> mention of the i8259 detection being moved? (as it's just a side
> effect of moving the masking).

No, better mention this aspect.

> So to recap, I think we are in agreement that calling enable_IO_APIC()
> just ahead of the call to setup_local_APIC() is the preferred
> solution?

Well, yes and no. My preferred course of action for the issue at hand
would be to convert RTE 0 to ExtInt (under the mentioned set of
conditions). I agree though that we also want to move the masking of
RTEs, and for that I further agree with the placement mentioned above.

That said, you're the contributor, so it's still up to you what you
submit in what order (you could still decide to leave out the RTE
conversion change). The two of us not reaching agreement merely makes
a little more difficult to decide which of the changes to backport
then.

Jan