Re: [RFC PATCH 2/3] kvm: arm/arm64: vgic-vits: free its resource when vm reboot/reset

[wanghaibin <wanghaibin.wang@huawei.com>]

On 2017/9/11 2:46, Auger Eric wrote:

> Hi Wanghaibin,
> 
> On 07/09/2017 13:28, Auger Eric wrote:
>> Hi Wanghaibin,
>>
>> On 07/09/2017 03:32, wanghaibin wrote:
>>> On 2017/9/7 0:20, Auger Eric wrote:
>>>
>>>> Hi,
>>>>
>>>> On 06/09/2017 15:05, wanghaibin wrote:
>>>>> This patch fix the migrate save tables failure.
>>>>>
>>>>> When the virtual machine is in booting and the devices haven't initialized,
>>>>> the all virtual dte/ite may be invalid. If migrate at this moment, the save
>>>>> tables interface traversal device list, and check the dte is valid or not.
>>>>> if not, it will return the -EINVAL.
>>>>
>>>> The issue on save is less clear to me. We are not checking the "dte" are
>>>> valid as it is said above. We are scrolling the ITS lists - which may be
>>>> empty - and dump them in guest memory.
>>>>
>>>> On save() there are quite few checks that can cause a failure.
>>>> vgic_its_check_id() can be among them. This typically requires the
>>>> GITS_BASER to have been properly set. Failing on save looks OK to me in
>>>> such situation.
>>>>
>>>> Sorry but I don't get the purpose of this patch. Does it fix a save failure?
>>>
>>>
>>> Yes, for save, vgic_its_check_id() func will check the L1 DTE valid or not through
>>> the code like :
>>>
>>> 	/* Each 1st level entry is represented by a 64-bit value. */
>>> 	if (kvm_read_guest(its->dev->kvm,
>>> 			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
>>> 			   &indirect_ptr, sizeof(indirect_ptr)))
>>> 		return false;
>>>
>>> 	indirect_ptr = le64_to_cpu(indirect_ptr);
>>>
>>> 	/* check the valid bit of the first level entry */
>>> 	if (!(indirect_ptr & BIT_ULL(63)))
>>> 		return false;
>>>
>>> If invalid , the save will return -EINVAL caused by the vgic_its_check_id() with return the false value.
>>>
>>> And form the cover letter, the problem happened when no one pci dev has been probed( guest driver haven't any
>>> mapd, mapti), So the L1 DTEs are all invalid currently. Just like you said, at this moment migrate, we are scrolling
>>> the ITS lists, next time check_id failed and save interface failed.
>>>
>>> I think the final reason is the device list free problem, at the reset/reboot, ITS dev/clo/itt lists are not be free
>>> and set NULL. So that, the save interface failed.
>>> This patch try to free the resource when vm reboot/reset.
>> OK understood. Indeed none of the device/collection lists should be non
>> empty at that stage, ie. when GITS_BASERn have not be written yet and
>> are marked invalid.
>>
>> For solving the specific save() issue here, I think the best is to check
>> the validity bit of the GITS_BASER (col, device) and if invalid do nothing.
> 
> Actually the above proposal does not work as GITS_BASERn is not properly
> reset. Maybe the best way is to introduce an ITS KVM device reset IOTCL
> in the control group. Upon this command we could properly reset the
> requested registers and the lists.


Yes, It should free these lists when vits reset.

This patch according the has_run_once and vcpu_init to mark the vcpu reset happened,
and scrolling all kvm devices to find the vits device to free the lists.
I think it's a little odd too.

If we can add the reset IOCTL, I think it must be the best way.

Thanks.

> 
> Thanks
> 
> Eric
>>
>> Then we need to have a more global discussion about whether, when and
>> where the device and collection lists need to be freed.
>>
>> If you want I can respin with above suggestion and add the valid pointer
>> to the entry_fn_t to handle the restore path. Up to you.


All along, I want to contribute code to the community, so far, It has not been achieved.
So I would like to collect the solutions for this problem and try to fix it first, can I?

Thanks.

>>
>> Thanks
>>
>> Eric
>>
>>
>>> BTW: these lists will re-bulid when the reboot vm run the probe pci device step.
>>
>>>
>>> Thanks
>>>
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>
>>>>
>>>>>
>>>>> This patch try to free the its list resource when vm reboot or reset to avoid this.
>>>>>
>>>>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>>>>> ---
>>>>>  virt/kvm/arm/arm.c           |  5 ++++-
>>>>>  virt/kvm/arm/vgic/vgic-its.c | 10 ++++++++++
>>>>>  virt/kvm/arm/vgic/vgic.h     |  1 +
>>>>>  3 files changed, 15 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index a39a1e1..db7632d 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -46,6 +46,7 @@
>>>>>  #include <asm/kvm_coproc.h>
>>>>>  #include <asm/kvm_psci.h>
>>>>>  #include <asm/sections.h>
>>>>> +#include "vgic.h"
>>>>>  
>>>>>  #ifdef REQUIRES_VIRT
>>>>>  __asm__(".arch_extension	virt");
>>>>> @@ -901,8 +902,10 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>>>>>  	 * Ensure a rebooted VM will fault in RAM pages and detect if the
>>>>>  	 * guest MMU is turned off and flush the caches as needed.
>>>>>  	 */
>>>>> -	if (vcpu->arch.has_run_once)
>>>>> +	if (vcpu->arch.has_run_once) {
>>>>>  		stage2_unmap_vm(vcpu->kvm);
>>>>> +		vgic_its_free_resource(vcpu->kvm);
>>>>> +	}
>>>>>  
>>>>>  	vcpu_reset_hcr(vcpu);
>>>>>  
>>>>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>>>>> index 25d614f..5c20352 100644
>>>>> --- a/virt/kvm/arm/vgic/vgic-its.c
>>>>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>>>>> @@ -2467,6 +2467,16 @@ static int vgic_its_get_attr(struct kvm_device *dev,
>>>>>  	.has_attr = vgic_its_has_attr,
>>>>>  };
>>>>>  
>>>>> +void vgic_its_free_resource(struct kvm *kvm)
>>>>> +{
>>>>> +	struct kvm_device *dev, *tmp;
>>>>> +
>>>>> +	list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
>>>>> +		if(dev->ops == &kvm_arm_vgic_its_ops)
>>>>> +			vgic_its_free_list(kvm, dev->private);
>>>>> +	}
>>>>> +}
>>>>> +
>>>>>  int kvm_vgic_register_its_device(void)
>>>>>  {
>>>>>  	return kvm_register_device_ops(&kvm_arm_vgic_its_ops,
>>>>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>>>>> index c2be5b7..fbcbdfd 100644
>>>>> --- a/virt/kvm/arm/vgic/vgic.h
>>>>> +++ b/virt/kvm/arm/vgic/vgic.h
>>>>> @@ -222,5 +222,6 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>>>>>  
>>>>>  bool lock_all_vcpus(struct kvm *kvm);
>>>>>  void unlock_all_vcpus(struct kvm *kvm);
>>>>> +void vgic_its_free_resource(struct kvm *kvm);
>>>>>  
>>>>>  #endif
>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>>
> 
> .
>