Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <19ef56ed7852a2097125a588b124e4598ab26e91.camel@intel.com>
Subject: Re: [PATCH v3 02/21] x86/virt/tdx: Detect TDX private KeyIDs
From:   Kai Huang <kai.huang@intel.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     seanjc@google.com, pbonzini@redhat.com, dave.hansen@intel.com,
        len.brown@intel.com, tony.luck@intel.com,
        rafael.j.wysocki@intel.com, reinette.chatre@intel.com,
        dan.j.williams@intel.com, peterz@infradead.org, ak@linux.intel.com,
        kirill.shutemov@linux.intel.com, isaku.yamahata@intel.com
Date:   Tue, 19 Apr 2022 22:07:37 +1200
In-Reply-To: <abfb95df-4f34-1663-42e8-b9d06bab3b58@linux.intel.com>
References: <cover.1649219184.git.kai.huang@intel.com>
         <2fb62e93734163d2f367fd44e3335cd8a2bf2995.1649219184.git.kai.huang@intel.com>
         <abfb95df-4f34-1663-42e8-b9d06bab3b58@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2022-04-18 at 22:42 -0700, Sathyanarayanan Kuppuswamy wrote:
> 
> On 4/5/22 9:49 PM, Kai Huang wrote:
> >   	detect_seam(c);
> > +	detect_tdx_keyids(c);
> 
> Do you want to add some return value to detect_seam() and not
> proceed if it fails?

I don't think this function should return.  However it may make sense to stop
detecting TDX KeyIDs when on some cpu SEAMRR is detected as not enabled (i.e. on
BSP when SEAMRR is not enabled by BIOS, or on any AP when there's BIOS bug that
BIOS doesn't configure SEAMRR consistently on all cpus).  The reason is TDX
KeyIDs can only be accessed by software runs in SEAM mode.  So if SEAMRR
configuration is broken, TDX KeyID configuration probably is broken too.

However detect_tdx_keyids() essentially only uses rdmsr_safe() to read some MSR,
so if there's any problem, rdmsr_safe() will catch it.  And SEAMRR is always
checked before doing any TDX related staff later, therefore in practice there
will be no problem.  But anyway I guess there's no harm to add additional SEAMRR
check in detect_tdx_keyids().  I'll think more on this.  Thanks.

> 
> In case if this function is going to be extended by future
> patch set, maybe do the same for detect_tdx_keyids()?
> 

I'd prefer to leaving this in current way until there's a real need.

-- 
Thanks,
-Kai