|
xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
|
|
Home