Message-ID: <20240508115957.1c13dd12.alex.williamson@redhat.com>
Date: Wed, 8 May 2024 11:59:57 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: liulongfang <liulongfang@...wei.com>
Cc: <jgg@...dia.com>, <shameerali.kolothum.thodi@...wei.com>,
 <jonathan.cameron@...wei.com>, <kvm@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linuxarm@...neuler.org>
Subject: Re: [PATCH v6 2/5] hisi_acc_vfio_pci: modify the register location
 of the XQC address

On Wed, 8 May 2024 15:18:55 +0800
liulongfang <liulongfang@...wei.com> wrote:

> On 2024/5/7 20:35, Alex Williamson wrote:
> > On Tue, 7 May 2024 16:29:05 +0800
> > liulongfang <liulongfang@...wei.com> wrote:
> >   
> >> On 2024/5/4 0:11, Alex Williamson wrote:  
> >>> On Thu, 25 Apr 2024 21:23:19 +0800
> >>> Longfang Liu <liulongfang@...wei.com> wrote:
> >>>     
> >>>> According to the latest hardware register specification. The DMA
> >>>> addresses of EQE and AEQE are not at the front of their respective
> >>>> register groups, but start from the second.
> >>>> So, previously fetching the value starting from the first register
> >>>> would result in an incorrect address.
> >>>>
> >>>> Therefore, the register location from which the address is obtained
> >>>> needs to be modified.    
> >>>
> >>> How does this affect migration?  Has it ever worked?  Does this make    
> >>
> >> The general HiSilicon accelerator task will only use SQE and CQE.
> >> EQE is only used when user running kernel mode task and uses interrupt mode.
> >> AEQE is only used when user running task exceptions occur and software reset
> >> is required.
> >>
> >> The DMA addresses of these four queues are written to the device by the device
> >> driver through the mailbox command during driver initialization.
> >> The DMA addresses of EQE and AEQE are migrated through the device register.
> >>
> >> EQE and AEQE are not used in general task, after the live migration is completed,
> >> this DMA address error will not be found. until we added a new kernel-mode test case
> >> that we discovered that this address was abnormal.
> >>  
> >>> the migration data incompatible?
> >>>    
> >>
> >> This address only affects the kernel mode interrupt mode task function and device
> >> exception recovery function.
> >> They do not affect live migration functionality  
> > 
> > Then why are we migrating them?  Especially EQE, if it is only used by
> > kernel mode drivers then why does the migration protocol have any
> > business transferring the value from the source device?  It seems the
> > fix should be not to apply the value from the source and mark these as
> > reserved fields in the migration data stream.  Thanks,
> >  
> 
> HiSilicon accelerator equipment can perform general services after completing live migration.
> This kind of business is executed through the user mode driver and only needs to use SQE and CQE.
> 
> At the same time, this device can also perform kernel-mode services in the VM through the crypto
> subsystem. This kind of service requires the use of EQE.
> 
> Finally, if the device is abnormal, the driver needs to perform a device reset, and AEQE needs to
> be used in this case.
> 
> Therefore, a complete device live migration function needs to ensure that device functions are
> normal in all these scenarios.
> Therefore, this data still needs to be migrated.

Ok, I had jumped to an in-kernel host driver in reference to "kernel
mode" rather than a guest kernel.  Migrating with bad data only affects
the current configuration of the device, reloading a guest driver to
update these registers or a reset of the device would allow proper
operation of the device, correct?

But I think this still isn't really a complete solution, we know
there's a bug in the migration data stream, so not only would we fix
the data stream, but I think we should also take measures to prevent
loading a known bad data stream.  AIUI migration of this device while
running in kernel mode (ie. a kernel driver within a guest VM) is
broken.  Therefore, the least we can do in a new kernel, knowing that
there was previously a bug in the migration data stream, is to fail to
load that migration data because it risks this scenario where the
device is broken after migration.  Shouldn't we then also increment a
migration version field in the data stream to block migrations that
risk this breakage, or barring that, change the magic data field to
prevent the migration?  Thanks,

Alex

Powered by blists - more mailing lists