|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 00/12] const-ify vendor checks
On Mon Feb 9, 2026 at 11:15 AM CET, Jan Beulich wrote: > On 09.02.2026 11:05, Alejandro Vallejo wrote: >> On Mon Feb 9, 2026 at 10:21 AM CET, Jan Beulich wrote: >>> On 06.02.2026 17:15, Alejandro Vallejo wrote: >>>> High level description >>>> ====================== >>>> >>>> When compared to the RFC this makes a different approach The series >>>> introduces >>>> cpu_vendor() which maps to a constant in the single vendor case and to >>>> (boot_cpu_data.vendor & X86_ENABLED_VENDORS), where X86_ENABLED_VENDORS is >>>> a >>>> mask of the compile-time chosen vendors. This enables the compiler to >>>> detect >>>> dead-code at the uses and remove all unreachable branches, including in >>>> switches. >>>> >>>> When compared to the x86_vendor_is() macro introduced in the RFC, this is >>>> simpler. It achieves MOST of what the older macro did without touching the >>>> switches, with a few caveats: >>>> >>>> 1. Compiled-out vendors cause a panic, they don't fallback onto the >>>> unknown >>>> vendor case. In retrospect, this is a much saner thing to do. >>> >>> I'm unconvinced here. Especially our Centaur and Shanghai support is at best >>> rudimentary. Treating those worse when configured-out than when >>> configured-in >>> feels questionable. >> >> Isn't that the point of configuring out? > > That's what I'm unsure about. I'm really missing what you're trying to make, sorry. How, if at all, is it helpful for a hypervisor with a compiled out vendor to be bootable on a machine of that vendor? > >> Besides the philosophical matter of whether or not a compiled-out vendor >> should be allowed to run there's the more practical matter of what to do >> with the x86_vendor field of boot_cpu_data. Because at that point our take >> that cross-vendor support is forbidden is a lot weaker. If I can run an >> AMD-hypervisor on an Intel host, what then? What policies would be allowed? >> If I >> wipe x86_vendor then policies with some unknown vendor would be fine. Should >> the >> leaves match too? If I do not wipe the field, should I do black magic to >> ensure >> the behaviour is different depending on whether the vendor is compiled in or >> not? What if I want to migrate a VM currently running in this hypothetical >> hypervisor? The rules becomes seriously complex. >> >> It's just a lot cleaner to take the stance that compiled out vendors can't >> run. >> Then everything else is crystal clear and we avoid a universe's worth of >> corner >> cases. I expect upstream Xen to support all cases (I'm skeptical about the >> utility of the unknown vendor path, but oh well), but many downstreams might >> benefit from killing off support for vendors they really will never touch. > > To them, will panic()ing (or not) make a difference? One would hope not because the're compiling them out for a reason. But for upstream, not panicking brings a sea of corner cases. The ones I mentioned above is not the whole list. Turning the question around. Who benefits from not panicking? > >>>> 2. equalities and inequalities have been replaced by equivalent >>>> (cpu_vendor() & ...) >>>> forms. This isn't stylistic preference. This form allows the compiler >>>> to merge the compared-against constant with X86_ENABLED_VENDORS, >>>> yielding >>>> much better codegen throughout the tree. >>>> >>>> The effect of (2) triples the delta in the full build below. >>>> >>>> Some differences might be attributable to the change from policy vendor >>>> checks >>>> to boot_cpu_data. In the case of the emulator it caused a 400 bytes >>>> increase >>>> due to the way it checks using LOTS of macro invocations, so I left that >>>> one >>>> piece using the policy vendor except for the single vendor case. >>> >>> For the emulator I'd like to point out this question that I raised in the >>> AVX10 series: >> >> There's no optimisation shortage for the emulator. For that patch I just >> ensure I didn't make a tricky situation worse. It is much better in the >> single-vendor case. >> >>> "Since it'll be reducing code size, we may want to further convert >>> host_and_vcpu_must_have() to just vcpu_must_have() where appropriate >>> (should be [almost?] everywhere)." >>> >>> Sadly there was no feedback an that (or really on almost all of that work) >>> at >>> all so far. >> >> It sounds fairly orthogonal to this, unless I'm missing the point. > > It's largely orthogonal, except that if we had gone that route already, your > codegen diff might look somewhat different. > >> In principle that would be fine. the vCPU features whose emulation requires >> special instructions are most definitely a subset of those of the host >> anyway. >> >> I agree many cases could be simplified as you describe. >> >> I do see a worrying danger of XSA should the max policy ever exceed the >> capabilities of the host on a feature required for emulating some instruction >> for that very feature. Then the guest could abuse the emulator to trigger #UD >> inside the hypervisor's emulation path. > > Well, that max-policy related question is why I've raised the point, rather > than making (more) patches right away. All jmp_rel() macros have amd_like() inside, and other checks are open-coded in many places. The problem is that offsets into the policy pointer (which is stored in a register) end up being global accesses to an offset from a non-register-cached 64bit address. And that adds up. having an amd_like boolean in the ctxt would've helped, but I went for the least intrusive solution. I'm not sure how what you brought up would've helped for this particular codegen matter. Cheers, Alejandro
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |