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;
MIME-Version: 1.0
References: <cover.1649219184.git.kai.huang@intel.com> <3f19ac995d184e52107e7117a82376cb7ecb35e7.1649219184.git.kai.huang@intel.com>
 <b3c81b7f-3016-8f4e-3ac5-bff1fc52a879@intel.com> <345753e50e4c113b1dfb71bba1ed841eee55aed3.camel@intel.com>
In-Reply-To: <345753e50e4c113b1dfb71bba1ed841eee55aed3.camel@intel.com>
From:   Sagi Shahar <sagis@google.com>
Date:   Wed, 18 May 2022 09:19:43 -0700
Message-ID: <CAAhR5DFFGTHAG9U74v9YXZkjfgfQ9vD4B76ky-MtM5fkjTgRFQ@mail.gmail.com>
Subject: Re: [PATCH v3 06/21] x86/virt/tdx: Shut down TDX module in case of error
To:     Kai Huang <kai.huang@intel.com>
Cc:     Dave Hansen <dave.hansen@intel.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.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,
        sathyanarayanan.kuppuswamy@linux.intel.com,
        "Yamahata, Isaku" <isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 26, 2022 at 5:06 PM Kai Huang <kai.huang@intel.com> wrote:
>
> On Tue, 2022-04-26 at 13:59 -0700, Dave Hansen wrote:
> > On 4/5/22 21:49, Kai Huang wrote:
> > > TDX supports shutting down the TDX module at any time during its
> > > lifetime.  After TDX module is shut down, no further SEAMCALL can be
> > > made on any logical cpu.
> >
> > Is this strictly true?
> >
> > I thought SEAMCALLs were used for the P-SEAMLDR too.
>
> Sorry will change to no TDX module SEAMCALL can be made on any logical cpu.
>
> [...]
>
> > >
> > > +/* Data structure to make SEAMCALL on multiple CPUs concurrently */
> > > +struct seamcall_ctx {
> > > +   u64 fn;
> > > +   u64 rcx;
> > > +   u64 rdx;
> > > +   u64 r8;
> > > +   u64 r9;
> > > +   atomic_t err;
> > > +   u64 seamcall_ret;
> > > +   struct tdx_module_output out;
> > > +};
> > > +
> > > +static void seamcall_smp_call_function(void *data)
> > > +{
> > > +   struct seamcall_ctx *sc = data;
> > > +   int ret;
> > > +
> > > +   ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9,
> > > +                   &sc->seamcall_ret, &sc->out);

Are the seamcall_ret and out fields in seamcall_ctx going to be used?
Right now it looks like no one is going to read them.
If they are going to be used then this is going to cause a race since
the different CPUs are going to write concurrently to the same address
inside seamcall().
We should either use local memory and write using atomic_set like the
case for the err field or hard code NULL at the call site if they are
not going to be used.

> > > +   if (ret)
> > > +           atomic_set(&sc->err, ret);
> > > +}
> > > +
> > > +/*
> > > + * Call the SEAMCALL on all online cpus concurrently.
> > > + * Return error if SEAMCALL fails on any cpu.
> > > + */
> > > +static int seamcall_on_each_cpu(struct seamcall_ctx *sc)
> > > +{
> > > +   on_each_cpu(seamcall_smp_call_function, sc, true);
> > > +   return atomic_read(&sc->err);
> > > +}
> >
> > Why bother returning something that's not read?
>
> It's not needed.  I'll make it void.
>
> Caller can check seamcall_ctx::err directly if they want to know whether any
> error happened.
>
>
>
> --
> Thanks,
> -Kai
>
>

Sagi