linux-kernel - Re: [PATCH v13 22/22] Documentation/x86: Add documentation for TDX host support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <862ad257bc87efbbd73f886a24b0e589a1c2be4c.camel@intel.com>
Date:   Wed, 13 Sep 2023 11:27:19 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "Raj, Ashok" <ashok.raj@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "david@...hat.com" <david@...hat.com>,
        "bagasdotme@...il.com" <bagasdotme@...il.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "ak@...ux.intel.com" <ak@...ux.intel.com>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>,
        "Chatre, Reinette" <reinette.chatre@...el.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "imammedo@...hat.com" <imammedo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "Gao, Chao" <chao.gao@...el.com>,
        "Brown, Len" <len.brown@...el.com>,
        "sathyanarayanan.kuppuswamy@...ux.intel.com" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        "Huang, Ying" <ying.huang@...el.com>,
        "Williams, Dan J" <dan.j.williams@...el.com>,
        "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v13 22/22] Documentation/x86: Add documentation for TDX
 host support

Thanks for reviewing the doc!

> > +Making SEAMCALL requires the CPU already being in VMX operation (VMXON
> > +has been done).  For now both tdx_enable() and tdx_cpu_enable() don't
> > +handle VMXON internally, but depends on the caller to guarantee that.
> Isn't this an implementation detail. It's fine mentioning that TDX 
> requires VMX being enabled but whether it's being handled by current 
> code or not is an implementation details. I think this is better left as 
> a comment in the code rather than in the doc, it will likely quickly go 
> stale.

Both above, and ...
> 
> > +
> > +To enable TDX, the caller of TDX should: 1) hold read lock of CPU hotplug
> 
> nit: Hold CPU hotplug lock in read mode. And again, this is more of an 
> implementation details, the important bit is that cpu hotplug must be 
> blocked while enabling is in progress, no?

... this are used to explain ...
> 
> > +lock; 2) do VMXON and tdx_enable_cpu() on all online cpus successfully;
> > +3) call tdx_enable().  For example::
> > +
> > +        cpus_read_lock();
> > +        on_each_cpu(vmxon_and_tdx_cpu_enable());
> > +        ret = tdx_enable();
> > +        cpus_read_unlock();
> > +        if (ret)
> > +                goto no_tdx;
> > +        // TDX is ready to use

... the pseudo code here, with the purpose to give caller example on how to use.

However I also agree we should make the doc concise as nobody wants to read
boring and lengthy materials.  I personally don't know how to draw the line, so
I can remove all those if that better.  But before I commit to do let's wait for
some more time to hear from others.

> > +
> > +And the caller of TDX must guarantee the tdx_cpu_enable() has been
> > +successfully done on any cpu before it wants to run any other SEAMCALL.
> > +A typical usage is do both VMXON and tdx_cpu_enable() in CPU hotplug
> > +online callback, and refuse to online if tdx_cpu_enable() fails.
> > +
> > +User can consult dmesg to see whether the TDX module has been initialized.
> > +
> > +If the TDX module is initialized successfully, dmesg shows something
> > +like below::
> > +
> > +  [..] tdx: TDX module: attributes 0x0, vendor_id 0x8086, major_version 1, minor_version 0, build_date 20211209, build_num 160
> > +  [..] tdx: 262668 KBs allocated for PAMT.
> > +  [..] tdx: module initialized.
> > +
> > +If the TDX module failed to initialize, dmesg also shows it failed to
> > +initialize::
> > +
> > +  [..] tdx: module initialization failed ...
> 
> nit: You give specific strings which tdx is going to use, of course 
> those can change and will go stale here. Instead, perhaps just 
> mentioning that the dmesg is going to be contains a message signifying 
> error or success.
> > +

Somehow I believe this would be useful, as this message isn't just for developer
(like the pseudo code above), but for the user.  Of course w/o explicitly saying
here, the user should also be able to easily find this particular sentence
(because it's too obvious), but I guess providing it here would make it even
easier for the user.  Also, I am not expecting this particular message to be
changed soon or frequently.

Again, I am quite open to all of these, but let's some more time :-)