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.
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
