Re: [Xen-devel] [PATCH for-4.13 2/2] x86/ioapic: don't use raw entry reads/writes in clear_IO_APIC_pin

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.11.2019 16:46, Roger Pau Monné  wrote:
> On Thu, Nov 07, 2019 at 04:28:56PM +0100, Jan Beulich wrote:
>> On 07.11.2019 16:06, Roger Pau Monne wrote:
>>> @@ -530,9 +530,9 @@ static void clear_IO_APIC_pin(unsigned int apic, 
>>> unsigned int pin)
>>>       */
>>>      memset(&entry, 0, sizeof(entry));
>>>      entry.mask = 1;
>>> -    __ioapic_write_entry(apic, pin, true, entry);
>>> +    __ioapic_write_entry(apic, pin, false, entry);
>>
>> I may be able to understand why this one can't use raw mode, but as
>> per above a better overall description is needed.
> 
> Yes, this is the one that's actually incorrect, but see my reasoning
> below.
> 
>>
>>> -    entry = __ioapic_read_entry(apic, pin, true);
>>> +    entry = __ioapic_read_entry(apic, pin, false);
>>>      if (entry.irr)
>>>          printk(KERN_ERR "IO-APIC%02x-%u: Unable to reset IRR\n",
>>>                 IO_APIC_ID(apic), pin);
>>
>> This read again shouldn't need conversion, as the IRR bit doesn't
>> get touched (I think) by the interrupt remapping code during the
>> translation it does.
> 
> TBH, I think raw mode should only be used by the iommu code in order
> to setup the entries to point to the interrupt remapping table,
> everything else shouldn't be using raw mode. While it's true that some
> of the cases here are safe to use raw mode I would discourage it's
> usage as it can lead to issues, and this is not a performance critical
> path anyway.

You also should take the other possible perspective - not using
raw mode means going through interrupt remapping logic, which
can (needlessly) trigger errors. I think you want to break the
patch into a necessary and an optional part. The optional part
should be discussed separately and deferred until after 4.13.

Jan

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