| Message ID | cover.1712915011.git.teddy.astie@vates.tech (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86/iommu: Drop IOMMU support when cx16 isn't supported | expand |
On 12/04/2024 1:33 pm, Teddy Astie wrote: > All hardware that supports VT-d/AMD-Vi that exists also supports cx16 (aside > specifically crafted virtual machines). > > Some IOMMU code paths in Xen consider cases where VT-d/AMD-Vi is supported > while cx16 isn't, those paths may be bugged and are barely tested, dead code > in practice. > > Disable IOMMU in case we have IOMMU hardware but no cx16, then cleanup > no-cx16 handling logic from VT-d and AMD-Vi drivers. > > Teddy > > Changed in v2: > > * Added cleanup no-cx16 code for x2APIC > * Fixed commit and code formatting > * Added missing Suggested-by note > > Changed in v3: > > * Use -ENODEV instead of -ENOSYS. Hmm - I've still got a double copy of patch 2. No idea what's going on, but I'll make do with what b4 thinks is on the list. A couple of things. 1) You introduced trailing whitespace in patch 1 in the middle line here: > + ASSERT(spin_is_locked(&iommu->intremap.lock)); + + old_ire = *entry; 2) In your commit messages, the grammar is a bit awkward. It would be clearer to say: "All hardware with VT-d/AMD-Vi has CMPXCHG16 support. Check this at initialisation time, and remove the effectively-dead logic for the non-cx16 case." just as you've done in the cover letter. 3) In patch 1, you shouldn't modify x2apic_bsp_setup() like that. x2APIC && no-IOMMU is a legal combination. Instead, you should put a cx16 check in both driver's supports_x2apic() hooks. 4) In patch 3, you should drop the Suggested-by me. You found that one all yourself. ~Andrew
Le 12/04/2024 à 16:49, Andrew Cooper a écrit : > 1) You introduced trailing whitespace in patch 1 in the middle line here: > >> + ASSERT(spin_is_locked(&iommu->intremap.lock)); + + old_ire = *entry; Good catch, will fix > 2) In your commit messages, the grammar is a bit awkward. It would be > clearer to say: > > "All hardware with VT-d/AMD-Vi has CMPXCHG16 support. Check this at > initialisation time, and remove the effectively-dead logic for the > non-cx16 case." > > just as you've done in the cover letter. Yes > 3) In patch 1, you shouldn't modify x2apic_bsp_setup() like that. > x2APIC && no-IOMMU is a legal combination. > > Instead, you should put a cx16 check in both driver's supports_x2apic() > hooks. In this case, you mean both intel_iommu_supports_eim and iov_supports_xt (AMD) ? > > 4) In patch 3, you should drop the Suggested-by me. You found that one > all yourself. > Will change this. Teddy --- Teddy Astie | Vates XCP-ng Intern XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
On 12/04/2024 4:51 pm, Teddy Astie wrote: > Le 12/04/2024 à 16:49, Andrew Cooper a écrit : >> 3) In patch 1, you shouldn't modify x2apic_bsp_setup() like that. >> x2APIC && no-IOMMU is a legal combination. >> >> Instead, you should put a cx16 check in both driver's supports_x2apic() >> hooks. > In this case, you mean both intel_iommu_supports_eim and iov_supports_xt > (AMD) ? Yes, with the changes in the appropriate patch. ~Andrew