On Tue, 2020-10-13 at 11:28 +0200, Thomas Gleixner wrote: > On Tue, Oct 13 2020 at 08:52, David Woodhouse wrote: > > On Tue, 2020-10-13 at 00:13 +0200, Thomas Gleixner wrote: > > + dom = irq_find_matching_fwspec(fwspec, DOMAIN_BUS_IR); > > + if (dom) > > + return IS_ERR(dom) ? NULL : dom; > > + > > + return x86_vector_domain; > > +} > > > > Ick. There's no need for that. > > > > Eliminating that awful "if not found then slip the x86_vector_domain in > > as a special case" was the whole *point* of using > > irq_find_matching_fwspec() in the first place. > > The point was to get rid of irq_remapping_get_irq_domain(). My reason for doing it was to get rid of irq_remapping_get_irq_domain() *because* I hated the special-casing and magical slipping in of x86_vector_domain when it returned NULL. > And TBH, > > if (apicid_valid(32768)) > > is just another way to slip the vector domain in. It's just differently > awful. For me, that's very much not just "slipping the vector domain in". That's the vector domain returning true in its *own* ->select() function, in the circumstances where it wants to be used. The key difference is that nobody needs an external magic pointer to the x86_vector_domain. In a true irqdomain hierarchy system, shouldn't we be trying to eliminate *all* those magic pointers to specific domains, if we can? And sure, the apicid_valid(32768) as a proxy for irq_remapping_enabled is a bit of an ugly trick. I suppose we can explicitly expose irq_remapping_enabled from drivers/iommu if we wanted to. > Having an explicit answer from the search for IR: > > - Here is the domain > - Your device is not registered properly > - IR not enabled or not supported > > is way more obvious than the above disguised is_remapping_enabled() > check. I just don't even like thinking of it as a 'search for IR'. HPET shouldn't be caring about IR any more than PCI devices do. It just wants its parent irqdomain, that's all. For I/OAPIC there's the slight complexity that it does actually ack level-triggered interrupts differently when it's behind IR. But I don't think we need a whole separate irq_chip for that; surely it could be handled internally in ioapic_ack_level() ? Either way, even with that slight hack it's nicer to think of mp_irqdomain_create() just wanting to find its parent domain, without any special knowledge of IR and falling back to x86_parent_domain. The hack for IR level-ack is then self-contained.