Re: [RFC][PATCH 2/2] kvm: x86: mmio: fix setting the present bit of mmio spte

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/11/14 18:11, Paolo Bonzini wrote:
>
>
> On 14/11/2014 10:31, Tiejun Chen wrote:
>> In PAE case maxphyaddr may be 52bit as well, we also need to
>> disable mmio page fault. Here we can check MMIO_SPTE_GEN_HIGH_SHIFT
>> directly to determine if we should set the present bit, and
>> bring a little cleanup.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   arch/x86/include/asm/kvm_host.h |  1 +
>>   arch/x86/kvm/mmu.c              | 23 +++++++++++++++++++++++
>>   arch/x86/kvm/x86.c              | 30 ------------------------------
>>   3 files changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index dc932d3..667f2b6 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -809,6 +809,7 @@ void kvm_mmu_write_protect_pt_masked(struct kvm *kvm,
>>   				     struct kvm_memory_slot *slot,
>>   				     gfn_t gfn_offset, unsigned long mask);
>>   void kvm_mmu_zap_all(struct kvm *kvm);
>> +void kvm_set_mmio_spte_mask(void);
>>   void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm);
>>   unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm);
>>   void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages);
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index ac1c4de..8e4be36 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -295,6 +295,29 @@ static bool check_mmio_spte(struct kvm *kvm, u64 spte)
>>   	return likely(kvm_gen == spte_gen);
>>   }
>>
>> +/*
>> + * Set the reserved bits and the present bit of an paging-structure
>> + * entry to generate page fault with PFER.RSV = 1.
>> + */
>> +void kvm_set_mmio_spte_mask(void)
>> +{
>> +	u64 mask;
>> +	int maxphyaddr = boot_cpu_data.x86_phys_bits;
>> +
>> +	/* Mask the reserved physical address bits. */
>> +	mask = rsvd_bits(maxphyaddr, MMIO_SPTE_GEN_HIGH_SHIFT - 1);
>> +
>> +	/* Magic bits are always reserved for 32bit host. */
>> +	mask |= 0x3ull << 62;
>
> This should be enough to trigger the page fault on PAE systems.
>
> The problem is specific to non-EPT 64-bit hosts, where the PTEs have no
> reserved bits beyond 51:MAXPHYADDR.  On EPT we use WX- permissions to
> trigger EPT misconfig, on 32-bit systems we have bit 62.

Thanks for your explanation.

>
>> +	/* Set the present bit to enable mmio page fault. */
>> +	if (maxphyaddr < MMIO_SPTE_GEN_HIGH_SHIFT)
>> +	    mask = PT_PRESENT_MASK;
>
> Shouldn't this be "|=" anyway, instead of "="?
>

Yeah, just miss this. Thanks a lot, I will fix this in next revision.

Thanks
Tiejun