[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.13 2/2] x86/ioapic: don't use raw entry reads/writes in clear_IO_APIC_pin
On Thu, Nov 07, 2019 at 04:56:09PM +0100, Jan Beulich wrote: > 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. IMO generic IO-APIC code has not business playing with raw entries when interrupt remapping is enabled, the layout of IO-APIC entries in that case is vendor-specific, and hence the generic IO-APIC code is not able to parse it. For example the code in clear_IO_APIC_pin modifies the mask or the trigger fields of RAW entries, is there any guarantee that those fields don't have different meanings/layout when interrupt remapping is enabled? I can split the specific bugfix into a separate patch, but IMO the code in clear_IO_APIC_pin is not safe. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |