On Sun, May 07 2017 at 2:00:30 pm BST, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On Sat, May 06 2017 at 4:24:35 pm BST, Eric Auger <eric.auger@xxxxxxxxxx> wrote:
>> Introduce new attributes in KVM_DEV_ARM_VGIC_GRP_CTRL group:
>> - KVM_DEV_ARM_ITS_SAVE_TABLES: saves the ITS tables into guest RAM
>> - KVM_DEV_ARM_ITS_RESTORE_TABLES: restores them into VGIC internal
>> structures.
>>
>> We hold the vcpus lock during the save and restore to make
>> sure no vcpu is running.
>>
>> At this stage the functionality is not yet implemented. Only
>> the skeleton is put in place.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>> v6 -> v7:
>> - also hold the its_lock on save and restore
>>
>> v5 -> v6:
>> - remove the pending table sync from the ITS table restore
>>
>> v4 -> v5:
>> - use KVM_DEV_ARM_ITS_SAVE_TABLES and KVM_DEV_ARM_ITS_RESTORE_TABLES
>> - rename *flush* into *save*
>> - call its_sync_lpi_pending_table at the end of restore
>> - use abi framework
>>
>> v3 -> v4:
>> - pass kvm struct handle to vgic_its_flush/restore_pending_tables
>> - take the kvm lock and vcpu locks
>> - ABI revision check
>> - check attr->attr is null
>>
>> v1 -> v2:
>> - remove useless kvm parameter
>> ---
>> arch/arm/include/uapi/asm/kvm.h | 4 +-
>> arch/arm64/include/uapi/asm/kvm.h | 4 +-
>> virt/kvm/arm/vgic/vgic-its.c | 107 ++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 109 insertions(+), 6 deletions(-)
>>
>
> [...]
>
>> @@ -1718,7 +1777,37 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>> */
>> static int vgic_its_restore_tables_v0(struct vgic_its *its)
>> {
>> - return -ENXIO;
>> + struct kvm *kvm = its->dev->kvm;
>> + int ret;
>> +
>> + mutex_lock(&kvm->lock);
>> + mutex_lock(&its->its_lock);
>> +
>> + if (!lock_all_vcpus(kvm)) {
>> + mutex_unlock(&its->its_lock);
>> + mutex_unlock(&kvm->lock);
>> + return -EBUSY;
>> + }
>> +
>> + ret = vgic_its_restore_collection_table(its);
>> + if (ret)
>> + goto out;
>> +
>> + ret = vgic_its_restore_device_tables(its);
>> +
>> +out:
>> + unlock_all_vcpus(kvm);
>> + mutex_unlock(&its->its_lock);
>> + mutex_unlock(&kvm->lock);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * On restore path, MSI injections can happen before the
>> + * first VCPU run so let's complete the GIC init here.
>> + */
>> + return kvm_vgic_map_resources(its->dev->kvm);
>
> This one is still a rather sore spot, but I've lost track of what we can
> do about it. Until now, this would only happen on first run. But it
> looks like QEMU and KVM have different views of what "runable" is.
>
> I'm not sure there is a good way to solve this, unfortunately. From a
> device PoV, everything is ready and the fact that nothing is clocking
> the CPUs is very much irrelevant. I'm almost tempted to say that the
> map_resource() call in kvm_vcpu_first_run_init shouldn't be there, and
> that we're missing a synchronization point with userspace that would say
> "system is entierely configured", triggering the iodev registration.
>
> Oh well. At that point, and short of having something better to offer:
>
> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>
> M.
Actually, there is a rather big problem here. This function is called on
a per-ITS basis. But once we have run map_resources *once*, any other
call becomes ineffective (vgic_ready() returns true). What happens to
the other ITSs? Can they still be successfully restored? Don't they
end-up with the same "blind spot" you've tried to close?
Thanks,
M.
--
Jazz is not dead, it just smell funny.
