Re: [PATCH 5/5] x86/kvm: Add KVM_FEATURE_MSI_EXT_DEST_ID

[David Woodhouse]

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.

Attachment: smime.p7s
Description: S/MIME cryptographic signature