Re: [PATCH v6 2/5] hisi_acc_vfio_pci: modify the register location of the XQC address

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 7 May 2024 16:29:05 +0800
liulongfang <liulongfang@huawei.com> wrote:

> On 2024/5/4 0:11, Alex Williamson wrote:
> > On Thu, 25 Apr 2024 21:23:19 +0800
> > Longfang Liu <liulongfang@huawei.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,

Alex

 
> > Fixes: ???
> >   
> 
> OK!
> 
> Thanks.
> Longfang.
> >> Signed-off-by: Longfang Liu <liulongfang@huawei.com>
> >> ---
> >>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 8 ++++----
> >>  drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 3 +++
> >>  2 files changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> >> index 45351be8e270..0c7e31076ff4 100644
> >> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> >> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> >> @@ -516,12 +516,12 @@ static int vf_qm_state_save(struct hisi_acc_vf_core_device *hisi_acc_vdev,
> >>  		return -EINVAL;
> >>  
> >>  	/* Every reg is 32 bit, the dma address is 64 bit. */
> >> -	vf_data->eqe_dma = vf_data->qm_eqc_dw[1];
> >> +	vf_data->eqe_dma = vf_data->qm_eqc_dw[QM_XQC_ADDR_HIGH];
> >>  	vf_data->eqe_dma <<= QM_XQC_ADDR_OFFSET;
> >> -	vf_data->eqe_dma |= vf_data->qm_eqc_dw[0];
> >> -	vf_data->aeqe_dma = vf_data->qm_aeqc_dw[1];
> >> +	vf_data->eqe_dma |= vf_data->qm_eqc_dw[QM_XQC_ADDR_LOW];
> >> +	vf_data->aeqe_dma = vf_data->qm_aeqc_dw[QM_XQC_ADDR_HIGH];
> >>  	vf_data->aeqe_dma <<= QM_XQC_ADDR_OFFSET;
> >> -	vf_data->aeqe_dma |= vf_data->qm_aeqc_dw[0];
> >> +	vf_data->aeqe_dma |= vf_data->qm_aeqc_dw[QM_XQC_ADDR_LOW];
> >>  
> >>  	/* Through SQC_BT/CQC_BT to get sqc and cqc address */
> >>  	ret = qm_get_sqc(vf_qm, &vf_data->sqc_dma);
> >> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> >> index 5bab46602fad..f887ab98581c 100644
> >> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> >> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
> >> @@ -38,6 +38,9 @@
> >>  #define QM_REG_ADDR_OFFSET	0x0004
> >>  
> >>  #define QM_XQC_ADDR_OFFSET	32U
> >> +#define QM_XQC_ADDR_LOW	0x1
> >> +#define QM_XQC_ADDR_HIGH	0x2
> >> +
> >>  #define QM_VF_AEQ_INT_MASK	0x0004
> >>  #define QM_VF_EQ_INT_MASK	0x000c
> >>  #define QM_IFC_INT_SOURCE_V	0x0020  
> > 
> > .
> >   
>