Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 2 Mar 2022 16:42:45 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        luto@kernel.org, peterz@infradead.org,
        sathyanarayanan.kuppuswamy@linux.intel.com, aarcange@redhat.com,
        ak@linux.intel.com, dan.j.williams@intel.com, david@redhat.com,
        hpa@zytor.com, jgross@suse.com, jmattson@google.com,
        joro@8bytes.org, jpoimboe@redhat.com, knsathya@kernel.org,
        pbonzini@redhat.com, sdeep@vmware.com, seanjc@google.com,
        tony.luck@intel.com, vkuznets@redhat.com, wanpengli@tencent.com,
        thomas.lendacky@amd.com, brijesh.singh@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCHv4 11/30] x86/tdx: Handle in-kernel MMIO
Message-ID: <20220302134245.paq2dcsdql77rz4y@black.fi.intel.com>
References: <20220224155630.52734-1-kirill.shutemov@linux.intel.com>
 <20220224155630.52734-12-kirill.shutemov@linux.intel.com>
 <af7df79f-02b8-3025-c9a3-929b7bdd33e0@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <af7df79f-02b8-3025-c9a3-929b7bdd33e0@intel.com>
Precedence: bulk

On Thu, Feb 24, 2022 at 12:11:54PM -0800, Dave Hansen wrote:
> 
> I found a few things lacking in that description.  How's that for a rewrite?

Looks great, thanks.
> > == Patching TDX drivers ==
> > 
> > Rather than touching the entire kernel, it might also be possible to
> > just go after drivers that use MMIO in TDX guests.  Right now, that's
> > limited only to virtio and some x86-specific drivers.
> > 
> > All virtio MMIO appears to be done through a single function, which
> > makes virtio eminently easy to patch. This will be implemented in the
> > future, removing the bulk of MMIO #VEs.
> 
> Given what is written here, this sounds like a great solution especially
> compared to all the instruction decoding nasiness.  What's wrong with it?

This will not cover non-virtio users. So #VE-based MMIO will remain as
fallback mechanism.

> > +	switch (mmio) {
> > +	case MMIO_WRITE:
> > +		memcpy(&val, reg, size);
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_WRITE_IMM:
> > +		val = insn.immediate.value;
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_READ:
> > +	case MMIO_READ_ZERO_EXTEND:
> > +	case MMIO_READ_SIGN_EXTEND:
> > +		break;
> > +	case MMIO_MOVS:
> > +	case MMIO_DECODE_FAILED:
> > +		return false;
> > +	default:
> > +		BUG();
> > +	}
> 
> Given the huge description above, it's borderline criminal to not
> discuss what could led to this BUG().

This bug actually covers "Unknown insn_decode_mmio() decode value" case.
I will add the comment.


> It could literally be some minor tweak in the compiler that changed a
> non-io.h-using MMIO access to get converted over to a instruction that
> can't be decoded.
> 
> Could we spend a few lines of comments to help out the future poor sod
> that sees "kernel bug at foo.c:1234"?  Maybe:
> 
> 	/*
> 	 * MMIO was accessed with an instruction that could not
> 	 * be decoded.  It was likely not using io.h helpers or
> 	 * accessed MMIO accidentally.
> 	 */

Thanks, I will use the comment for MMIO_DECODE_FAILED handling.

-- 
 Kirill A. Shutemov