Re: [patch 00/26] x64, x2apic/intr-remap: Interrupt-remapping and x2apic support

[Suresh Siddha <suresh.b.siddha@intel.com>]

On Thu, Jul 10, 2008 at 03:52:50PM -0700, Eric W. Biederman wrote:
> Suresh Siddha <suresh.b.siddha@intel.com> writes:
> 
> > Flushing the interrupt entry cache will take care of this. We modify the IRTE
> > and then flush the interrupt entry cache before cleaning up the original
> > vector allocated.
> >
> > Any new interrupts from the device will see the new entry. Old in flight
> > interrupts will be registered at the CPU before the flush of the cache is
> > complete.
> 
> That sounds nice in principle.  I saw cpu cache flushes, I saw writes.
> I did not see any reads which is necessary to get that behavior with
> the standard pci transaction rules.

qi_flush_iec() will submit an invalidation descriptor and will wait
till it finishes the invalidation of the interrupt entry cache.
qi_submit_sync() will do the job. Descriptor completion ensures that
the inflight interrupts are flushed.

> Having seen enough little races and misbehaving hardware I'm very paranoid
> about irq migration.  The current implementation is belt and suspenders
> and I still think there are races that I have missed.

Eric, This process irq migration is done on the cutting edge hardware
which was designed with all the feedback and experiences in the mind ;)

And also, I don't think we are deviating much from what we are currently doing.
We are still using cleanup vector etc, to clean up the previous vector
allocation.

> >> You are sizing an array as NR_IRQS this is something there should be
> > sufficient
> >> existing infrastructure to avoid.  Arrays sized by NR_IRQS is a significant
> >> problem both for scaling the system up and down so ultimately we need to kill
> >> this.  For now we should not introduce any new arrays.
> >
> > Ok. Ideally dynamic_irq_init()/cleanup() can take care of this. or
> > create_irq()/destroy_irq() and embed this as a pointer somewhere inside
> > irq_desc. I need to take a look at this more closer and post a fix up patch.
> 
> Sounds good.  Ultimately we are looking at handler_data or chip_data.
> There are very specific rules that meant I could not use them for
> the msi data but otherwise I don't remember exactly what the are for.
> IOMMU are covered though.

IOMMU is covered as part of pci_dev (pci_sysdata). But in the case of
interrupt-remapping, there are some interrupt resources like ioapics and
hpet, which don't have the corresponding pci dev. Will take a look at this.

> At least for msi the code you are working on was essentially unified
> when it was written, it just happened to have two copies.  I don't
> think I'm asking for heaving lifting.  Mostly just putting code that
> is touched into something other then the growing monstrosity that is
> ioapic.c

We can create msi.c which handles MSI specific handling. I will
look into this. But I def welcome somone beating me in posting those
patches :) I made a note of this however.

> Further can we please see some better abstractions.  In particular can
> we generate a token for the irq destination.  And have the msi and
> ioapic setup read that token and program it into the hardware.  The
> rules for which bits go where is exactly the same both with and
> without irq_remapping so having an if statement there seems to obscure
> what is really happening.  Especially if as it appears that we may be used
> the new token format with x2apics without remapping.

unfortunately x2apic can't be enabled with out enabling interrupt-remapping.
Interrupts don't work in majority of the configurations (as I mentioned
earlier). Programming IOAPIC RTE's and MSI address/data registers are
completely different based on the presence of interrupt-remapping.

> 
> My primary concern is that the end result be well factored irq handling code
> so it is possible to get in there and look at the code and maintain it.
> 
> A small part of that is the 32bit support.  Another part are the missing
> abstractions I described.  I don't know what else since I have barely scratched the surface patch
> review wise.

Please keep the expert comments coming.

thanks,
suresh