On Thu, 2022-12-15 at 21:26 -0800, Isaku Yamahata wrote:
> On Wed, Dec 14, 2022 at 11:43:14AM +0000,
> "Huang, Kai" <kai.huang@xxxxxxxxx> wrote:
>
> > On Sat, 2022-10-29 at 23:23 -0700, isaku.yamahata@xxxxxxxxx wrote:
> > > +static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> > > +{
> > > + if (is_td_vcpu(vcpu)) {
> > > + if (is_mmio)
> > > + return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> > > + return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
> > > + }
> > > +
> > > + return vmx_get_mt_mask(vcpu, gfn, is_mmio);
> > > +}
> >
> > So you are returning WB for _ALL_ guest memory, including shared. Wouldn't this
> > break MTRR handling for shared memory? For instance, IIUC we can still support
> > assigning a device to a TDX guest while the VT-d doesn't support coherent
> > memory, in which case guest's MTRR/PAT are honored. I think this should also
> > apply to TDX guest's shared memory?
>
> You're right. So here is the updated change.
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -798,11 +798,8 @@ static int vt_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)
>
> static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> {
> - if (is_td_vcpu(vcpu)) {
> - if (is_mmio)
> - return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> - return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
> - }
> + if (is_td_vcpu(vcpu))
> + return tdx_get_mt_mask(vcpu, gfn, is_mmio);
>
> return vmx_get_mt_mask(vcpu, gfn, is_mmio);
> }
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ac47b20e4e91..f1842eb32d6c 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -568,7 +568,31 @@ int tdx_vm_init(struct kvm *kvm)
> return 0;
> }
>
> +u8 tdx_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
> +{
> + if (is_mmio)
> + return MTRR_TYPE_UNCACHABLE << VMX_EPT_MT_EPTE_SHIFT;
> +
> + if (!kvm_arch_has_noncoherent_dma(vcpu->kvm))
> + return (MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT) | VMX_EPT_IPAT_BIT;
> +
> + /* Guest CR0 is unknown. Assume CR0.CD = 0. */
This comment is horrible. You need to explain why guest CR0.CD is irrelevant
here.
And the fact is for TDX guest, TDX module tells us the CR0.CD is always 0.
> +
> + /* TDX private GPA is always WB. */
> + if (gfn & kvm_gfn_shared_mask(vcpu->kvm))
> + return MTRR_TYPE_WRBACK << VMX_EPT_MT_EPTE_SHIFT;
Dose the 'gfn' still have the shared bit set? I don't think so?
> +
> + /*
> + * Because the definition of MTRR MSR is unaware of shared-bit,
> + * clear shared-bit.
> + */
> + gfn = kvm_gfn_private(vcpu->kvm, gfn);
> + return kvm_mtrr_get_guest_memory_type(vcpu, gfn) << VMX_EPT_MT_EPTE_SHIFT;
> +}
Overall, I think a better logic should be: 1) for private page, return WB (and
WARN_ON(is_mmio)), although this doesn't matter anyway as the SPTE won't be used
by CPU anyway. 2) Then for shared pages, we try to copy vmx_get_mt_mask() logic,
or use vmx_get_mt_mask() directly.
But since how KVM and TDX module virtualize MTRR/PAT is different, thus for now
I am not sure whether just using vmx_get_mt_mask() can work. For example, IIUC
for TDX guest KVM cannot know guest's PAT, but for VMX guest KVM traps MTRR/PAT
so it can know guest's memory type.
For now I am not even sure whether we can/should support device assignment w/o
VT-d snooping capability as for TDX guest KVM cannot know guest's PAT? Perhaps
we should just make sure this won't happen so we can always returns WB for
shared memory too for TDX guest.
