[lkml]   [2020]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 5/5] x86/kvm: Add KVM_FEATURE_MSI_EXT_DEST_ID
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.
[unhandled content-type:application/x-pkcs7-signature]
 \ /
  Last update: 2020-10-13 12:16    [W:0.181 / U:1.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site