On 2025/1/14 3:45, Alex Williamson wrote:
> On Mon, 13 Jan 2025 08:34:41 -0500
> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
>
>> On Mon, 13 Jan 2025 17:43:09 +0800
>> liulongfang <liulongfang@xxxxxxxxxx> wrote:
>>
>>> On 2025/1/3 6:30, Alex Williamson wrote:
>>>> On Thu, 19 Dec 2024 10:01:03 +0000
>>>> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx> wrote:
>>>>
>>>>>> -----Original Message-----
>>>>>> From: liulongfang <liulongfang@xxxxxxxxxx>
>>>>>> Sent: Thursday, December 19, 2024 9:18 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 v2 1/5] hisi_acc_vfio_pci: fix XQE dma address error
>>>>>>
>>>>>> The dma addresses of EQE and AEQE are wrong after migration and
>>>>>> results in guest kernel-mode encryption services failure.
>>>>>> Comparing the definition of hardware registers, we found that
>>>>>> there was an error when the data read from the register was
>>>>>> combined into an address. Therefore, the address combination
>>>>>> sequence needs to be corrected.
>>>>>>
>>>>>> Even after fixing the above problem, we still have an issue
>>>>>> where the Guest from an old kernel can get migrated to
>>>>>> new kernel and may result in wrong data.
>>>>>>
>>>>>> In order to ensure that the address is correct after migration,
>>>>>> if an old magic number is detected, the dma address needs to be
>>>>>> updated.
>>>>>>
>>>>>> Fixes:b0eed085903e("hisi_acc_vfio_pci: Add support for VFIO live
>>>>>> migration")
>>>>>> Signed-off-by: Longfang Liu <liulongfang@xxxxxxxxxx>
>>>>>> ---
>>>>>> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 34 +++++++++++++++----
>>>>>> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 9 ++++-
>>>>>> 2 files changed, 36 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>>>> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>>>> index 451c639299eb..8518efea3a52 100644
>>>>>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>>>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>>>>>> @@ -350,6 +350,27 @@ static int vf_qm_func_stop(struct hisi_qm *qm)
>>>>>> return hisi_qm_mb(qm, QM_MB_CMD_PAUSE_QM, 0, 0, 0);
>>>>>> }
>>>>>>
>>>>>> +static int vf_qm_magic_check(struct acc_vf_data *vf_data)
>>>>>> +{
>>>>>> + switch (vf_data->acc_magic) {
>>>>>> + case ACC_DEV_MAGIC_V2:
>>>>>> + break;
>>>>>> + case ACC_DEV_MAGIC_V1:
>>>>>> + /* Correct dma address */
>>>>>> + 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[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[QM_XQC_ADDR_LOW];
>>>>>> + break;
>>>>>> + default:
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static int vf_qm_check_match(struct hisi_acc_vf_core_device
>>>>>> *hisi_acc_vdev,
>>>>>> struct hisi_acc_vf_migration_file *migf)
>>>>>> {
>>>>>> @@ -363,7 +384,8 @@ static int vf_qm_check_match(struct
>>>>>> hisi_acc_vf_core_device *hisi_acc_vdev,
>>>>>> if (migf->total_length < QM_MATCH_SIZE || hisi_acc_vdev-
>>>>>>> match_done)
>>>>>> return 0;
>>>>>>
>>>>>> - if (vf_data->acc_magic != ACC_DEV_MAGIC) {
>>>>>> + ret = vf_qm_magic_check(vf_data);
>>>>>> + if (ret) {
>>>>>> dev_err(dev, "failed to match ACC_DEV_MAGIC\n");
>>>>>> return -EINVAL;
>>>>>> }
>>>>>> @@ -418,7 +440,7 @@ static int vf_qm_get_match_data(struct
>>>>>> hisi_acc_vf_core_device *hisi_acc_vdev,
>>>>>> int vf_id = hisi_acc_vdev->vf_id;
>>>>>> int ret;
>>>>>>
>>>>>> - vf_data->acc_magic = ACC_DEV_MAGIC;
>>>>>> + vf_data->acc_magic = ACC_DEV_MAGIC_V2;
>>>>>> /* Save device id */
>>>>>> vf_data->dev_id = hisi_acc_vdev->vf_dev->device;
>>>>>>
>>>>>> @@ -496,12 +518,12 @@ static int vf_qm_read_data(struct hisi_qm
>>>>>> *vf_qm, struct acc_vf_data *vf_data)
>>>>>> 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 245d7537b2bc..2afce68f5a34 100644
>>>>>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
>>>>>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h
>>>>>> @@ -39,6 +39,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
>>>>>> @@ -50,10 +53,14 @@
>>>>>> #define QM_EQC_DW0 0X8000
>>>>>> #define QM_AEQC_DW0 0X8020
>>>>>>
>>>>>> +enum acc_magic_num {
>>>>>> + ACC_DEV_MAGIC_V1 = 0XCDCDCDCDFEEDAACC,
>>>>>> + ACC_DEV_MAGIC_V2 = 0xAACCFEEDDECADEDE,
>>>>>
>>>>>
>>>>> I think we have discussed this before that having some kind of
>>>>> version info embed into magic_num will be beneficial going
>>>>> forward. ie, may be use the last 4 bytes for denoting version.
>>>>>
>>>>> ACC_DEV_MAGIC_V2 = 0xAACCFEEDDECA0002
>>>>>
>>>>> The reason being, otherwise we have to come up with a random
>>>>> magic each time when a fix like this is required in future.
>>>>
>>>> Overloading the magic value like this is a bit strange. Typically
>>>> the magic value should be the cookie that identifies the data blob as
>>>> generated by this driver and a separate version field would identify
>>>> any content or format changes. In the mtty driver I included a magic
>>>> field along with major and minor versions to provide this flexibility.
>>>> I wonder why we wouldn't do something similar here rather than create
>>>> this combination magic/version field. It's easy enough to append a
>>>> couple fields onto the structure or redefine a v2 structure to use
>>>> going forward. There's even a padding field in the structure already
>>>> that could be repurposed.
>>>>
>>>
>>> If we add the major and minor version numbers like mtty driver, the current
>>> data structure needs to be changed, and this change also needs to be compatible
>>> with the old and new versions.
>>> And the old version does not have this information. The new version cannot
>>> be adapted when it is imported.
>>
>> Are you suggesting that we cannot use a different data structure based
>> on the magic value? That's rather the point of the magic value.
>>
>>> Now in this patch to fix this problem, we only need to update a magic number,
>>> and then the problem is corrected. Migration between old and new versions can
>>> also be fixed:
>>>
>>> Old --> Old, wrong, but we can't stop it;
>>> Old --> New, corrected migrated incorrect data, recovery successful;
>>> New --> New, correct, recovery successful;
>>> New --> Old, correct, recovery successful.
>
> BTW, New->Old is not possible given the current old code and this
> series doesn't enable it. New magic is saved, old restore could should
> reject it:
>
> static int vf_qm_check_match(struct hisi_acc_vf_core_device *hisi_acc_vdev,
> struct hisi_acc_vf_migration_file *migf)
> {
> ...
> if (vf_data->acc_magic != ACC_DEV_MAGIC) {
> dev_err(dev, "failed to match ACC_DEV_MAGIC\n");
> return -EINVAL;
> }
>
>>From this patch:
Yes, New->Old will be rejected when matching like this.
>
> @@ -418,7 +440,7 @@ static int vf_qm_get_match_data(struct hisi_acc_vf_core_device *hisi_acc_vdev,
> int vf_id = hisi_acc_vdev->vf_id;
> int ret;
>
> - vf_data->acc_magic = ACC_DEV_MAGIC;
> + vf_data->acc_magic = ACC_DEV_MAGIC_V2;
> /* Save device id */
> vf_data->dev_id = hisi_acc_vdev->vf_dev->device;
>
> Thanks,
> Alex
>
>>>
>>> As for the compatibility issues between old and new versions in the future,
>>> we do not need to reserve version numbers to deal with them now. Because
>>> before encountering specific problems, our design may be redundant.
>>
>> A magic value + version number would prevent the need to replace the
>> magic value every time an issue is encountered, which I think was also
>> Shameer's commit, which is not addressed by forcing the formatting of a
>> portion of the magic value. None of what you're trying to do here
>> precludes a new data structure for the new magic value or defining the
>> padding fields for different use cases. Thanks,
>>
>> Alex
>
If we want to use the original magic number, we also need to add the major
and minor version numbers. It does not cause compatibility issues and can
only reuse the original u64 memory space.
This is also Shameerali's previous plan. Do you accept this plan?
Thanks.
Longfang.
>
> .
>
