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

[Kunkun Jiang]

Hi Eric,

On 2021/7/6 21:52, Eric Auger wrote:
> Hi,
>
> On 7/6/21 10:18 AM, Kunkun Jiang wrote:
> > Hi Eric,
> >
> > On 2021/6/30 17:16, Eric Auger wrote:
> > > 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.
> > Sorry for late reply.
> >
> > Can we distinguish from the 'RunState'?
> > When we stop the VM, the RunState will be set. There are many types of
> > RunState, such as RUN_STATE_FINISH_MIGRATE/RUN_STATE_SAVE_VM/
> > RUN_STATE_SHUTDOWN/RUN_STATE_GUEST_PANICKED, etc.
> >
> > Maybe RUN_STATE_SHUTDOWN doesn't belong to save/restore use case,
> > right?
> Adding Dave, Juan and Peter in the loop for migration expertise.
>
> At the moment we save the ARM ITS MSI controller tables whenever the VM
> gets stopped. Saving the tables from KVM caches into the guest RAM is
> needed for migration and save/restore use cases.
> However with GICv4 this fails at KVM level because some MSIs are
> forwarded and saving their state is not supported with GICv4.
>
> While GICv4 migration is not supported we would like the VM to work
> properly, ie. being stoppable without taking care of table saving.
>
> So could we be more precise and identifiy the save/restore and migration
> use cases instead of saving the tables on each VM shutdown.
>
> The tables are saved into guest RAM so when need the CPUs and devices to
> be stopped but we need the guest RAM to be saved after the ITS save
> operation.
>
> Kunkun, by the way you currently just get an error from qemu, ie. qemu
> does not exit? Couldn't we just ignore -EACCESS error?
Sorry for late reply. There is something wrong with my mailbox,
so I didn't receive your email...

Yeah, just get an error. I just wanted to know if you have a good
way to solve it. It's not causing any other problems, so ignoring
it should be OK.

Thanks,
Kunkun Jiang
> Thanks
>
> Eric
>
>
>
>
> > > > 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?
> > It's not going to return -EACCES here, if we unset the forwarding
> > first. But
> > this may cause problems in save/restore scenarios. The GICv4 architecture
> > doesn't give any guarantee that the pending state is written into the
> > pending table.
> >
> > Thanks,
> > Kunkun Jiang
> > > 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
> > > > > .
> > > .
>
> .