Message-ID: <Yf/PN8rBy3m5seU9@zn.tnic>
Date:   Sun, 6 Feb 2022 14:37:59 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Michael Roth <michael.roth@....com>
Cc:     Brijesh Singh <brijesh.singh@....com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        Tom Lendacky <thomas.lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        brijesh.ksingh@...il.com, tony.luck@...el.com, marcorr@...gle.com
Subject: Re: [PATCH v9 31/43] x86/compressed/64: Add support for SEV-SNP
 CPUID table in #VC handlers

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.

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.

And please explain to me what's so unclear about leaf->eax vs *eax?!

> and from the persective of someone auditing this from a security
> perspective that needs to quickly check what registers come from the
> CPUID table, what registers come from HV

Having a struct which is properly named will actually be beneficial in
this case:

	hv_leaf->eax

or even

	hv_reported_leaf->eax

vs

	*eax

Now guess which is which.

> But if we start passing around this higher-level structure that does
> not do anything other than abstract away e{a,b,c,x} to save on function
> arguments, things become muddier, and there's more pointer dereference
> operations and abstractions to sift through.

Huh?! I'm sorry but I cannot follow that logic here.

> 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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists