virt_mshv_vtl: Returning appropriate values for various CPUIDs. #749
Conversation
| if (result.level_number() != super::CPUID_LEAF_B_LEVEL_NUMBER_SMT) | ||
| || (result.level_type() != super::CPUID_LEAF_B_LEVEL_TYPE_SMT) | ||
| { | ||
| tracing::trace!("Topology not found!"); |
There was a problem hiding this comment.
Are these error scenarios? Is emitting a trace sufficient? Can the message at least contain more detail on what's missing and why that's a problem? Note that traces won't be emitted during normal operation, only during specific debug scenarios.
There was a problem hiding this comment.
cpuid instruction execution is intercepted by the kernel. The kernel returns all 0s for this leaf and some other leaves. Hence, we get an incorrect ecx value here. Ideally this behavior should be corrected in the kernel. For basic checking purposes, made a note of the error. I will change the trace to error with more error details.
There was a problem hiding this comment.
What are the implications for the guest if the ecx is incorrect? Could it potentially cause the guest to do unexpected behavior? I'm wondering if we should fix the kernel first.
| @@ -183,6 +183,7 @@ pub struct CpuidResults { | |||
| max_extended_state: u64, | |||
| arch_support: Box<dyn CpuidArchSupport>, | |||
| vps_per_socket: u32, | |||
| max_xfd: u32, | |||
There was a problem hiding this comment.
Where is this value being used? If it's only going to be used in TDX scenarios can we do everything that needs to be done inside of process_extended_state_subleaves instead of returning it?
| @@ -65,7 +65,7 @@ trait CpuidArchInitializer { | |||
| &self, | |||
There was a problem hiding this comment.
If we're going to do this, update this comment to explain what the return value is.
Although, seeing the TODO on this method, should we make that change now to handle max_xfd instead of returning it?
There was a problem hiding this comment.
Given the TODO comment, it might be a good idea to pass CpuidResults and update the max_xfd value in it, instead of returning the value.
There was a problem hiding this comment.
Do you think there are any cpuid leaves that we currently aren't supporting that we should support, or that we are supporting and shouldn't support? Also, there's this comment:
// TODO TDX: The following aren't required from AMD. Need to double-check if
// they're required for TDX
I'm curious if these are required.
There was a problem hiding this comment.
The CPUID list needs to be evaluated. There is a different issue tracking this: #562
There was a problem hiding this comment.
Regarding the comment on what if ecx is incorrect. Since leaf 0xB is used to read the topology, invalid data received and erroring out here will lead to incorrect evaluation of vps and TD boot failure. Similar issue was observed in the topology builder while calculating the vps for the TD. As a backup, leaf 0x4 is being read over there to figure the correct number of threads and vps. (Issue #481)
Yes, this behavior for CPUID 0xB and others should be fixed in the kernel.
There was a problem hiding this comment.
Should we just be fixing this in the kernel then instead of in usermode?
| if (result.level_number() != super::CPUID_LEAF_B_LEVEL_NUMBER_SMT) | ||
| || (result.level_type() != super::CPUID_LEAF_B_LEVEL_TYPE_SMT) | ||
| { | ||
| tracing::trace!("Topology not found!"); |
There was a problem hiding this comment.
What are the implications for the guest if the ecx is incorrect? Could it potentially cause the guest to do unexpected behavior? I'm wondering if we should fix the kernel first.
While accessing MSRs MSR_XFD(0x1C4) and MSR_XFD_ERR(0x1C5) the calculated max_xfd value would have been used. Since, these two MSRs are in the allowed MSRs read list, the L1VMM won't receive an intercept for them and the value won't be used. Hence, removing this change.
vibhutet
force-pushed
the
cpuid_correctness
branch
from
1e7f4a2 to
413b338
Compare
Addresses issue #556.