On 11/9/23 03:55, Kai Huang wrote:
> +static bool is_pamt_page(unsigned long phys)
> +{
> + struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> + int i;
> +
> + /*
> + * This function is called from #MC handler, and theoretically
> + * it could run in parallel with the TDX module initialization
> + * on other logical cpus. But it's not OK to hold mutex here
> + * so just blindly check module status to make sure PAMTs/TDMRs
> + * are stable to access.
> + *
> + * This may return inaccurate result in rare cases, e.g., when
> + * #MC happens on a PAMT page during module initialization, but
> + * this is fine as #MC handler doesn't need a 100% accurate
> + * result.
> + */
It doesn't need perfect accuracy. But how do we know it's not going to
go, for instance, chase a bad pointer?
> + if (tdx_module_status != TDX_MODULE_INITIALIZED)
> + return false;
As an example, what prevents this CPU from observing
tdx_module_status==TDX_MODULE_INITIALIZED while the PAMT structure is
being assembled?
> + for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++) {
> + unsigned long base, size;
> +
> + tdmr_get_pamt(tdmr_entry(tdmr_list, i), &base, &size);
> +
> + if (phys >= base && phys < (base + size))
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Return whether the memory page at the given physical address is TDX
> + * private memory or not. Called from #MC handler do_machine_check().
> + *
> + * Note this function may not return an accurate result in rare cases.
> + * This is fine as the #MC handler doesn't need a 100% accurate result,
> + * because it cannot distinguish #MC between software bug and real
> + * hardware error anyway.
> + */
> +bool tdx_is_private_mem(unsigned long phys)
> +{
> + struct tdx_module_args args = {
> + .rcx = phys & PAGE_MASK,
> + };
> + u64 sret;
> +
> + if (!platform_tdx_enabled())
> + return false;
> +
> + /* Get page type from the TDX module */
> + sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
> + /*
> + * Handle the case that CPU isn't in VMX operation.
> + *
> + * KVM guarantees no VM is running (thus no TDX guest)
> + * when there's any online CPU isn't in VMX operation.
> + * This means there will be no TDX guest private memory
> + * and Secure-EPT pages. However the TDX module may have
> + * been initialized and the memory page could be PAMT.
> + */
> + if (sret == TDX_SEAMCALL_UD)
> + return is_pamt_page(phys);
Either this is comment is wonky or the module initialization is buggy.
config_global_keyid() goes and does SEAMCALLs on all CPUs. There are
zero checks or special handling in there for whether the CPU has done
VMXON. So, by the time we've started initializing the TDX module
(including the PAMT), all online CPUs must be able to do SEAMCALLs. Right?
So how can we have a working PAMT here when this CPU can't do SEAMCALLs?
I don't think we should even bother with this complexity. I think we
can just fix the whole thing by saying that unless you can make a
non-init SEAMCALL, we just assume the memory can't be private.
The transition to being able to make non-init SEAMCALLs is also #MC safe
*and* it's at a point when the tdmr_list is stable.
Can anyone shoot any holes in that? :)
On Fri, 2023-12-01 at 12:35 -0800, Hansen, Dave wrote:
> On 11/9/23 03:55, Kai Huang wrote:
> > +static bool is_pamt_page(unsigned long phys)
> > +{
> > + struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> > + int i;
> > +
> > + /*
> > + * This function is called from #MC handler, and theoretically
> > + * it could run in parallel with the TDX module initialization
> > + * on other logical cpus. But it's not OK to hold mutex here
> > + * so just blindly check module status to make sure PAMTs/TDMRs
> > + * are stable to access.
> > + *
> > + * This may return inaccurate result in rare cases, e.g., when
> > + * #MC happens on a PAMT page during module initialization, but
> > + * this is fine as #MC handler doesn't need a 100% accurate
> > + * result.
> > + */
>
> It doesn't need perfect accuracy. But how do we know it's not going to
> go, for instance, chase a bad pointer?
>
> > + if (tdx_module_status != TDX_MODULE_INITIALIZED)
> > + return false;
>
> As an example, what prevents this CPU from observing
> tdx_module_status==TDX_MODULE_INITIALIZED while the PAMT structure is
> being assembled?
There are two types of memory order serializing operations between assembling
the TDMR/PAMT structure and setting the tdx_module_status to
TDX_MODULE_INITIALIZED: 1) wbvind_on_all_cpus(); 2) bunch of SEAMCALLs;
WBINVD is a serializing instruction. SEAMCALL is a VMEXIT to the TDX module,
which involves GDT/LDT/control registers/MSRs switch so it is also a serializing
operation.
But perhaps we can explicitly add a smp_wmb() between assembling TDMR/PAMT
structure and setting tdx_module_status if that's better.
>
> > + for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++) {
> > + unsigned long base, size;
> > +
> > + tdmr_get_pamt(tdmr_entry(tdmr_list, i), &base, &size);
> > +
> > + if (phys >= base && phys < (base + size))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * Return whether the memory page at the given physical address is TDX
> > + * private memory or not. Called from #MC handler do_machine_check().
> > + *
> > + * Note this function may not return an accurate result in rare cases.
> > + * This is fine as the #MC handler doesn't need a 100% accurate result,
> > + * because it cannot distinguish #MC between software bug and real
> > + * hardware error anyway.
> > + */
> > +bool tdx_is_private_mem(unsigned long phys)
> > +{
> > + struct tdx_module_args args = {
> > + .rcx = phys & PAGE_MASK,
> > + };
> > + u64 sret;
> > +
> > + if (!platform_tdx_enabled())
> > + return false;
> > +
> > + /* Get page type from the TDX module */
> > + sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
> > + /*
> > + * Handle the case that CPU isn't in VMX operation.
> > + *
> > + * KVM guarantees no VM is running (thus no TDX guest)
> > + * when there's any online CPU isn't in VMX operation.
> > + * This means there will be no TDX guest private memory
> > + * and Secure-EPT pages. However the TDX module may have
> > + * been initialized and the memory page could be PAMT.
> > + */
> > + if (sret == TDX_SEAMCALL_UD)
> > + return is_pamt_page(phys);
>
> Either this is comment is wonky or the module initialization is buggy.
>
> config_global_keyid() goes and does SEAMCALLs on all CPUs. There are
> zero checks or special handling in there for whether the CPU has done
> VMXON. So, by the time we've started initializing the TDX module
> (including the PAMT), all online CPUs must be able to do SEAMCALLs. Right?
>
> So how can we have a working PAMT here when this CPU can't do SEAMCALLs?
The corner case is KVM can enable VMX on all cpus, initialize the TDX module,
and then disable VMX on all cpus. One example is KVM can be unloaded after it
initializes the TDX module.
In this case CPU cannot do SEAMCALL but PAMTs are already working :-)
However if SEAMCALL cannot be made (due to out of VMX), then the module can only
be initialized or the initialization hasn't been tried, so both
tdx_module_status and the tdx_tdmr_list are stable to access.
>
> I don't think we should even bother with this complexity. I think we
> can just fix the whole thing by saying that unless you can make a
> non-init SEAMCALL, we just assume the memory can't be private.
>
> The transition to being able to make non-init SEAMCALLs is also #MC safe
> *and* it's at a point when the tdmr_list is stable.
>
> Can anyone shoot any holes in that? :)
On 12/3/23 03:44, Huang, Kai wrote:
...
>> It doesn't need perfect accuracy. But how do we know it's not going to
>> go, for instance, chase a bad pointer?
>>
>>> + if (tdx_module_status != TDX_MODULE_INITIALIZED)
>>> + return false;
>>
>> As an example, what prevents this CPU from observing
>> tdx_module_status==TDX_MODULE_INITIALIZED while the PAMT structure is
>> being assembled?
>
> There are two types of memory order serializing operations between assembling
> the TDMR/PAMT structure and setting the tdx_module_status to
> TDX_MODULE_INITIALIZED: 1) wbvind_on_all_cpus(); 2) bunch of SEAMCALLs;
>
> WBINVD is a serializing instruction. SEAMCALL is a VMEXIT to the TDX module,
> which involves GDT/LDT/control registers/MSRs switch so it is also a serializing
> operation.
>
> But perhaps we can explicitly add a smp_wmb() between assembling TDMR/PAMT
> structure and setting tdx_module_status if that's better.
... and there's zero documentation of this dependency because ... ?
I suspect it's because it was never looked at until Tony made a comment
about it and we started looking at it. In other words, it worked by
coincidence.
>>> + for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++) {
>>> + unsigned long base, size;
>>> +
>>> + tdmr_get_pamt(tdmr_entry(tdmr_list, i), &base, &size);
>>> +
>>> + if (phys >= base && phys < (base + size))
>>> + return true;
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +/*
>>> + * Return whether the memory page at the given physical address is TDX
>>> + * private memory or not. Called from #MC handler do_machine_check().
>>> + *
>>> + * Note this function may not return an accurate result in rare cases.
>>> + * This is fine as the #MC handler doesn't need a 100% accurate result,
>>> + * because it cannot distinguish #MC between software bug and real
>>> + * hardware error anyway.
>>> + */
>>> +bool tdx_is_private_mem(unsigned long phys)
>>> +{
>>> + struct tdx_module_args args = {
>>> + .rcx = phys & PAGE_MASK,
>>> + };
>>> + u64 sret;
>>> +
>>> + if (!platform_tdx_enabled())
>>> + return false;
>>> +
>>> + /* Get page type from the TDX module */
>>> + sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
>>> + /*
>>> + * Handle the case that CPU isn't in VMX operation.
>>> + *
>>> + * KVM guarantees no VM is running (thus no TDX guest)
>>> + * when there's any online CPU isn't in VMX operation.
>>> + * This means there will be no TDX guest private memory
>>> + * and Secure-EPT pages. However the TDX module may have
>>> + * been initialized and the memory page could be PAMT.
>>> + */
>>> + if (sret == TDX_SEAMCALL_UD)
>>> + return is_pamt_page(phys);
>>
>> Either this is comment is wonky or the module initialization is buggy.
>>
>> config_global_keyid() goes and does SEAMCALLs on all CPUs. There are
>> zero checks or special handling in there for whether the CPU has done
>> VMXON. So, by the time we've started initializing the TDX module
>> (including the PAMT), all online CPUs must be able to do SEAMCALLs. Right?
>>
>> So how can we have a working PAMT here when this CPU can't do SEAMCALLs?
>
> The corner case is KVM can enable VMX on all cpus, initialize the TDX module,
> and then disable VMX on all cpus. One example is KVM can be unloaded after it
> initializes the TDX module.
>
> In this case CPU cannot do SEAMCALL but PAMTs are already working :-)
>
> However if SEAMCALL cannot be made (due to out of VMX), then the module can only
> be initialized or the initialization hasn't been tried, so both
> tdx_module_status and the tdx_tdmr_list are stable to access.
None of this even matters. Let's remind ourselves how unbelievably
unlikely this is:
1. You're on an affected system that has the erratum
2. The KVM module gets unloaded, runs vmxoff
3. A kernel bug using a very rare partial write corrupts the PAMT
4. A second bug reads the PAMT consuming poison, #MC is generated
5. Enter #MC handler, SEAMCALL fails
6. #MC handler just reports a plain hardware error
The only thing even remotely wrong with this situation is that the
report won't pin the #MC on TDX. Play stupid games (removing modules),
win stupid prizes (worse error message).
Can we dynamically mark a module as unsafe to remove? If so, I'd
happily just say that we should make kvm_intel.ko unsafe to remove when
TDX is supported and move on with life.
tl;dr: I think even looking a #MC on the PAMT after the kvm module is
removed is a fool's errand.
On Mon, 2023-12-04 at 09:07 -0800, Dave Hansen wrote:
> On 12/3/23 03:44, Huang, Kai wrote:
> ...
> > > It doesn't need perfect accuracy. But how do we know it's not going to
> > > go, for instance, chase a bad pointer?
> > >
> > > > + if (tdx_module_status != TDX_MODULE_INITIALIZED)
> > > > + return false;
> > >
> > > As an example, what prevents this CPU from observing
> > > tdx_module_status==TDX_MODULE_INITIALIZED while the PAMT structure is
> > > being assembled?
> >
> > There are two types of memory order serializing operations between assembling
> > the TDMR/PAMT structure and setting the tdx_module_status to
> > TDX_MODULE_INITIALIZED: 1) wbvind_on_all_cpus(); 2) bunch of SEAMCALLs;
> >
> > WBINVD is a serializing instruction. SEAMCALL is a VMEXIT to the TDX module,
> > which involves GDT/LDT/control registers/MSRs switch so it is also a serializing
> > operation.
> >
> > But perhaps we can explicitly add a smp_wmb() between assembling TDMR/PAMT
> > structure and setting tdx_module_status if that's better.
>
> ... and there's zero documentation of this dependency because ... ?
>
> I suspect it's because it was never looked at until Tony made a comment
> about it and we started looking at it. In other words, it worked by
> coincidence.
I should have put a comment around here. My bad.
Kirill also helped to look at the code.
>
> > > > + for (i = 0; i < tdmr_list->nr_consumed_tdmrs; i++) {
> > > > + unsigned long base, size;
> > > > +
> > > > + tdmr_get_pamt(tdmr_entry(tdmr_list, i), &base, &size);
> > > > +
> > > > + if (phys >= base && phys < (base + size))
> > > > + return true;
> > > > + }
> > > > +
> > > > + return false;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Return whether the memory page at the given physical address is TDX
> > > > + * private memory or not. Called from #MC handler do_machine_check().
> > > > + *
> > > > + * Note this function may not return an accurate result in rare cases.
> > > > + * This is fine as the #MC handler doesn't need a 100% accurate result,
> > > > + * because it cannot distinguish #MC between software bug and real
> > > > + * hardware error anyway.
> > > > + */
> > > > +bool tdx_is_private_mem(unsigned long phys)
> > > > +{
> > > > + struct tdx_module_args args = {
> > > > + .rcx = phys & PAGE_MASK,
> > > > + };
> > > > + u64 sret;
> > > > +
> > > > + if (!platform_tdx_enabled())
> > > > + return false;
> > > > +
> > > > + /* Get page type from the TDX module */
> > > > + sret = __seamcall_ret(TDH_PHYMEM_PAGE_RDMD, &args);
> > > > + /*
> > > > + * Handle the case that CPU isn't in VMX operation.
> > > > + *
> > > > + * KVM guarantees no VM is running (thus no TDX guest)
> > > > + * when there's any online CPU isn't in VMX operation.
> > > > + * This means there will be no TDX guest private memory
> > > > + * and Secure-EPT pages. However the TDX module may have
> > > > + * been initialized and the memory page could be PAMT.
> > > > + */
> > > > + if (sret == TDX_SEAMCALL_UD)
> > > > + return is_pamt_page(phys);
> > >
> > > Either this is comment is wonky or the module initialization is buggy.
> > >
> > > config_global_keyid() goes and does SEAMCALLs on all CPUs. There are
> > > zero checks or special handling in there for whether the CPU has done
> > > VMXON. So, by the time we've started initializing the TDX module
> > > (including the PAMT), all online CPUs must be able to do SEAMCALLs. Right?
> > >
> > > So how can we have a working PAMT here when this CPU can't do SEAMCALLs?
> >
> > The corner case is KVM can enable VMX on all cpus, initialize the TDX module,
> > and then disable VMX on all cpus. One example is KVM can be unloaded after it
> > initializes the TDX module.
> >
> > In this case CPU cannot do SEAMCALL but PAMTs are already working :-)
> >
> > However if SEAMCALL cannot be made (due to out of VMX), then the module can only
> > be initialized or the initialization hasn't been tried, so both
> > tdx_module_status and the tdx_tdmr_list are stable to access.
>
> None of this even matters. Let's remind ourselves how unbelievably
> unlikely this is:
>
> 1. You're on an affected system that has the erratum
> 2. The KVM module gets unloaded, runs vmxoff
> 3. A kernel bug using a very rare partial write corrupts the PAMT
> 4. A second bug reads the PAMT consuming poison, #MC is generated
> 5. Enter #MC handler, SEAMCALL fails
> 6. #MC handler just reports a plain hardware error
Yes totally agree it is very unlikely to happen.
>
> The only thing even remotely wrong with this situation is that the
> report won't pin the #MC on TDX. Play stupid games (removing modules),
> win stupid prizes (worse error message).
>
> Can we dynamically mark a module as unsafe to remove? If so, I'd
> happily just say that we should make kvm_intel.ko unsafe to remove when
> TDX is supported and move on with life.
>
> tl;dr: I think even looking a #MC on the PAMT after the kvm module is
> removed is a fool's errand.
Sorry I wasn't clear enough. KVM actually turns off VMX when it destroys the
last VM, so the KVM module doesn't need to be removed to turn off VMX. I used
"KVM can be unloaded" as an example to explain the PAMT can be working when VMX
is off.
On 12/4/23 13:00, Huang, Kai wrote:
>> tl;dr: I think even looking a #MC on the PAMT after the kvm module is
>> removed is a fool's errand.
> Sorry I wasn't clear enough. KVM actually turns off VMX when it destroys the
> last VM, so the KVM module doesn't need to be removed to turn off VMX. I used
> "KVM can be unloaded" as an example to explain the PAMT can be working when VMX
> is off.
Can't we just fix this by having KVM do an "extra" hardware_enable_all()
before initializing the TDX module? It's not wrong to say that TDX is a
KVM user. If KVm wants 'kvm_usage_count' to go back to 0, it can shut
down the TDX module. Then there's no PAMT to worry about.
The shutdown would be something like:
1. TDX module shutdown
2. Deallocate/Convert PAMT
3. vmxoff
Then, no SEAMCALL failure because of vmxoff can cause a PAMT-induced #MC
to be missed.
On Mon, 2023-12-04 at 14:04 -0800, Hansen, Dave wrote:
> On 12/4/23 13:00, Huang, Kai wrote:
> > > tl;dr: I think even looking a #MC on the PAMT after the kvm module is
> > > removed is a fool's errand.
> > Sorry I wasn't clear enough. KVM actually turns off VMX when it destroys the
> > last VM, so the KVM module doesn't need to be removed to turn off VMX. I used
> > "KVM can be unloaded" as an example to explain the PAMT can be working when VMX
> > is off.
>
> Can't we just fix this by having KVM do an "extra" hardware_enable_all()
> before initializing the TDX module?
>
Yes KVM needs to do hardware_enable_all() anyway before initializing the TDX
module.
I believe you mean we can keep VMX enabled after initializing the TDX module,
i.e., not calling hardware_disable_all() after that, so that kvm_usage_count
will remain non-zero even when last VM is destroyed?
The current behaviour that KVM only enable VMX when there's active VM is because
it (or the kernel) wants to allow to be able to load and run third-party VMX
module (yes the virtual BOX) when KVM module is loaded. Only one of them can
actually use the VMX hardware but they can be both loaded.
In ancient time KVM used to immediately enable VMX when it is loaded, but later
it was changed to only enable VMX when there's active VM because of the above
reason.
See commit 10474ae8945ce ("KVM: Activate Virtualization On Demand").
> It's not wrong to say that TDX is a
> KVM user. If KVm wants 'kvm_usage_count' to go back to 0, it can shut
> down the TDX module. Then there's no PAMT to worry about.
>
> The shutdown would be something like:
>
> 1. TDX module shutdown
> 2. Deallocate/Convert PAMT
> 3. vmxoff
>
> Then, no SEAMCALL failure because of vmxoff can cause a PAMT-induced #MC
> to be missed.
The limitation is once the TDX module is shutdown, it cannot be initialized
again unless it is runtimely updated.
Long-termly, if we go this design then there might be other problems when other
kernel components are using TDX. For example, the VT-d driver will need to be
changed to support TDX-IO, and it will need to enable TDX module much earlier
than KVM to do some initialization. It might need to some TDX work (e.g.,
cleanup) while KVM is unloaded. I am not super familiar with TDX-IO but looks
we might have some problem here if we go with such design.
On 12/4/23 15:24, Huang, Kai wrote:
> On Mon, 2023-12-04 at 14:04 -0800, Hansen, Dave wrote:
...
> In ancient time KVM used to immediately enable VMX when it is loaded, but later
> it was changed to only enable VMX when there's active VM because of the above
> reason.
>
> See commit 10474ae8945ce ("KVM: Activate Virtualization On Demand").
Fine. This doesn't need to change ... until you load TDX. Once you
initialize the TDX module, no more out-of-tree VMMs for you.
That doesn't seem too insane. This is yet *ANOTHER* reason that doing
dynamic TDX module initialization is a good idea.
>> It's not wrong to say that TDX is a
>> KVM user. If KVm wants 'kvm_usage_count' to go back to 0, it can shut
>> down the TDX module. Then there's no PAMT to worry about.
>>
>> The shutdown would be something like:
>>
>> 1. TDX module shutdown
>> 2. Deallocate/Convert PAMT
>> 3. vmxoff
>>
>> Then, no SEAMCALL failure because of vmxoff can cause a PAMT-induced #MC
>> to be missed.
>
> The limitation is once the TDX module is shutdown, it cannot be initialized
> again unless it is runtimely updated.
>
> Long-termly, if we go this design then there might be other problems when other
> kernel components are using TDX. For example, the VT-d driver will need to be
> changed to support TDX-IO, and it will need to enable TDX module much earlier
> than KVM to do some initialization. It might need to some TDX work (e.g.,
> cleanup) while KVM is unloaded. I am not super familiar with TDX-IO but looks
> we might have some problem here if we go with such design.
The burden for who does vmxon will simply need to change from KVM itself
to some common code that KVM depends on. Probably not dissimilar to
those nutty (sorry folks, just calling it as I see 'em) multi-KVM module
patches that are floating around.
On Mon, 2023-12-04 at 23:24 +0000, Huang, Kai wrote:
> Long-termly, if we go this design then there might be other problems when other
> kernel components are using TDX. For example, the VT-d driver will need to be
> changed to support TDX-IO, and it will need to enable TDX module much earlier
> than KVM to do some initialization. It might need to some TDX work (e.g.,
> cleanup) while KVM is unloaded. I am not super familiar with TDX-IO but looks
> we might have some problem here if we go with such design.
Perhaps I shouldn't use the future feature as argument, e.g., with multiple TDX
users we are likely to have a refcount to see whether we can truly shutdown TDX.
And VMX on/off will also need to be moved out of KVM for these work.
But the point is it's better to not assume how these kernel components will use
VMX on/off. E.g., it may just choose to simply turn on VMX, do SEMACALL, and
then turn off VMX immediately. While the TDX module will be alive all the time.
Keeping VMX on will suppress INIT, I guess that's another reason we prefer to
turning VMX on when needed.
/*
* Disable virtualization, i.e. VMX or SVM, to ensure INIT is recognized during
* reboot. VMX blocks INIT if the CPU is post-VMXON, and SVM blocks INIT if
* GIF=0, i.e. if the crash occurred between CLGI and STGI.
*/
void cpu_emergency_disable_virtualization(void)
{
...
}
On Mon, 2023-12-04 at 15:39 -0800, Dave Hansen wrote:
> On 12/4/23 15:24, Huang, Kai wrote:
> > On Mon, 2023-12-04 at 14:04 -0800, Hansen, Dave wrote:
> ...
> > In ancient time KVM used to immediately enable VMX when it is loaded, but later
> > it was changed to only enable VMX when there's active VM because of the above
> > reason.
> >
> > See commit 10474ae8945ce ("KVM: Activate Virtualization On Demand").
>
> Fine. This doesn't need to change ... until you load TDX. Once you
> initialize the TDX module, no more out-of-tree VMMs for you.
>
> That doesn't seem too insane. This is yet *ANOTHER* reason that doing
> dynamic TDX module initialization is a good idea.
I don't have objection to this.
>
> > > It's not wrong to say that TDX is a
> > > KVM user. If KVm wants 'kvm_usage_count' to go back to 0, it can shut
> > > down the TDX module. Then there's no PAMT to worry about.
> > >
> > > The shutdown would be something like:
> > >
> > > 1. TDX module shutdown
> > > 2. Deallocate/Convert PAMT
> > > 3. vmxoff
> > >
> > > Then, no SEAMCALL failure because of vmxoff can cause a PAMT-induced #MC
> > > to be missed.
> >
> > The limitation is once the TDX module is shutdown, it cannot be initialized
> > again unless it is runtimely updated.
> >
> > Long-termly, if we go this design then there might be other problems when other
> > kernel components are using TDX. For example, the VT-d driver will need to be
> > changed to support TDX-IO, and it will need to enable TDX module much earlier
> > than KVM to do some initialization. It might need to some TDX work (e.g.,
> > cleanup) while KVM is unloaded. I am not super familiar with TDX-IO but looks
> > we might have some problem here if we go with such design.
>
> The burden for who does vmxon will simply need to change from KVM itself
> to some common code that KVM depends on. Probably not dissimilar to
> those nutty (sorry folks, just calling it as I see 'em) multi-KVM module
> patches that are floating around.
>
Right we will need to move VMX on/off out of KVM for that purpose. I think the
point is it's better to not assume how these kernel components will use VMX
on/off. E.g., it may just choose to simply turn on VMX, do SEMACALL, and
then turn off VMX immediately, while the TDX module will be alive all the time.
Or we require they all need to: 1) enable VMX; 2) enable/use TDX; 3) disable TDX
when no need; 4) disable VMX.
But I don't have strong opinion here too.
On Mon, Dec 04, 2023, Dave Hansen wrote:
> On 12/4/23 15:24, Huang, Kai wrote:
> > On Mon, 2023-12-04 at 14:04 -0800, Hansen, Dave wrote:
> ...
> > In ancient time KVM used to immediately enable VMX when it is loaded, but later
> > it was changed to only enable VMX when there's active VM because of the above
> > reason.
> >
> > See commit 10474ae8945ce ("KVM: Activate Virtualization On Demand").
Huh, I always just assumed it was some backwards thinking about enabling VMX/SVM
being "dangerous" or something.
> Fine. This doesn't need to change ... until you load TDX. Once you
> initialize the TDX module, no more out-of-tree VMMs for you.
It's not just out-of-tree hypervisors, which IMO should be little more than an
afterthought. The other more important issue is that being post-VMXON blocks INIT,
i.e. VMX needs to be disabled before reboot, suspend, etc. Forcing kvm_usage_count
would work, but it would essentially turn "graceful" reboots, i.e. reboots where
the host isn't running VMs and thus VMX is already disabled. Having VMX be enabled
so long as KVM is loaded would turn all reboots into the "oh crap, the system is
rebooting, quick do VMXOFF!" variety.
> That doesn't seem too insane. This is yet *ANOTHER* reason that doing
> dynamic TDX module initialization is a good idea.
>
> >> It's not wrong to say that TDX is a KVM user. If KVm wants
> >> 'kvm_usage_count' to go back to 0, it can shut down the TDX module. Then
> >> there's no PAMT to worry about.
> >>
> >> The shutdown would be something like:
> >>
> >> 1. TDX module shutdown
> >> 2. Deallocate/Convert PAMT
> >> 3. vmxoff
> >>
> >> Then, no SEAMCALL failure because of vmxoff can cause a PAMT-induced #MC
> >> to be missed.
> >
> > The limitation is once the TDX module is shutdown, it cannot be initialized
> > again unless it is runtimely updated.
> >
> > Long-termly, if we go this design then there might be other problems when other
> > kernel components are using TDX. For example, the VT-d driver will need to be
> > changed to support TDX-IO, and it will need to enable TDX module much earlier
> > than KVM to do some initialization. It might need to some TDX work (e.g.,
> > cleanup) while KVM is unloaded. I am not super familiar with TDX-IO but looks
> > we might have some problem here if we go with such design.
>
> The burden for who does vmxon will simply need to change from KVM itself
> to some common code that KVM depends on. Probably not dissimilar to
> those nutty (sorry folks, just calling it as I see 'em) multi-KVM module
You misspelled "amazing" ;-)
> patches that are floating around.
Joking aside, why shove TDX module ownership into KVM? It honestly sounds like
a terrible fit, even without the whole TDX-IO mess. KVM state is largely ephemeral,
in the sense that loading and unloading kvm.ko doesn't allocate/free much memory
or do all that much initialization or teardown.
TDX on the other hand is quite different. IIRC the PAMT is hundreds of MiB, maybe
over a GiB in most expected use cases? And also IIRC, TDH.SYS.INIT is rather
long running operation, blocks IRQs, NMIs, (SMIs?), etc.
So rather than shove TDX ownership into KVM and force KVM to figure out how to
manage the TDX module, why not do what us nutty people are suggesting and move
hardware enabling and TDX-module management into a dedicated base module (bonus
points if you call it vac.ko ;-) ).
Alternatively, we could have a dedicated kernel module for TDX, e.g. tdx.ko, and
then have tdx.ko and kvm.ko depend on vac.ko. But I think that ends up being
quite gross and unnecessary, e.g. in such a setup, kvm-intel.ko ideally wouldn't
take a hard dependency on tdx.ko, as auto-loading tdx.ko would defeat some of the
purpose of the split, and KVM shouldn't fail to load just because TDX isn't supported.
But that'd mean conditionally doing request_module("tdx") or whatever and would
create other conundrums.
(Oof, typing that out made me realize that KVM depends on the PSP driver if
CONFIG_KVM_AMD_SEV=y, even if if the platform owner has no intention of ever using
SEV/SEV-ES. IIUC, it works because sp_mod_init() just registers a driver, i.e.
doesn't fail out of there's no PSP. That's kinda gross).
Anyways, vac.ko provides an API to grab a reference to the TDX module, e.g. the
"create a VM" API gets extended to say "create a VM of the TDX variety", and then
vac.ko manages its refcounts to VMX and TDX accordingly. And KVM obviously keeps
its existing behavior of getting and putting references for each VM.
That way userspace gets to decide when to (un)load tdx.ko without needing to add
a KVM module param or whatever to allow forcefully unloading tdx.ko (which would
be bizarre and probably quite difficult to implement correctly), and unloading
kvm-intel.ko wouldn't require unloading the TDX module.
The end behavior might not be all that different in the short term, but it would
give us more options, e.g. for this erratum, it would be quite easy for vac.ko to
let usersepace choose between keeping VMX "on" (while the TDX module is loaded)
and potentially having imperfect #MC messages.
And out-of-tree hypervisors could even use vac.ko's exported APIs to manage hardware
enabling if they so choose.
On 12/4/23 18:04, Sean Christopherson wrote:
> Joking aside, why shove TDX module ownership into KVM? It honestly sounds like
> a terrible fit, even without the whole TDX-IO mess. KVM state is largely ephemeral,
> in the sense that loading and unloading kvm.ko doesn't allocate/free much memory
> or do all that much initialization or teardown.
Yeah, you have a good point there. We really do need some core code to
manage VMXON/OFF now that there is increased interest outside of
_purely_ running VMs.
For the purposes of _this_ patch, I think I'm happy to leave open the
possibility that SEAMCALL can simply fail due to VMXOFF. For now, it
means that we can't attribute #MC's to the PAMT unless a VM is running
but that seems like a reasonable compromise for the moment.
Once TDX gains the ability to "pin" VMXON, the added precision here will
be appreciated.
>> Fine. This doesn't need to change ... until you load TDX. Once you
>> initialize the TDX module, no more out-of-tree VMMs for you.
>
> It's not just out-of-tree hypervisors, which IMO should be little more than an
> afterthought. The other more important issue is that being post-VMXON blocks INIT,
Does that make CPU offline a one-way process? Linux uses INIT to bring a CPU back
online again.
-Tony
On Tue, Dec 05, 2023, Dave Hansen wrote:
> On 12/4/23 18:04, Sean Christopherson wrote:
> > Joking aside, why shove TDX module ownership into KVM? It honestly sounds like
> > a terrible fit, even without the whole TDX-IO mess. KVM state is largely ephemeral,
> > in the sense that loading and unloading kvm.ko doesn't allocate/free much memory
> > or do all that much initialization or teardown.
>
> Yeah, you have a good point there. We really do need some core code to
> manage VMXON/OFF now that there is increased interest outside of
> _purely_ running VMs.
>
> For the purposes of _this_ patch, I think I'm happy to leave open the
> possibility that SEAMCALL can simply fail due to VMXOFF. For now, it
> means that we can't attribute #MC's to the PAMT unless a VM is running
> but that seems like a reasonable compromise for the moment.
+1
> Once TDX gains the ability to "pin" VMXON, the added precision here will
> be appreciated.
On Tue, Dec 05, 2023, Tony Luck wrote:
> >> Fine. This doesn't need to change ... until you load TDX. Once you
> >> initialize the TDX module, no more out-of-tree VMMs for you.
> >
> > It's not just out-of-tree hypervisors, which IMO should be little more than an
> > afterthought. The other more important issue is that being post-VMXON blocks INIT,
>
> Does that make CPU offline a one-way process? Linux uses INIT to bring a CPU back
> online again.
No, KVM does VMXOFF on the CPU being offlined, and then VMXON if/when the CPU is
onlined again. This also handles secondary CPUs for suspend/resume (the primary
CPU hooks .suspend() and .resume()).
static int kvm_offline_cpu(unsigned int cpu)
{
mutex_lock(&kvm_lock);
if (kvm_usage_count)
hardware_disable_nolock(NULL);
mutex_unlock(&kvm_lock);
return 0;
}
static int kvm_online_cpu(unsigned int cpu)
{
int ret = 0;
/*
* Abort the CPU online process if hardware virtualization cannot
* be enabled. Otherwise running VMs would encounter unrecoverable
* errors when scheduled to this CPU.
*/
mutex_lock(&kvm_lock);
if (kvm_usage_count)
ret = __hardware_enable_nolock();
mutex_unlock(&kvm_lock);
return ret;
}