DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B7624C04B948
From: Bandan Das <bsd@redhat.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Cc: David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org,
        pbonzini@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/3] KVM: nVMX: Emulate EPTP switching for the L1 hypervisor
References: <20170710204936.4001-4-bsd@redhat.com>
        <2d50ebc4-9328-ce08-b55b-6a331ee13cc3@redhat.com>
        <jpgy3ruam9i.fsf@linux.bootlegged.copy>
        <jpg1spmsugn.fsf@linux.bootlegged.copy> <20170711193235.GE3326@potion>
        <jpgd1964usz.fsf@linux.bootlegged.copy>
        <20170711202118.GC28875@potion>
        <jpgzica3e83.fsf@linux.bootlegged.copy> <20170711204521.GF3326@potion>
        <jpgeftmwujj.fsf@linux.bootlegged.copy> <20170712132445.GD3442@potion>
Date: Wed, 12 Jul 2017 14:11:14 -0400
In-Reply-To: <20170712132445.GD3442@potion> ("Radim \=\?utf-8\?B\?S3LEjW3DocWZ\?\=
 \=\?utf-8\?B\?Iidz\?\= message of "Wed,
        12 Jul 2017 15:24:45 +0200")
Message-ID: <jpgeftljzjx.fsf@linux.bootlegged.copy>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3245
Lines: 68

Radim Krčmář <rkrcmar@redhat.com> writes:
...
>> Why do you think it's a bug ?
>
> SDM defines a different behavior and hardware doesn't do that either.
> There are only two reasons for a VMFUNC VM exit from EPTP switching:
>
>  1) ECX > 0
>  2) EPTP would cause VM entry to fail if in VMCS.EPT_POINTER
>
> KVM can fail for other reasons because of its bugs, but that should be
> notified to the guest in another way.  Rebooting the guest is kind of
> acceptable in that case.
>
>>                               The eptp switching function really didn't
>> succeed as far as our emulation goes when kvm_mmu_reload() fails.
>> And as such, the generic vmexit failure event should be a vmfunc vmexit.
>
> I interpret it as two separate events -- at first, the vmfunc succeeds
> and when it later tries to access memory through the new EPTP (valid,
> but not pointing into backed memory), it results in a EPT_MISCONFIG VM
> exit.
>
>> We cannot strictly follow the spec here, the spec doesn't even mention a way
>> to emulate eptp switching.  If setting up the switching succeeded and the
>> new root pointer is invalid or whatever, I really don't care what happens
>> next but this is not the case. We fail to get a new root pointer and without
>> that, we can't even make a switch!
>
> We just make it behave exactly how the spec says that it behaves.  We do
> have a value (we read 'address') to put into VMCS.EPT_POINTER, which is
> all we need for the emulation.
> The function doesn't dereference that pointer, it just looks at its
> value to decide whether it is valid or not.  (btw. we should check that
> properly, because we cannot depend on VM entry failure pass-through like
> the normal case.)
>
> The dereference done in kvm_mmu_reload() should happen after EPTP
> switching finishes, because the spec doesn't mention a VM exit for other
> reason than invalid EPT_POINTER value.
>
>>> just keep the original bug -- we want to eventually fix it and it's no
>>> worse till then.
>> 
>> Anyway, can you please confirm again what is the behavior that you
>> are expecting if kvm_mmu_reload fails ? This would be a rarely used
>> branch and I am actually fine diverging from what I think is right if
>> I can get the reviewers to agree on a common thing.
>
> kvm_mmu_reload() fails when mmu_check_root() is false, which means that
> the pointed physical address is not backed.  We've hit this corner-case
> in the past -- Jim said that the chipset returns all 1s if a read is not
> claimed.
>
> So in theory, KVM should not fail kvm_mmu_reload(), but behave as if the
> pointer pointed to a memory of all 1s, which would likely result in
> EPT_MISCONFIG when the guest does a memory access.

As much as I would like to disagree with you, I have already spent way more
time on this then I want. Please let's just leave it here, then ? The mmu unload
will make sure there's an invalid root hpa and whatever happens next, happens.

> It is a mishandled corner case, but turning it into VM exit would only
> confuse an OS that receives the impossible VM exit and potentially
> confuse reader of the KVM logic.
>
> I think that not using kvm_mmu_reload() directly in EPTP switching is
> best.  The bug is not really something we care about.