On 28/02/2018 19:06, KarimAllah Ahmed wrote:
> ... to avoid having a stale value when handling an EPT misconfig for MMIO
> regions.
>
> MMIO regions that are not passed-through to the guest are handled through
> EPT misconfigs. The first time a certain MMIO page is touched it causes an
> EPT violation, then KVM marks the EPT entry to cause an EPT misconfig
> instead. Any subsequent accesses to the entry will generate an EPT
> misconfig.
>
> Things gets slightly complicated with nested guest handling for MMIO
> regions that are not passed through from L0 (i.e. emulated by L0
> user-space).
>
> An EPT violation for one of these MMIO regions from L2, exits to L0
> hypervisor. L0 would then look at the EPT12 mapping for L1 hypervisor and
> realize it is not present (or not sufficient to serve the request). Then L0
> injects an EPT violation to L1. L1 would then update its EPT mappings. The
> EXIT_QUALIFICATION value for L1 would come from exit_qualification variable
> in "struct vcpu". The problem is that this variable is only updated on EPT
> violation and not on EPT misconfig. So if an EPT violation because of a
> read happened first, then an EPT misconfig because of a write happened
> afterwards. The L0 hypervisor will still contain exit_qualification value
> from the previous read instead of the write and end up injecting an EPT
> violation to the L1 hypervisor with an out of date EXIT_QUALIFICATION.
>
> The EPT violation that is injected from L0 to L1 needs to have the correct
> EXIT_QUALIFICATION specially for the access bits because the individual
> access bits for MMIO EPTs are updated only on actual access of this
> specific type. So for the example above, the L1 hypervisor will keep
> updating only the read bit in the EPT then resume the L2 guest. The L2
> guest would end up causing another exit where the L0 *again* will inject
> another EPT violation to L1 hypervisor with *again* an out of date
> exit_qualification which indicates a read and not a write. Then this
> ping-pong just keeps happening without making any forward progress.
>
> The behavior of mapping MMIO regions changed in:
>
> commit a340b3e229b24 ("kvm: Map PFN-type memory regions as writable (if
> possible)")
>
> ... where an EPT violation for a read would also fixup the write bits to
> avoid another EPT violation which by acciddent would fix the bug mentioned
> above.
>
> This commit fixes this situation and ensures that the access bits for the
> exit_qualifcation is up to date. That ensures that even L1 hypervisor
> running with a KVM version before the commit mentioned above would still
> work.
>
> ( The description above assumes EPT to be available and used by L1
> hypervisor + the L1 hypervisor is passing through the MMIO region to the L2
> guest while this MMIO region is emulated by the L0 user-space ).
This looks okay. Would it be possible to add a kvm-unit-tests testcase
for this?
Thanks,
Paolo
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Radim Krčmář <rkrc...@redhat.com>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Ingo Molnar <mi...@redhat.com>
> Cc: H. Peter Anvin <h...@zytor.com>
> Cc: x...@kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: KarimAllah Ahmed <karah...@amazon.de>
> ---
> arch/x86/kvm/paging_tmpl.h | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 5abae72..6288e9d 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -452,14 +452,21 @@ static int FNAME(walk_addr_generic)(struct guest_walker
> *walker,
> * done by is_rsvd_bits_set() above.
> *
> * We set up the value of exit_qualification to inject:
> - * [2:0] - Derive from [2:0] of real exit_qualification at EPT violation
> + * [2:0] - Derive from the access bits. The exit_qualification might be
> + * out of date if it is serving an EPT misconfiguration.
> * [5:3] - Calculated by the page walk of the guest EPT page tables
> * [7:8] - Derived from [7:8] of real exit_qualification
> *
> * The other bits are set to 0.
> */
> if (!(errcode & PFERR_RSVD_MASK)) {
> - vcpu->arch.exit_qualification &= 0x187;
> + vcpu->arch.exit_qualification &= 0x180;
> + if (write_fault)
> + vcpu->arch.exit_qualification |=
> EPT_VIOLATION_ACC_WRITE;
> + if (user_fault)
> + vcpu->arch.exit_qualification |= EPT_VIOLATION_ACC_READ;
> + if (fetch_fault)
> + vcpu->arch.exit_qualification |=
> EPT_VIOLATION_ACC_INSTR;
> vcpu->arch.exit_qualification |= (pte_access & 0x7) << 3;
> }
> #endif
>
