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.
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.
>
> 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@xxxxxxxxxx>
>>>> ---
>>>> 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
>>>>
>>>
>>> .
>>>
>>
>>
>>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
