The change is more intrusive than I would like because the feature
requires that a vector number is written to a special register.
Thus, now the vector number has to be provided to lapic_eoi().
It was readily available in the IO-APIC and MSI cases, but the IPI
handlers required more work.
Also, we now store the VMM IPI number in a global variable, so that it
is available to the justreturn handler for the same reasons.
Details
Details
Tested on my 10h machine with both i386 and amd64 kernels.
Also, I tested bhyve/vmm with the amd64 kernel.
All devices worked as before, no anomalities were seen.
Some devices uses MSIs, some IO-APIC interrupts.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/i386/i386/apic_vector.s | ||
|---|---|---|
| 203 ↗ | (On Diff #25958) | What is this ? AMD64 ABI passes arg0 in %edi. |
| sys/x86/x86/local_apic.c | ||
| 450 ↗ | (On Diff #25958) | You moved the functions around, but old location contains a better variant of amd_read_ext_features(), at least. |
| 1033 ↗ | (On Diff #25958) | I suggest to define two symbolic constants e.g. EOIS_INTEL and EOIS_AMD, and assign it to lapic_eoi_suppression. Then you would only need to compare lapic_eoi_suppression with the constant (one check), instead of verifying that the var is not zero and check vendor (two checks). |
| sys/i386/i386/apic_vector.s | ||
|---|---|---|
| 203 ↗ | (On Diff #25958) | This is i386 file. |
| sys/x86/x86/local_apic.c | ||
| 450 ↗ | (On Diff #25958) | Oh! Thank you for noticing. |
| 1033 ↗ | (On Diff #25958) | This sounds like a good suggestion. One concern is how to keep things simple for the tunable. |
| sys/x86/x86/local_apic.c | ||
|---|---|---|
| 1033 ↗ | (On Diff #25958) | You only need to distinguish between zero and non-zero for tunable; any non-zero value should be adjusted into the vendor constant. |
Comment Actions
address comment form kib:
- restore better version of amd_read_ext_features
- use different values of lapic_eoi_suppression to distinguish Intel and AMD SEOI implementations
| sys/amd64/amd64/apic_vector.S | ||
|---|---|---|
| 312 ↗ | (On Diff #25972) | All this comment is outdated. |
| 318 ↗ | (On Diff #25972) | It would be much less wordy to define the vmm_ipinum in C and only reference the symbol in asm. |
| sys/amd64/vmm/vmm.c | ||
| 218 ↗ | (On Diff #25972) | I.e. just remove static there. |
| sys/x86/x86/local_apic.c | ||
| 462 ↗ | (On Diff #25972) | 'else' is not needed, -1 line. |
| 491 ↗ | (On Diff #25972) | Merge all int vars declarations into single line. |
| sys/amd64/amd64/apic_vector.S | ||
|---|---|---|
| 318 ↗ | (On Diff #25972) | I agree, but what would be a good place to do it? (see my other comment) |
| sys/amd64/vmm/vmm.c | ||
| 218 ↗ | (On Diff #25972) | vmm can be compiled as a modules (in fact, I am not sure if it can be compiled into the kernel). |
| sys/amd64/vmm/vmm.c | ||
|---|---|---|
| 218 ↗ | (On Diff #25972) | I do not see a problem with #ifdef amd64 around a define in local_apic.c. |
Comment Actions
It seems that the Specific EOI feature (along with LAPIC side interrupt masking) is only useful for assigning hardware interrupts to virtual machines.
It does not provide performance benefits similar to Intel EOI Suppression feature.
So, Specific EOI could still be useful, but given the amount of code change and no immediate use for it, I am closing this request now.
We can reconsider the code later if we see that we need it.