Re: [PATCH v3 2/4] hisi_acc_vfio_pci: Create subfunction for data reading

[liulongfang]

On 2024/3/7 16:33, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: liulongfang <liulongfang@xxxxxxxxxx>
>> Sent: Thursday, March 7, 2024 6:04 AM
>> To: alex.williamson@xxxxxxxxxx; jgg@xxxxxxxxxx; Shameerali Kolothum
>> Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; Jonathan Cameron
>> <jonathan.cameron@xxxxxxxxxx>
>> Cc: kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>> linuxarm@xxxxxxxxxxxxx; liulongfang <liulongfang@xxxxxxxxxx>
>> Subject: [PATCH v3 2/4] hisi_acc_vfio_pci: Create subfunction for data
>> reading
>>
>> During the live migration process. It needs to obtain various status
>> data of drivers and devices. In order to facilitate calling it in the
>> debugfs function. For all operations that read data from device registers,
>> the driver creates a subfunction.
>> Also fixed the location of address data.
>>
>> Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
>> ---
>>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c    | 55 ++++++++++---------
>>  .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h    |  3 +
>>  2 files changed, 33 insertions(+), 25 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..1881f3fa9266 100644
>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> @@ -486,42 +486,22 @@ static int vf_qm_load_data(struct
>> hisi_acc_vf_core_device *hisi_acc_vdev,
>>  	return 0;
>>  }
>>
>> -static int vf_qm_state_save(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>> -			    struct hisi_acc_vf_migration_file *migf)
>> +static int vf_qm_read_data(struct hisi_qm *vf_qm, struct acc_vf_data
>> *vf_data)
>>  {
>> -	struct acc_vf_data *vf_data = &migf->vf_data;
>> -	struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
>>  	struct device *dev = &vf_qm->pdev->dev;
>>  	int ret;
>>
>> -	if (unlikely(qm_wait_dev_not_ready(vf_qm))) {
>> -		/* Update state and return with match data */
>> -		vf_data->vf_qm_state = QM_NOT_READY;
>> -		hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state;
>> -		migf->total_length = QM_MATCH_SIZE;
>> -		return 0;
>> -	}
>> -
>> -	vf_data->vf_qm_state = QM_READY;
>> -	hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state;
>> -
>> -	ret = vf_qm_cache_wb(vf_qm);
>> -	if (ret) {
>> -		dev_err(dev, "failed to writeback QM Cache!\n");
>> -		return ret;
>> -	}
>> -
>>  	ret = qm_get_regs(vf_qm, vf_data);
> 
> 
> So this doesn't need the qm_wait_dev_not_ready(vf_qm) check above for the
> debugfs ? What happens if you read when device is not ready?
>

There is still a call to qm_wait_dev_not_ready() in vf_qm_state_save.
Here is just the data reading operation of the new vf_qm_read_data() function.


>>  	if (ret)
>>  		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];
> 
> Also since there is no serialization with core migration now, what will be the data
> returned in debugfs when there is a vf_qm_load_data() is in progress?
>> So I guess the intention or assumption here is that the debugfs data is only
> valid when the user knows that devices not under migration process.
> 
> Is that right?
>

The data read by debugfs is the data in the device when the read operation occurs.
As for whether the device is in the running state or the resuming state,
debugfs does not need to pay attention to it.

If the user wants to read the status data of the device in different states,
he can first query the device's state, and then read the save data based on
the state result.

Thanks,
Longfang.

> Thanks,
> Shameer
> 
>>  	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);
>> @@ -536,6 +516,31 @@ static int vf_qm_state_save(struct
>> hisi_acc_vf_core_device *hisi_acc_vdev,
>>  		return -EINVAL;
>>  	}
>>
>> +	return 0;
>> +}
>> +
>> +static int vf_qm_state_save(struct hisi_acc_vf_core_device *hisi_acc_vdev,
>> +			    struct hisi_acc_vf_migration_file *migf)
>> +{
>> +	struct acc_vf_data *vf_data = &migf->vf_data;
>> +	struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm;
>> +	int ret;
>> +
>> +	if (unlikely(qm_wait_dev_not_ready(vf_qm))) {
>> +		/* Update state and return with match data */
>> +		vf_data->vf_qm_state = QM_NOT_READY;
>> +		hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state;
>> +		migf->total_length = QM_MATCH_SIZE;
>> +		return 0;
>> +	}
>> +
>> +	vf_data->vf_qm_state = QM_READY;
>> +	hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state;
>> +
>> +	ret = vf_qm_read_data(vf_qm, vf_data);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>>  	migf->total_length = sizeof(struct acc_vf_data);
>>  	return 0;
>>  }
>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
>> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
>> index 5bab46602fad..7a9dc87627cd 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
>> --
>> 2.24.0
> 
> .
>