Re: [PATCH 3/6] svm: Add support for APIC_ACCESS_PAGE_PRIVATE_MEMSLOT setup/destroy

From: "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>
To: "Jan H. Schönherr" <jschoenh@amazon.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Cc: "joro@8bytes.org" <joro@8bytes.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"rkrcmar@redhat.com" <rkrcmar@redhat.com>
Subject: Re: [PATCH 3/6] svm: Add support for APIC_ACCESS_PAGE_PRIVATE_MEMSLOT setup/destroy
Date: Sun, 30 Jun 2019 16:19:47 +0000	[thread overview]
Message-ID: <7215044a-af35-49a6-771d-1f1a5ec72bfd@amd.com> (raw)
In-Reply-To: <5b786dde-1fc4-9abc-ae95-8360e033fb97@amazon.de>

Jan,

On 5/8/2019 2:14 PM, Jan H. Schönherr wrote:
> On 22/03/2019 12.57, Suthikulpanit, Suravee wrote:
>> Activate/deactivate AVIC requires setting/unsetting the memory region used
>> for APIC_ACCESS_PAGE_PRIVATE_MEMSLOT. So, re-factor avic_init_access_page()
>> to avic_setup_access_page() and add srcu_read_lock/unlock, which are needed
>> to allow this function to be called during run-time.
>>
>> Also, introduce avic_destroy_access_page() to unset the page when
>> deactivate AVIC.
>>
>> Signed-off-by: Suravee Suthikulpanit<suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/svm.c | 28 ++++++++++++++++++++++++++--
>>   1 file changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 4cf93a729ad8..f41f34f70dde 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1666,7 +1666,7 @@ static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
>>    * field of the VMCB. Therefore, we set up the
>>    * APIC_ACCESS_PAGE_PRIVATE_MEMSLOT (4KB) here.
>>    */
>> -static int avic_init_access_page(struct kvm_vcpu *vcpu)
>> +static int avic_setup_access_page(struct kvm_vcpu *vcpu, bool init)
>>   {
>>        struct kvm *kvm = vcpu->kvm;
>>        int ret = 0;
>> @@ -1675,10 +1675,14 @@ static int avic_init_access_page(struct kvm_vcpu *vcpu)
>>        if (kvm->arch.apic_access_page_done)
>>                goto out;
>>
>> +     if (!init)
>> +             srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
>>        ret = __x86_set_memory_region(kvm,
>>                                      APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
>>                                      APIC_DEFAULT_PHYS_BASE,
>>                                      PAGE_SIZE);
>> +     if (!init)
>> +             vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
>>        if (ret)
>>                goto out;
>>
>> @@ -1688,6 +1692,26 @@ static int avic_init_access_page(struct kvm_vcpu *vcpu)
>>        return ret;
>>   }
>>
>> +static void avic_destroy_access_page(struct kvm_vcpu *vcpu)
>> +{
>> +     struct kvm *kvm = vcpu->kvm;
>> +
>> +     mutex_lock(&kvm->slots_lock);
>> +
>> +     if (!kvm->arch.apic_access_page_done)
>> +             goto out;
>> +
>> +     srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
>> +     __x86_set_memory_region(kvm,
>> +                             APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
>> +                             APIC_DEFAULT_PHYS_BASE,
>> +                             0);
>> +     vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
> This pattern of "unlock, do something, re-lock" strikes me as odd --
> here and in the setup function.
> 
> There seem to be a few assumptions for this to work:
> a) SRCU read-side critical sections must not be nested.
> b) We must not keep any pointer to a SRCU protected structure
>     across a call to this function.
> 
> Can we guarantee these assumptions? Now and in the future (given that this is already
> a bit hidden in the call stack)?
> 
> (And if we can guarantee them, why are we holding the SRCU lock in the first place?)

The reason we need to call srcu_read_unlock() here is because the srcu_read_lock() is
called at the beginning of vcpu_run() before going into vcpu_enter_guest().

Here, since we need to __x86_set_memory_region(), which update the page table.
If we do not call srcu_read_unlock(), we end up with the following call trace:

[94617.992835] INFO: task qemu-system-x86:246799 blocked for more than 120 seconds.
[94618.001635]       Not tainted 5.1.0-avic+ #14
[94618.006934] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[94618.016114] qemu-system-x86 D    0 246799 246788 0x00000084
[94618.022773] Call Trace:
[94618.025919]  ? __schedule+0x2f9/0x860
[94618.030416]  schedule+0x32/0x70
[94618.034335]  schedule_timeout+0x1d8/0x300
[94618.039234]  ? __queue_work+0x12c/0x3b0
[94618.043938]  wait_for_completion+0xb9/0x140
[94618.049036]  ? wake_up_q+0x70/0x70
[94618.053265]  __synchronize_srcu.part.17+0x8a/0xc0
[94618.058959]  ? __bpf_trace_rcu_invoke_callback+0x10/0x10
[94618.065359]  install_new_memslots+0x56/0x90 [kvm]
[94618.071080]  __kvm_set_memory_region+0x7df/0x8c0 [kvm]
[94618.077275]  __x86_set_memory_region+0xb6/0x190 [kvm]
[94618.083347]  svm_pre_update_apicv_exec_ctrl+0x42/0x70 [kvm_amd]
[94618.090410]  kvm_make_apicv_deactivate_request+0xa4/0xd0 [kvm]
[94618.097450]  enable_irq_window+0x119/0x170 [kvm_amd]
[94618.103461]  kvm_arch_vcpu_ioctl_run+0x8bf/0x1a80 [kvm]
[94618.109774]  kvm_vcpu_ioctl+0x3ab/0x5d0 [kvm]
[94618.115119]  do_vfs_ioctl+0xa9/0x630
[94618.119593]  ksys_ioctl+0x60/0x90
[94618.123780]  __x64_sys_ioctl+0x16/0x20
[94618.128459]  do_syscall_64+0x55/0x110
[94618.133037]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[94618.139163] RIP: 0033:0x7f2a9d5478d7
[94618.143630] Code: Bad RIP value.
[94618.147707] RSP: 002b:00007f2a9ade4988 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[94618.156660] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f2a9d5478d7
[94618.165160] RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000011
[94618.173649] RBP: 00007f2a9ade4a90 R08: 0000000000000000 R09: 00000000000000ff
[94618.182142] R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000000
[94618.190641] R13: 0000000000801000 R14: 0000000000000000 R15: 00007f2a9ade5700

Regards,
Suravee