Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <3e9a26c3-8eee-88f5-f8e2-8a2dd2c028ea@intel.com>
 <20210602194220.2227863-1-sathyanarayanan.kuppuswamy@linux.intel.com> <20210602194220.2227863-2-sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <20210602194220.2227863-2-sathyanarayanan.kuppuswamy@linux.intel.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Sat, 5 Jun 2021 14:56:22 -0700
Message-ID: <CAPcyv4jR4Ci=yKmCWk2toNFr-8NBy-MXfDYiH0Xmv9KFiBm2wQ@mail.gmail.com>
Subject: Re: [RFC v2-fix-v2 1/2] x86/sev-es: Abstract out MMIO instruction decoding
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@intel.com>,
        Tony Luck <tony.luck@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kirill Shutemov <kirill.shutemov@linux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
        Raj Ashok <ashok.raj@intel.com>,
        Sean Christopherson <seanjc@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 2, 2021 at 12:42 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>

Lead in with the what because I read 2 paragraphs to figure out that
this was a prep patch.

"In preparation for sharing MMIO instruction decode between SEV-ES and
TDX factor out the common decode into a new insn_decode_mmio()
helper."

> For regular virtual machine, MMIO is handled by the VMM: KVM
> emulates instruction that caused MMIO. But, this model doesn't
> work for a secure VMs (like SEV or TDX) as VMM doesn't have
> access to the guest memory and register state. VMM needs
> assistance in handling MMIO: it induces exception in the guest.
> Guest has to decode the instruction and handle it on its own.
>
> Instruction decoding logic is similar between AMD SEV and TDX
> code. So extract the decoding code to insn-eval.c where it can
> be used by both SEV and TDX.
>
> This code adds no functional changes. It is only build-tested
> for SEV.

The diff is such that I could not verify "no functional change" change
without doing more careful analysis. Typically with non-trivial
refactoring they are split out over a few patches with a final removal
of replaced infra at the end. This does the entire conversion all at
once.

How about an approach that has vc_handle_mmio() handle
MMIO_DECODE_FAILED for missing support in the common helper until the
final patch that can do:

> +       mmio = insn_decode_mmio(insn, &bytes);
> +       if (mmio == MMIO_DECODE_FAILED)
> +               return ES_DECODE_FAILED;

...i.e. insn_decode_mmio() is finally prepared to handle all scenarios
and vc_handle_mmio_twobyte_ops() can finally be deleted. This also
helps a future bisect that finds "whoops, 'no functional changes' was
incorrect".

>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Joerg Roedel <jroedel@suse.de>

Missing Sathya signed-off-by...