Received-SPF: pass (google.com: domain of linux-kernel+bounces-106799-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 18 Mar 2024 16:16:56 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@intel.com>,
	"Zhang, Tina" <tina.zhang@intel.com>,
	"Yuan, Hang" <hang.yuan@intel.com>,
	"Huang, Kai" <kai.huang@intel.com>, "Chen, Bo2" <chen.bo@intel.com>,
	"sagis@google.com" <sagis@google.com>,
	"isaku.yamahata@linux.intel.com" <isaku.yamahata@linux.intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Aktas, Erdem" <erdemaktas@google.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>
Subject: Re: [PATCH v19 120/130] KVM: TDX: Add a method to ignore dirty
 logging
Message-ID: <20240318231656.GC1645738@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <1491dd247829bf1a29df1904aeed5ed6b464d29c.1708933498.git.isaku.yamahata@intel.com>
 <b4cde44a884f2f048987826d59e2054cd1fa532b.camel@intel.com>
 <20240315013511.GF1258280@ls.amr.corp.intel.com>
 <fc6278a55deeccf8c67fba818647829a1dddcf0a.camel@intel.com>
 <20240318171218.GA1645738@ls.amr.corp.intel.com>
 <6986b1ddf25f064d3609793979ca315567d7e875.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <6986b1ddf25f064d3609793979ca315567d7e875.camel@intel.com>

On Mon, Mar 18, 2024 at 05:43:33PM +0000,
"Edgecombe, Rick P" <rick.p.edgecombe@intel.com> wrote:

> On Mon, 2024-03-18 at 10:12 -0700, Isaku Yamahata wrote:
> > I categorize as follows. Unless otherwise, I'll update this series.
> > 
> > - dirty log check
> >   As we will drop this ptach, we'll have no call site.
> > 
> > - KVM_BUG_ON() in main.c
> >   We should drop them because their logic isn't complex.
> What about "KVM: TDX: Add methods to ignore guest instruction
> emulation"? Is it cleanly blocked somehow?

KVM fault handler, kvm_mmu_page_fault(), is the caller into the emulation,
It should skip the emulation.

As the second guard, x86_emulate_instruction(), calls
check_emulate_instruction() callback to check if the emulation can/should be
done.  TDX callback can return it as X86EMUL_UNHANDLEABLE.  Then, the flow goes
to user space as error.  I'll update the vt_check_emulate_instruction().


> > - KVM_BUG_ON() in tdx.c
> >   - The error check of the return value from SEAMCALL
> >     We should keep it as it's unexpected error from TDX module. When
> > we hit
> >     this, we should mark the guest bugged and prevent further
> > operation.  It's
> >     hard to deduce the reason.  TDX mdoule might be broken.
> Yes. Makes sense.
> 
> > 
> >   - Other check
> >     We should drop them.
> 
> Offhand, I'm not sure what is in this category.

- Checking error code on TD enter/exit
  I'll revise how to check error from TD enter/exit.  We'll have new code.  I
  will update wrapper function to take struct kvm_tdx or struct tdx_vcpu, and
  revise to remove random check.  Cleanups related to kvm_rebooting,
  TDX_SW_ERROR, kvm_spurious_fault()

- Remaining random check for debug.
  The examples are as follows.  They were added for debug.


@@ -797,18 +788,14 @@ void tdx_vcpu_free(struct kvm_vcpu *vcpu)
         * list_{del,add}() on associated_tdvcpus list later.
         */
        tdx_disassociate_vp_on_cpu(vcpu);
-       WARN_ON_ONCE(vcpu->cpu != -1);
 
        /*
         * This methods can be called when vcpu allocation/initialization
         * failed. So it's possible that hkid, tdvpx and tdvpr are not assigned
         * yet.
         */
-       if (is_hkid_assigned(to_kvm_tdx(vcpu->kvm))) {
-               WARN_ON_ONCE(tdx->tdvpx_pa);
-               WARN_ON_ONCE(tdx->tdvpr_pa);
+       if (is_hkid_assigned(to_kvm_tdx(vcpu->kvm)))
                return;
-       }
 
        if (tdx->tdvpx_pa) {
                for (i = 0; i < tdx_info->nr_tdvpx_pages; i++) {
@@ -831,9 +818,9 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 
        /* vcpu_deliver_init method silently discards INIT event. */
-       if (KVM_BUG_ON(init_event, vcpu->kvm))
+       if (init_event)
                return;
-       if (KVM_BUG_ON(is_td_vcpu_created(to_tdx(vcpu)), vcpu->kvm))
+       if (is_td_vcpu_created(to_tdx(vcpu)))
                return;
 
        /*
@@ -831,9 +818,9 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 
        /* vcpu_deliver_init method silently discards INIT event. */
-       if (KVM_BUG_ON(init_event, vcpu->kvm))
+       if (init_event)
                return;
-       if (KVM_BUG_ON(is_td_vcpu_created(to_tdx(vcpu)), vcpu->kvm))
+       if (is_td_vcpu_created(to_tdx(vcpu)))
                return;
 
        /*
@@ -831,9 +818,9 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 
        /* vcpu_deliver_init method silently discards INIT event. */
-       if (KVM_BUG_ON(init_event, vcpu->kvm))
+       if (init_event)
                return;
-       if (KVM_BUG_ON(is_td_vcpu_created(to_tdx(vcpu)), vcpu->kvm))
+       if (is_td_vcpu_created(to_tdx(vcpu)))
                return;
 
        /*

  
-- 
Isaku Yamahata <isaku.yamahata@intel.com>