msi: report error from BUS_REMAP_INTR

[emaste]

Summary

BUS_REMAP_INTR failing is likely non-fatal, resulting in a performance issue rather than a functional issue. However we should report failure so that bugs can be found and fixed.

[emaste]

Have to have D41449 first or we'll have false warnings.

[emaste]

sys/x86/x86/msi.c

330

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

[jhb]

sys/x86/x86/msi.c

306

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);

[jhb]

sys/x86/x86/msi.c

306

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.

[emaste]

untested attempt at @jhb's suggestion

[emaste]

sys/x86/x86/msi.c

297–298

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

309–318

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

338

we're missing an apic_free_vector for old_vector, no?

338

oh, no we do it at line 330

[emaste]

take 3

[emaste]

take 4

[emaste]

take n+1

[emaste]

Proposed commit message

msi: handle error from BUS_REMAP_INTR

Previously we silently ignored a failure from BUS_REMAP_INTR, which
would result in non-functional interrupts.

Now we allocate and enable new vectors, but postpone assignment of new
APIC IDs and vectors until after BUS_REMAP_INTR is successful.  We then
disable and free the old vectors.  If BUS_REMAP_INTR fails we leave the
old configuration intact, and disable and free the new, unused vectors.

Sponsored by:   The FreeBSD Foundation

[jhb]

sys/x86/x86/msi.c

304

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.

307

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

323–324

Might be a nicer error message?

325

So here you would insert:

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

before updating the old values.

[emaste]

move msi_cpu/msi_vector setting back before BUS_REMAP_INTR

[emaste]

use device_printf

[jhb]

Thanks for reworking this.

sys/x86/x86/msi.c

289