On 09.05.2023 16:03, Andrew Cooper wrote: > On 08/05/2023 8:45 am, Jan Beulich wrote: >> On 04.05.2023 21:39, Andrew Cooper wrote: >>> When adding new words to a featureset, there is a reasonable amount of >>> boilerplate and it is preforable to split the addition into multiple >>> patches. >>> >>> GCC 12 spotted a real (transient) error which occurs when splitting >>> additions >>> like this. Right now, FEATURESET_NR_ENTRIES is dynamically generated from >>> the >>> highest numeric XEN_CPUFEATURE() value, and can be less than what the >>> FEATURESET_* constants suggest the length of a featureset bitmap ought to >>> be. >>> >>> This causes the policy <-> featureset converters to genuinely access >>> out-of-bounds on the featureset array. >>> >>> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it >>> specifically to grow larger than FEATURESET_NR_ENTRIES. >>> >>> Reported-by: Jan Beulich <jbeul...@suse.com> >>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >> While, like you, I could live with the previous patch even if I don't >> particularly like it, I'm not convinced of the route you take here. > > It's the route you tentatively agreed to in > https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82b...@suse.com/
Right. Yet I deliberately said "may be the best" there, as something better might turn up. And getting the two numbers to always agree, as suggested, might end up being better. >> Can't >> we instead improve build-time checking, so the issue spotted late in the >> build by gcc12 can be spotted earlier and/or be connected better to the >> underlying reason? > > I don't understand what you mean by this. For the transient period of > time, Xen's idea of a featureset *is* longer the autogen idea, hence the > work in this patch to decouple the two. Well, this part of my reply was just aiming at diagnosing the issue as early as possible and as clearly as possible, such that one can easily and quickly adjust whatever is missing in a change being worked on. The main goal of course needs to be that this can't easily go entirely unnoticed (as had happened, which has prompted this attempt of yours of addressing the issue). I.e. diagnosing late is still far better than failing to (without the compiler spotting it) altogether. >> One idea I have would deal with another aspect I don't like about our >> present XEN_CPUFEATURE() as well: The *32 that's there in every use of >> the macro. How about >> >> XEN_CPUFEATURE(FSRCS, 10, 12) /*A Fast Short REP CMPSB/SCASB */ >> >> as the common use and e.g. >> >> XEN_CPUFEATURE(16) >> >> or (if that ends up easier in gen-cpuid-py and/or the public header) >> something like >> >> XEN_CPUFEATURE(, 16, ) >> >> as the placeholder required for (at least trailing) unpopulated slots? Of >> course the macro used may also be one of a different name, which may even >> be necessary to keep the public header reasonably simple; maybe as much >> as avoiding use of compiler extensions there. (This would then mean to >> leave alone XEN_CPUFEATURE(), and my secondary adjustment would perhaps >> become an independent change to make.) > > Honestly, I don't want to hide the *32 part of the expression. This > logic is already magic enough. Well, I certainly wouldn't insist, but to me it looks pretty odd to have it on all the lines. > If we were to do something like this, I don't see what's wrong with just > having the value as a regular define at the end anyway. > > One way or another with this approach, something needs updating in the > tail of cpufeatureset.h, and gen-cpuid.py can easily parse for a > specific named constant, and it will be less magic than overloading > XEN_CPUFEATURE(). If less overloading is deemed better - fine with me. Looking at the script I wasn't sure hunting for an entirely different construct would end up being more tidy. What isn't really clear to me from your reply: Are you okay with trying such an alternative approach? Or are you opposed to it? Or something in the middle, like you being okay, but only if it's not you to actually try it out? >>> To preempt what I expect will be the first review question, no FEATURESET_* >>> can't become an enumeration, because the constants undergo token >>> concatination >>> in the preprocess as part of making DECL_BITFIELD() work. >> Just as a remark: I had trouble understanding this. Which was a result of >> you referring to token concatenation being the problem (which is fine when >> the results are enumerators), when really the issue is with the result of >> the concatenation wanting to be expanded to a literal number. >> >> Then again - do CPUID_BITFIELD_<n> really need to be named that way? >> Couldn't they equally well be CPUID_BITFIELD_1d, CPUID_BITFIELD_e1c, and >> alike, thus removing the need for intermediate macro expansion? > > gen-cpuid.py doesn't know the short names; only Xen does, which is why > the expansion needs to know the name->word mapping. > > I suppose this can be fixed, but it will require more magic comments and > more parsing to achieve. Okay, let's leave this entire aspect aside for now. It started from a not-to-be-committed remark only anyway. Jan
