don't really want to do this if the BUS_REMAP_INTR failed as then we'll have no functional interrupt, but I am not sure of other tear-down we have to do (i.e., undoing Allocate IDT vectors) so I think the warning message is better than nothing

I think the extra cleanup would be:

if (msi->msi_intsrc.is_handlers > 0)
    apic_disable_vector(msi->msi_cpu, msi->msi_vector);
msi->msi_cpu = old_id;
msi->msi_vector = old_vector;
for (i = 1; i < msi->msi_count; i++) {
    sib = ....;
    if (sib->msi_intsrc.is_handlers > 0)
        apic_disable_vector(sib->msi_cpu, sib->msi_vector);
    sib->msi_cpu = old_id;
    sib->msi_cpu = old_vector + i;
}

for (i = 0; i < msi->msi_count; i++)
    apic_free_vector(apic_id, vector + i);
return (error);

However, what I would probably do instead, is to restructure this a bit so you avoid changing msi_cpu and msi_vector for the secondary vectors (sib) until after the REMAP returns success.

I would also move the bootverbose printfs after the REMAP as well.

I still need to do this prior to BUS_REMAP_INTR, yeah?

and the apic_disable_vector / apic_free_vector needs to be done after BUS_REMAP_INTR in the failing case

but, maybe I can set old_vector = vector and old_id = apic_id and use the same apic_disable_vector / apic_free_vector code

we're missing an apic_free_vector for old_vector, no?

oh, no we do it at line 330

The issue here I think is that this will call back into the MSI code and expects to see the "new" values in msi_cpu and msi_vector. You can defer updating them for all the siblings (sib), but the "main" MSI IRQ in the group you have to update before calling this.

So these two lines have to be before the BUS_REMAP_INTR call, and you have to undo them in the error case.

Might be a nicer error message?

So here you would insert:

msi->msi_cpu = old_id;
msi->msi_vector = old_vector;

before updating the old values.