On Sun, Feb 06, 2022 at 02:37:59PM +0100, Borislav Petkov wrote:
> First of all,
>
> let me give you a very important piece of advice for the future:
> ignoring review feedback is a very unfriendly thing to do:
>
> - if you agree with the feedback, you work it in in the next revision
>
> - if you don't agree, you *say* *why* you don't
>
> What you should avoid is what you've done - ignore it silently. Because
> reviewing submitters code is the most ungrateful work around the kernel,
> and, at the same time, it is the most important one.
>
> So please make sure you don't do that in the future when submitting
> patches for upstream inclusion. I'm sure you can imagine, the ignoring
> can go both ways.
Absolutely, I know a thorough review is grueling work, and would never
want to give the impression that I don't appreciate it. Was just hoping
to revisit these in the context of v9 since there were some concerning
things in flight WRT the spec handling and I was sort of focused on
getting ahead of those in case they involved firmware/spec changes. But
I realize that's resulted in a waste of your time and I should have at
least provided some indication of where I was with these before your
review. Won't happen again.
>
> On Sat, Feb 05, 2022 at 10:22:49AM -0600, Michael Roth wrote:
> > The documentation for lea (APM Volume 3 Chapter 3) seemed to require
> > that the destination register be a general purpose register, so it
> > seemed like there was potential for breakage in allowing GCC to use
> > anything otherwise.
>
> There's no such potential: binutils encodes the unstruction operands
> and what types are allowed. IOW, the assembler knows that there goes a
> register.
>
> > Maybe GCC is smart enough to figure that out, but since we know the
> > constraint in advance it seemed safer to stick with the current
> > approach of enforcing that constraint.
>
> I guess in this particular case it won't matter whether it is "=r" or
> "=g" but still...
>
> > I did look into it and honestly it just seemed to add more abstractions that
> > made it harder to parse the specific operations taken place here. For
> > instance, post-processing of 0x8000001E entry, we have e{a,b,c,d}x from
> > the CPUID table, then to post process:
> >
> > switch (func):
> > case 0x8000001E:
> > /* extended APIC ID */
> > snp_cpuid_hv(func, subfunc, eax, &ebx2, &ecx2, NULL);
>
> Well, my suggestion was to put it *all* in the subleaf struct - not just
> the regs:
>
> struct cpuid_leaf {
> u32 func;
> u32 subfunc;
> u32 eax;
> u32 ebx;
> u32 ecx;
> u32 edx;
> };
>
> so that the call signature is:
>
> snp_cpuid_postprocess(struct cpuid_leaf *leaf)
>
>
> > and it all reads in a clear/familiar way to all the other
> > cpuid()/native_cpuid() users throughout the kernel,
>
> maybe it should read differently *on* *purpose*. Exactly because this is
> *not* the usual CPUID handling code but CPUID verification code for SNP
> guests.
Well, there's also sev_cpuid_hv(), which really is sort of a variant of
cpuid()/native_cpuid() helpers that happens to use the GHCB protocol, and
snp_cpuid() gets called at roughly the same callsite in the #VC handler,
so it made sense to me to just stick to a similar signature across all of
them. But no problem implementing it as you suggested, that was just my
reasoning there.
>
> And please explain to me what's so unclear about leaf->eax vs *eax?!
It's not that one is unclear, it's just that, in the context of reading
through a function which has a lot of similar/repetitive cases for
various leaves:
snp_cpuid_hv(fn, subfn, &eax, &ebx2, NULL, NULL)
ebx = fixup(ebx2)
seems slightly easier to process to me than:
snp_cpuid_hv(&leaf_hv)
leaf->eax = leaf_hv->eax
leaf->ebx = fixup(leaf_hv->ebx)
because I already have advance indication from the function call that eax
will be overwritten by hv, ebx2 will getting fixed up in subsequent lines,
and ecx/edx will be used as is, and don't need to parse multiple lines to
discern that.
But I know I may very well be biased from staring at the existing
implementation so much, and that maybe your approach would be clearer to
someone looking at the code who isn't as familiar with that pattern.
>
> > I saved the diff from when I looked into it previously (was just a
> > rough-sketch, not build-tested), and included it below for reference,
> > but it just didn't seem to help with readability to me,
>
> Well, looking at it, the only difference is that the IO is done
> over a struct instead of separate pointers. And the diff is pretty
> straight-forward.
>
> So no, having a struct cpuid_leaf containing it all is actually better
> in this case because you know which is which if you name it properly and
> you have a single pointer argument which you pass around - something
> which is done all around the kernel.
Ok, will work this in for v10. My plan is to introduce this struct:
struct cpuid_leaf {
u32 fn;
u32 subfn;
u32 eax;
u32 ebx;
u32 ecx;
u32 edx;
}
as part of the patch which introduces sev_cpuid_hv():
x86/sev: Move MSR-based VMGEXITs for CPUID to helper
and then utilize that for the function parameters there, here, and any
other patches in the SNP code involved with fetching/manipulating cpuid
values before returning them to the #VC handler.
Thanks!
-Mike
>
> --
> Regards/Gruss,
> Boris.
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7Cd409d10fc3cc41c4cbfb08d9e975ebfc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637797515011518153%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2FDjnNrQ7a8%2F6eI%2FRVJJ2vmLEpeDaAeDVOJcW9CBnggU%3D&reserved=0
