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

[David Woodhouse]

On Tue, 2020-10-13 at 00:13 +0200, Thomas Gleixner wrote:
> On Mon, Oct 12 2020 at 21:20, David Woodhouse wrote:
> > On Mon, 2020-10-12 at 20:38 +0200, Thomas Gleixner wrote:
> > > Nasty, but way better than what we have now. 
> > 
> > Want me to send that out in email or is the git tree enough for now?
> > 
> > I've cleaned it up a little and fixed a bug in the I/OAPIC error path.
> 
> Mail would be nice once you are confident with the pile.

After the next cup of coffee. Will send it as an incremental series on
top of my previous ext_dest_id series.

> > Still not entirely convinced about the apic->apic_id_valid(32768) thing
> > but it should work well enough, and doesn't require exporting any extra
> > state from apic.c that way.
> 
> Yeah, that part is odd.
> 
> I really dislike the way how irq_find_matching_fwspec() works. The 'rc'
> value is actually boolean despite being type 'int' and if 'rc' is not 0
> then it returns the domain even if 'rc' is an error code.
> 
> But that does not allow to return error codes from a domain match() /
> select() callback which is what we really want to express that there is
> something fishy.

I don't know that we need to return an explicit error. I made
x86_vector_select() refuse to match if IRQ remapping is enabled, which
means that the irq_find_matching_fwspec() call will fail by returning
NULL in precisely the cases it should. (Which should never happen; qv).

That failure means HPET will refuse to set up MSI, or I/OAPIC will
refuse to initialise (later causing a BUG and a failure to boot, which
is probably the correct thing to do).

It's all fine.

+       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.

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