Messages in this thread | | | Date | Tue, 31 Mar 2009 12:38:37 -0700 | From | Jeremy Fitzhardinge <> | Subject | Re: [Xen-devel] Re: [GIT PULL] Xen for 2.6.30 #2 |
| |
Ingo Molnar wrote: > - review it in detail > 1- then after a round of review feedbacks merge it into the x86 tree > - then to test it there > - then to fix the (inevitable) bugs and go to 1 until bug-free > - then to stage it to linux-next > - then after many weeks and months, to eventually send it to Linus > > That's NOT the same thing as you sending it straight to Linus, > without the broad acks from the x86 maintainers for all details. >
I sent mail to you about this several days ago, announcing my intention to post if I didn't hear back from you. I heard nothing and went ahead.
I've been working with HPA to get him to review all the x86 interactions, and reviewed-by the patches accordingly. I have sent you these patches several times over the last month, but haven't seen any response.
> I had a quick look, and stuff like this is not acceptable: > > static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg) > { > - struct io_apic __iomem *io_apic = io_apic_base(apic); > + struct io_apic __iomem *io_apic; > + > + if (xen_initial_domain()) > + return xen_io_apic_read(apic, reg); > + > + io_apic = io_apic_base(apic); > > Should be done by introducing your own xen specific irqchip. And > this is not news to you, it has been told you in _early February_: > > http://lkml.indiana.edu/hypermail/linux/kernel/0902.1/00410.html > > You didnt reply to that feedback of mine and you didnt fix it. >
Yes, you've suggested that several times; that particular mail was about a different issue, for which it also wasn't the answer. (I didn't reply because shortly after you sent me with another mail saying "Ok, never mind my comment on the do_IRQ() detail, this looks good after all[...]")
We *do* define our own irqchip (drivers/xen/events.c), but that interface doesn't cover IO apic interactions, which are primarily used when doing apic setup, and to set up interrupt routing. ioapic_write_entry(), for example, is not reached via any irq_chip method.
In this case we want the normal apic setup to go ahead, but the actual read/writes to the apic registers need to be directed to a hypercall.
> We are not putting some xen-specific hack into core x86 code ... The > irqchip method wont put overhead and ugliness into native Linux. > It's an existing abstraction for such stuff, use it and extend it if > needed. > No, it isn't, because it doesn't encapsulate the whole apic layer. I don't want to duplicate all that code; I want to use it (mostly) as-is.
I went around this several times with HPA. My initial version of the patch introduced an io_apic_ops and hooked it appropriately. He objected on the grounds that its pointless adding an extra level of abstraction for a single user; he preferred a straightforward call, as it is here. This change is Xen-specific, but it disappears completely if you don't enable Xen and it is not on a performance-critical path. If any other users appear here, we can easily add an appropriate abstraction layer.
> And stuff like this in arch/x86/kernel/pci-swiotlb.c: > > dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr) > { > +#ifdef CONFIG_PCI_XEN > + if (xen_pv_domain()) > + return xen_phys_to_bus(paddr); > +#endif > return paddr; > } > > and the other PCI bits very much need the ack of the PCI and > sw-IOMMU folks (Fujita Tomonori mainly). I'd be surprised if they > werent disgusted by it. >
I believe they've been cc:ed on all these patches, but I'll repost the relevent bits to make sure. The #ifdef definitely should not be there.
> I dont mind pull requests outside of maintenance boundaries, as long > as the changes are good. >
Well, I've been trying to get your comments about these patches for at least a month now, with the intention of hitting this merge window. I realize you're very busy overall, so when HPA took the time to review them I didn't see the need to also press it with you. And I certainly wasn't going to let the window go by without doing anything.
> You know our stance which is very simple: dont put in Xen-only hooks > that slow down native, and get rid of the existing Xen-only hooks. >
Yes, I understand that. Unlike the pvops stuff, the dom0 changes are largely all init-time and setup, and so have no performance impact.
J
| |