Re: [question] Shall we flush ITS tables into guest RAM when shutdown the VM?

[Eric Auger <eric.auger@redhat.com>]

On 6/30/21 3:38 AM, Kunkun Jiang wrote:
> On 2021/6/30 4:14, Eric Auger wrote:
>> Hi Kunkun,
>>
>> On 6/29/21 11:33 AM, Kunkun Jiang wrote:
>>> Hi all,
>>>
>>> Accroding to the patch cddafd8f353d2d251b1a5c6c948a577a85838582,
>>> our original intention is to flush the ITS tables into guest RAM at
>>> the point
>>> RUN_STATE_FINISH_MIGRATE, but sometimes the VM gets stopped before
>>> migration launch so let's simply flush the tables each time the VM gets
>>> stopped.
>>>
>>> But I encountered an error when I shut down the virtual machine.
>>>
>>>> qemu-system-aarch64: KVM_SET_DEVICE_ATTR failed: Group 4 attr
>>>> 0x0000000000000001: Permission denied
>>> Shall we need to flush ITS tables into guest RAM when 'shutdown' the
>>> VM?
>>> Or do you think this error is normal?
>> yes we determined in the past this was the right moment do save the
>> tables
>>
>> "with a live migration the guest is still running after
>> the RAM has been first saved, and so the tables may still change
>> during the iterative RAM save. You would actually need to do this
>> at just the point we stop the VM before the final RAM save; that *might*
>> be possible by using a notifier hook a vm run state change to
>> RUN_STATE_FINISH_MIGRATE
>> - if you can do the changes just as the migration flips into that mode
>> it *should* work. " said David.
>>
>> But sometimes as the commit msg says, the VM is stopped before the
>> migration launch - I do not remember the exact scenario tbh -.
> Well, I initially wanted to know more about this scenario to determine
> whether
> a normal shutdown would fall into it.😂
I think it was for save/restore use case. In that case you need to flush
the KVM cache in memory on VM shutdown.
> In my opinion, when the virtual machine is normally shutdown, flushing
> the
> ITS tables is not necessary. If we can't tell the difference between
> 'normal shutdown'
> and the above scenario, then this 'error' is inevitable.
>> So each time the VM is stopped we flush the caches into guest RAM.
>>
>>
>>
>>> This error occurs in the following scenario:
>>> Kunpeng 920 、enable GICv4、passthrough a accelerator Hisilicon SEC to
>>> the VM.
>>>
>>> The flow is as follows:
>>>
>>> QEMU:
>>> vm_shutdown
>>>      do_vm_stop(RUN_STATE_SHUTDOWN)
>>>          vm_state_notify
>>>              ...
>>>              vm_change_state_handler (hw/intc/arm_gicv3_its_kvm.c)
>>>                  kvm_device_access
>>>
>>> Kernel:
>>>      vgic_its_save_tables_v0
>>>          vgic_its_save_device_tables
>>>              vgic_its_save_itt
>>>
>>> There is such a code in vgic_its_save_itt():
>>>> /*
>>>>   * If an LPI carries the HW bit, this means that this
>>>>   * interrupt is controlled by GICv4, and we do not
>>>>   * have direct access to that state without GICv4.1.
>>>>   * Let's simply fail the save operation...
>>>>   */
>>>> if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)
>>>>            return -EACCES;
Maybe we miss a piece of code for 4.0 that unsets the forwarding. The
only way to handle this is to make sure  ite->irq->hw is not set on
shutdown, no?

Thanks

Eric
>> As far as I understand you need a v4.1 to migrate,
>> following Shenming's series
>> [PATCH v5 0/6] KVM: arm64: Add VLPI migration support on GICv4.1
>> Maybe sync with him?
> Yes, GICv4 does not support live migrate.
>
> Thanks,
> Kunkun Jiang
>>
>> Thanks
>>
>> Eric
>>
>>
>>> Looking forward to your reply.
>>>
>>> Thanks,
>>> Kunkun Jiang
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>> .
>
>