- Linuxarm - mailweb.openeuler.org

Re: [PATCH v2 1/4] uacce: fix for cdev memory leak
by linwenkai (C) 26 Sep '25

26 Sep '25

在 2025/9/26 16:47, huangchenghai 写道: > > On Wed, Sep 17, 2025 at 06:18 PM +0800, Greg KH wrote: >> On Wed, Sep 17, 2025 at 05:56:16PM +0800, huangchenghai wrote: >>> On Mon, Sep 16, 2025 at 11:15 PM +0800, Greg KH wrote: >>>> On Tue, Sep 16, 2025 at 10:48:08PM +0800, Chenghai Huang wrote: >>>>> From: Wenkai Lin <linwenkai6(a)hisilicon.com> >>>>> >>>>> If cdev_device_add failed, it is hard to determine >>>>> whether cdev_del has been executed, which lead to a >>>>> memory leak issue, so we use cdev_init to avoid it. >>>> I do not understand, what is wrong with the current code? It checks if >>>> add fails: >>>> >>>>> Fixes: 015d239ac014 ("uacce: add uacce driver") >>>>> Cc: stable(a)vger.kernel.org >>>>> Signed-off-by: Wenkai Lin <linwenkai6(a)hisilicon.com> >>>>> Signed-off-by: Chenghai Huang <huangchenghai2(a)huawei.com> >>>>> --- >>>>> drivers/misc/uacce/uacce.c | 13 ++++--------- >>>>> include/linux/uacce.h | 2 +- >>>>> 2 files changed, 5 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c >>>>> index 42e7d2a2a90c..12370469f646 100644 >>>>> --- a/drivers/misc/uacce/uacce.c >>>>> +++ b/drivers/misc/uacce/uacce.c >>>>> @@ -522,14 +522,10 @@ int uacce_register(struct uacce_device *uacce) >>>>> if (!uacce) >>>>> return -ENODEV; >>>>> - uacce->cdev = cdev_alloc(); >>>>> - if (!uacce->cdev) >>>>> - return -ENOMEM; >>>> This is the check. >>>> >>>> >>>>> - >>>>> - uacce->cdev->ops = &uacce_fops; >>>>> - uacce->cdev->owner = THIS_MODULE; >>>>> + cdev_init(&uacce->cdev, &uacce_fops); >>>>> + uacce->cdev.owner = THIS_MODULE; >>>>> - return cdev_device_add(uacce->cdev, &uacce->dev); >>>>> + return cdev_device_add(&uacce->cdev, &uacce->dev); >>>> And so is this. So what is wrong here? >>>> >>>> >>>>> } >>>>> EXPORT_SYMBOL_GPL(uacce_register); >>>>> @@ -568,8 +564,7 @@ void uacce_remove(struct uacce_device *uacce) >>>>> unmap_mapping_range(q->mapping, 0, 0, 1); >>>>> } >>>>> - if (uacce->cdev) >>>>> - cdev_device_del(uacce->cdev, &uacce->dev); >>>>> + cdev_device_del(&uacce->cdev, &uacce->dev); >>>>> xa_erase(&uacce_xa, uacce->dev_id); >>>>> /* >>>>> * uacce exists as long as there are open fds, but ops will >>>>> be freed >>>>> diff --git a/include/linux/uacce.h b/include/linux/uacce.h >>>>> index e290c0269944..98b896192a44 100644 >>>>> --- a/include/linux/uacce.h >>>>> +++ b/include/linux/uacce.h >>>>> @@ -126,7 +126,7 @@ struct uacce_device { >>>>> bool is_vf; >>>>> u32 flags; >>>>> u32 dev_id; >>>>> - struct cdev *cdev; >>>>> + struct cdev cdev; >>>>> struct device dev; >>>> You can not do this, you now have 2 different reference counts >>>> controlling the lifespan of this one structure. That is just going to >>>> cause so many more bugs... >>>> >>>> How was this tested? What is currently failing that requires this >>>> change? >>>> >>>> thanks, >>>> >>>> greg k-h >>> We analyze it theoretically there may be a memory leak >>> issue here, if the cdev_device_add returns a failure, >>> the uacce_remove will not be executed, which results in the >>> uacce cdev memory not being released. >> Then properly clean up if that happens. >> >>> Therefore, we have decided to align with the design of other >>> drivers by making cdev a static member of uacce_device and >>> releasing the memory through uacce_device. >> But again, this is wrong to do. >> >>> found one example in drivers/watchdog/watchdog_dev.h. >>> struct watchdog_core_data { >>> struct device dev; >>> struct cdev cdev; >> This is also wrong and needs to be fixed. Please send a patch to >> resolve it as well, as it should not be copied as a valid example. >> >> thanks, >> >> greg k-h > Very sorry for the delayed response. > > In v1, our first thought was that if cdev_device_add returns a > failure, we could release the resources allocated by cdev_alloc > using cdev_del. For this, we attempted the following modification: > > @@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(uacce_alloc); > */ > int uacce_register(struct uacce_device *uacce) > { > + int ret; > + > if (!uacce) > return -ENODEV; > > @@ -529,7 +531,14 @@ int uacce_register(struct uacce_device *uacce) > uacce->cdev->ops = &uacce_fops; > uacce->cdev->owner = THIS_MODULE; > > - return cdev_device_add(uacce->cdev, &uacce->dev); > + ret = cdev_device_add(uacce->cdev, &uacce->dev); > + if (ret) { > + cdev_del(uacce->cdev); > + uacce->cdev = NULL; > + return ret; > + } > + > + return 0; > } > > However, after further analysis, we found that cdev_device_add does > not increment the reference count when it fails. Therefore, in this > case, cdev_del is not necessary. This means that the resources > allocated by cdev_alloc will not cause a memory leak in the failure > path. > > Thus, I believe this patch modification is unnecessary. In the > upcoming v3 version, I will remove this modification. > > Thank you for your patient guidance! > > Best regards, > Chenghai >> After further analysis, it was found that during the cdev_alloc process, the reference count of the cdev is initialized to 1. If kobject_put is not actively executed, the release function of the cdev cannot be called, and the memory will never be released, leading to a leak, so kobject_put is still necessary here? Here is the new solution: @@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(uacce_alloc); */ int uacce_register(struct uacce_device *uacce) { + int ret; + if (!uacce) return -ENODEV; @@ -529,7 +531,14 @@ int uacce_register(struct uacce_device *uacce) uacce->cdev->ops = &uacce_fops; uacce->cdev->owner = THIS_MODULE; - return cdev_device_add(uacce->cdev, &uacce->dev); + ret = cdev_device_add(uacce->cdev, &uacce->dev); + if (ret) { + kobject_put(&uacce->cdev->kobject); + uacce->cdev = NULL; + return ret; + } + + return 0; } Thanks, WenKai

1 0

Re: [PATCH v6 3/3] migration: adapt to new migration configuration
by Shameerali Kolothum Thodi 28 Jul '25

28 Jul '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, July 17, 2025 2:15 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; > herbert(a)gondor.apana.org.au; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: linux-crypto(a)vger.kernel.org; kvm(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org; linuxarm(a)openeuler.org; liulongfang > <liulongfang(a)huawei.com> > Subject: [PATCH v6 3/3] migration: adapt to new migration configuration > > On new platforms greater than QM_HW_V3, the migration region has been > relocated from the VF to the PF. The driver must also be modified > accordingly to adapt to the new hardware device. > > Utilize the PF's I/O base directly on the new hardware platform, > and no mmap operation is required. If it is on an old platform, > the driver needs to be compatible with the old solution. I think it's good to clarify here how the new hardware platform will continue to work with the existing driver. From our off-list discussions, my understanding is that the QM_HW_V4 hardware is backward compatible and will function identically to QM_HW_V3 as long as the PF is not configured to enable the migration region, as introduced in patch #2. This backward compatibility is the reason we are not preventing migration from older kernels to the new one. With that in place: Reviewed-by: Shameer Kolothum shameerali.kolothum.thodi(a)huawei.com Thanks, Shameer > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 164 ++++++++++++------ > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 + > 2 files changed, 118 insertions(+), 53 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 515ff87f9ed9..bf4a7468bca0 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -125,6 +125,72 @@ static int qm_get_cqc(struct hisi_qm *qm, u64 > *addr) > return 0; > } > > +static int qm_get_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_read_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_read_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int qm_set_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_write_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_write_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > static int qm_get_regs(struct hisi_qm *qm, struct acc_vf_data *vf_data) > { > struct device *dev = &qm->pdev->dev; > @@ -167,20 +233,6 @@ static int qm_get_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to read QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to read QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -239,20 +291,6 @@ static int qm_set_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to write QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to write QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -522,6 +560,10 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + ret = qm_set_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = hisi_qm_mb(qm, QM_MB_CMD_SQC_BT, qm->sqc_dma, 0, 0); > if (ret) { > dev_err(dev, "set sqc failed\n"); > @@ -589,6 +631,10 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > vf_data->vf_qm_state = QM_READY; > hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > > + ret = qm_get_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = vf_qm_read_data(vf_qm, vf_data); > if (ret) > return ret; > @@ -1186,34 +1232,45 @@ static int hisi_acc_vf_qm_init(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > { > struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > + struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > struct pci_dev *vf_dev = vdev->pdev; > > - /* > - * ACC VF dev BAR2 region consists of both functional register space > - * and migration control register space. For migration to work, we > - * need access to both. Hence, we map the entire BAR2 region here. > - * But unnecessarily exposing the migration BAR region to the Guest > - * has the potential to prevent/corrupt the Guest migration. Hence, > - * we restrict access to the migration control space from > - * Guest(Please see mmap/ioctl/read/write override functions). > - * > - * Please note that it is OK to expose the entire VF BAR if migration > - * is not supported or required as this cannot affect the ACC PF > - * configurations. > - * > - * Also the HiSilicon ACC VF devices supported by this driver on > - * HiSilicon hardware platforms are integrated end point devices > - * and the platform lacks the capability to perform any PCIe P2P > - * between these devices. > - */ > - > - vf_qm->io_base = > - ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > - pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > - if (!vf_qm->io_base) > - return -EIO; > + if (pf_qm->ver == QM_HW_V3) { > + /* > + * ACC VF dev BAR2 region consists of both functional > register space > + * and migration control register space. For migration to > work, we > + * need access to both. Hence, we map the entire BAR2 > region here. > + * But unnecessarily exposing the migration BAR region to > the Guest > + * has the potential to prevent/corrupt the Guest migration. > Hence, > + * we restrict access to the migration control space from > + * Guest(Please see mmap/ioctl/read/write override > functions). > + * > + * Please note that it is OK to expose the entire VF BAR if > migration > + * is not supported or required as this cannot affect the ACC > PF > + * configurations. > + * > + * Also the HiSilicon ACC VF devices supported by this driver > on > + * HiSilicon hardware platforms are integrated end point > devices > + * and the platform lacks the capability to perform any PCIe > P2P > + * between these devices. > + */ > > + vf_qm->io_base = > + ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > + pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > + if (!vf_qm->io_base) > + return -EIO; > + } else { > + /* > + * On hardware platforms greater than QM_HW_V3, the > migration function > + * register is placed in the BAR2 configuration region of the > PF, > + * and each VF device occupies 8KB of configuration space. > + */ > + vf_qm->io_base = pf_qm->io_base + > QM_MIG_REGION_OFFSET + > + hisi_acc_vdev->vf_id * > QM_MIG_REGION_SIZE; > + } > vf_qm->fun_type = QM_HW_VF; > + vf_qm->ver = pf_qm->ver; > vf_qm->pdev = vf_dev; > mutex_init(&vf_qm->mailbox_lock); > > @@ -1539,7 +1596,8 @@ static void hisi_acc_vfio_pci_close_device(struct > vfio_device *core_vdev) > hisi_acc_vf_disable_fds(hisi_acc_vdev); > mutex_lock(&hisi_acc_vdev->open_mutex); > hisi_acc_vdev->dev_opened = false; > - iounmap(vf_qm->io_base); > + if (vf_qm->ver == QM_HW_V3) > + iounmap(vf_qm->io_base); > mutex_unlock(&hisi_acc_vdev->open_mutex); > vfio_pci_core_close_device(core_vdev); > } > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > index 91002ceeebc1..348f8bb5b42c 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > @@ -59,6 +59,13 @@ > #define ACC_DEV_MAGIC_V1 0XCDCDCDCDFEEDAACC > #define ACC_DEV_MAGIC_V2 0xAACCFEEDDECADEDE > > +#define QM_MIG_REGION_OFFSET 0x180000 > +#define QM_MIG_REGION_SIZE 0x2000 > + > +#define QM_SUB_VERSION_ID 0x100210 > +#define QM_EQC_PF_DW0 0x1c00 > +#define QM_AEQC_PF_DW0 0x1c20 > + > struct acc_vf_data { > #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) > /* QM match information */ > -- > 2.24.0

1 0

Re: [PATCH v5 3/3] migration: adapt to new migration configuration
by Shameerali Kolothum Thodi 08 Jul '25

08 Jul '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Monday, June 30, 2025 9:54 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v5 3/3] migration: adapt to new migration configuration > > On new platforms greater than QM_HW_V3, the migration region has been > relocated from the VF to the PF. The driver must also be modified > accordingly to adapt to the new hardware device. > > Utilize the PF's I/O base directly on the new hardware platform, > and no mmap operation is required. If it is on an old platform, > the driver needs to be compatible with the old solution. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 166 ++++++++++++------ > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 + > 2 files changed, 120 insertions(+), 53 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 1ddc9dbadb70..3aec3b92787f 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -125,6 +125,72 @@ static int qm_get_cqc(struct hisi_qm *qm, u64 > *addr) > return 0; > } > > +static int qm_get_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_read_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_read_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int qm_set_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_write_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_write_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > static int qm_get_regs(struct hisi_qm *qm, struct acc_vf_data *vf_data) > { > struct device *dev = &qm->pdev->dev; > @@ -167,20 +233,6 @@ static int qm_get_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to read QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to read QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -239,20 +291,6 @@ static int qm_set_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to write QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to write QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -522,6 +560,10 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + ret = qm_set_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = hisi_qm_mb(qm, QM_MB_CMD_SQC_BT, qm->sqc_dma, 0, 0); > if (ret) { > dev_err(dev, "set sqc failed\n"); > @@ -589,6 +631,10 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > vf_data->vf_qm_state = QM_READY; > hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > > + ret = qm_get_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = vf_qm_read_data(vf_qm, vf_data); > if (ret) > return ret; > @@ -1186,34 +1232,47 @@ static int hisi_acc_vf_qm_init(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > { > struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > + struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > struct pci_dev *vf_dev = vdev->pdev; > > - /* > - * ACC VF dev BAR2 region consists of both functional register space > - * and migration control register space. For migration to work, we > - * need access to both. Hence, we map the entire BAR2 region here. > - * But unnecessarily exposing the migration BAR region to the Guest > - * has the potential to prevent/corrupt the Guest migration. Hence, > - * we restrict access to the migration control space from > - * Guest(Please see mmap/ioctl/read/write override functions). > - * > - * Please note that it is OK to expose the entire VF BAR if migration > - * is not supported or required as this cannot affect the ACC PF > - * configurations. > - * > - * Also the HiSilicon ACC VF devices supported by this driver on > - * HiSilicon hardware platforms are integrated end point devices > - * and the platform lacks the capability to perform any PCIe P2P > - * between these devices. > - */ > + if (pf_qm->ver == QM_HW_V3) { > + /* > + * ACC VF dev BAR2 region consists of both functional > register space > + * and migration control register space. For migration to > work, we > + * need access to both. Hence, we map the entire BAR2 > region here. > + * But unnecessarily exposing the migration BAR region to > the Guest > + * has the potential to prevent/corrupt the Guest migration. > Hence, > + * we restrict access to the migration control space from > + * Guest(Please see mmap/ioctl/read/write override > functions). > + * > + * Please note that it is OK to expose the entire VF BAR if > migration > + * is not supported or required as this cannot affect the ACC > PF > + * configurations. > + * > + * Also the HiSilicon ACC VF devices supported by this driver > on > + * HiSilicon hardware platforms are integrated end point > devices > + * and the platform lacks the capability to perform any PCIe > P2P > + * between these devices. > + */ > > - vf_qm->io_base = > - ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > - pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > - if (!vf_qm->io_base) > - return -EIO; > + vf_qm->io_base = > + ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > + pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > + if (!vf_qm->io_base) > + return -EIO; > > - vf_qm->fun_type = QM_HW_VF; > + vf_qm->fun_type = QM_HW_VF; > + vf_qm->ver = pf_qm->ver; > + } else { > + /* > + * On hardware platforms greater than QM_HW_V3, the > migration function > + * register is placed in the BAR2 configuration region of the > PF, > + * and each VF device occupies 8KB of configuration space. > + */ > + vf_qm->io_base = pf_qm->io_base + > QM_MIG_REGION_OFFSET + > + hisi_acc_vdev->vf_id * > QM_MIG_REGION_SIZE; > + vf_qm->fun_type = QM_HW_PF; I don't think you can use the QM fun_type to distinguish this, because it is still a VF dev. Better to have another field to detect this. Also I have another question. In the hisi_acc_vfio_pci_probe() we currently look for pf_qm-> >= QM_HW_V3. This means current driver can get loaded on this new hardware, right? If so, I think we need to prevent that. And also we need to block any migration attempt from existing host kernels to new ones. Thanks, Shameer > + } > vf_qm->pdev = vf_dev; > mutex_init(&vf_qm->mailbox_lock); > > @@ -1539,7 +1598,8 @@ static void hisi_acc_vfio_pci_close_device(struct > vfio_device *core_vdev) > hisi_acc_vf_disable_fds(hisi_acc_vdev); > mutex_lock(&hisi_acc_vdev->open_mutex); > hisi_acc_vdev->dev_opened = false; > - iounmap(vf_qm->io_base); > + if (vf_qm->ver == QM_HW_V3) > + iounmap(vf_qm->io_base); > mutex_unlock(&hisi_acc_vdev->open_mutex); > vfio_pci_core_close_device(core_vdev); > } > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > index 91002ceeebc1..348f8bb5b42c 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > @@ -59,6 +59,13 @@ > #define ACC_DEV_MAGIC_V1 0XCDCDCDCDFEEDAACC > #define ACC_DEV_MAGIC_V2 0xAACCFEEDDECADEDE > > +#define QM_MIG_REGION_OFFSET 0x180000 > +#define QM_MIG_REGION_SIZE 0x2000 > + > +#define QM_SUB_VERSION_ID 0x100210 > +#define QM_EQC_PF_DW0 0x1c00 > +#define QM_AEQC_PF_DW0 0x1c20 > + > struct acc_vf_data { > #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) > /* QM match information */ > -- > 2.24.0

1 0

Re: [PATCH v5 2/3] migration: qm updates BAR configuration
by Shameerali Kolothum Thodi 08 Jul '25

08 Jul '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Monday, June 30, 2025 9:54 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v5 2/3] migration: qm updates BAR configuration > > On new platforms greater than QM_HW_V3, the configuration region for > the > live migration function of the accelerator device is no longer > placed in the VF, but is instead placed in the PF. > > Therefore, the configuration region of the live migration function > needs to be opened when the QM driver is loaded. When the QM driver > is uninstalled, the driver needs to clear this configuration. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/crypto/hisilicon/qm.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c > index d3f5d108b898..0a8888304e15 100644 > --- a/drivers/crypto/hisilicon/qm.c > +++ b/drivers/crypto/hisilicon/qm.c > @@ -242,6 +242,9 @@ > #define QM_QOS_MAX_CIR_U 6 > #define QM_AUTOSUSPEND_DELAY 3000 > > +#define QM_MIG_REGION_SEL 0x100198 > +#define QM_MIG_REGION_EN 0x1 > + > /* abnormal status value for stopping queue */ > #define QM_STOP_QUEUE_FAIL 1 > #define QM_DUMP_SQC_FAIL 3 > @@ -3004,11 +3007,36 @@ static void qm_put_pci_res(struct hisi_qm *qm) > pci_release_mem_regions(pdev); > } > > +static void hisi_mig_region_clear(struct hisi_qm *qm) > +{ > + u32 val; > + > + /* Clear migration region set of PF */ > + if (qm->fun_type == QM_HW_PF && qm->ver > QM_HW_V3) { > + val = readl(qm->io_base + QM_MIG_REGION_SEL); > + val &= ~BIT(0); > + writel(val, qm->io_base + QM_MIG_REGION_SEL); > + } > +} > + > +static void hisi_mig_region_enable(struct hisi_qm *qm) > +{ > + u32 val; > + > + /* Select migration region of PF */ > + if (qm->fun_type == QM_HW_PF && qm->ver > QM_HW_V3) { > + val = readl(qm->io_base + QM_MIG_REGION_SEL); > + val |= QM_MIG_REGION_EN; > + writel(val, qm->io_base + QM_MIG_REGION_SEL); > + } > +} May adding a comment for above functions will be helpful. > + > static void hisi_qm_pci_uninit(struct hisi_qm *qm) > { > struct pci_dev *pdev = qm->pdev; > > pci_free_irq_vectors(pdev); > + hisi_mig_region_clear(qm); > qm_put_pci_res(qm); > pci_disable_device(pdev); > } > @@ -5630,6 +5658,7 @@ int hisi_qm_init(struct hisi_qm *qm) > goto err_free_qm_memory; > > qm_cmd_init(qm); > + hisi_mig_region_enable(qm); > > return 0; Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> You need to CC QM driver maintainers for this. Thanks, SHameer

1 0

Re: [PATCH v5 1/3] migration: update BAR space size
by Shameerali Kolothum Thodi 08 Jul '25

08 Jul '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Monday, June 30, 2025 9:54 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v5 1/3] migration: update BAR space size > > On new platforms greater than QM_HW_V3, the live migration > configuration > region is moved from VF to PF. The VF's own configuration space is > restored to the complete 64KB, and there is no need to divide the > size of the BAR configuration space equally. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- See one minor comment below. LGTM: Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 36 ++++++++++++++----- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 2149f49aeec7..1ddc9dbadb70 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -1250,6 +1250,28 @@ static struct hisi_qm *hisi_acc_get_pf_qm(struct > pci_dev *pdev) > return !IS_ERR(pf_qm) ? pf_qm : NULL; > } > > +static size_t hisi_acc_get_resource_len(struct vfio_pci_core_device *vdev, > + unsigned int index) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = > + hisi_acc_drvdata(vdev->pdev); > + > + /* > + * On the old QM_HW_V3 device, the ACC VF device BAR2 > + * region encompasses both functional register space > + * and migration control register space. > + * only the functional region should be report to Guest. > + * > + * On the new HW device, the migration control register > + * has been moved to the PF device BAR2 region. > + * The VF device BAR2 is entirely functional register space. > + */ Nit: May be move the above comment about new HW to the below check is better. > + if (hisi_acc_vdev->pf_qm->ver == QM_HW_V3) > + return (pci_resource_len(vdev->pdev, index) >> 1); > + > + return pci_resource_len(vdev->pdev, index); > +} > + > static int hisi_acc_pci_rw_access_check(struct vfio_device *core_vdev, > size_t count, loff_t *ppos, > size_t *new_count) > @@ -1260,8 +1282,9 @@ static int hisi_acc_pci_rw_access_check(struct > vfio_device *core_vdev, > > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = hisi_acc_get_resource_len(vdev, index); > /* Check if access is for migration control region */ > if (pos >= end) > return -EINVAL; > @@ -1282,8 +1305,9 @@ static int hisi_acc_vfio_pci_mmap(struct > vfio_device *core_vdev, > index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT); > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > u64 req_len, pgoff, req_start; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = hisi_acc_get_resource_len(vdev, index); > req_len = vma->vm_end - vma->vm_start; > pgoff = vma->vm_pgoff & > ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1); > @@ -1330,7 +1354,6 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (cmd == VFIO_DEVICE_GET_REGION_INFO) { > struct vfio_pci_core_device *vdev = > container_of(core_vdev, struct vfio_pci_core_device, > vdev); > - struct pci_dev *pdev = vdev->pdev; > struct vfio_region_info info; > unsigned long minsz; > > @@ -1345,12 +1368,7 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (info.index == VFIO_PCI_BAR2_REGION_INDEX) { > info.offset = > VFIO_PCI_INDEX_TO_OFFSET(info.index); > > - /* > - * ACC VF dev BAR2 region consists of both > functional > - * register space and migration control register > space. > - * Report only the functional region to Guest. > - */ > - info.size = pci_resource_len(pdev, info.index) / 2; > + info.size = hisi_acc_get_resource_len(vdev, > info.index); > > info.flags = VFIO_REGION_INFO_FLAG_READ | > VFIO_REGION_INFO_FLAG_WRITE | > -- > 2.24.0

1 0

Re: [PATCH v4 3/3] migration: adapt to new migration configuration
by Shameerali Kolothum Thodi 24 Jun '25

24 Jun '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, June 10, 2025 7:33 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v4 3/3] migration: adapt to new migration configuration > > On the new hardware platform, the migration region has been > relocated from the VF to the PF. The driver must also be modified > accordingly to adapt to the new hardware device. > > Utilize the PF's I/O base directly on the new hardware platform, > and no mmap operation is required. If it is on an old platform, > the driver needs to be compatible with the old solution. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 166 ++++++++++++------ > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 + > 2 files changed, 120 insertions(+), 53 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index b16115f590fd..ab815fa4258c 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -125,6 +125,72 @@ static int qm_get_cqc(struct hisi_qm *qm, u64 > *addr) > return 0; > } > > +static int qm_get_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_read_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_read_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int qm_set_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_write_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_write_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > static int qm_get_regs(struct hisi_qm *qm, struct acc_vf_data *vf_data) > { > struct device *dev = &qm->pdev->dev; > @@ -167,20 +233,6 @@ static int qm_get_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to read QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to read QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -239,20 +291,6 @@ static int qm_set_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to write QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to write QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -522,6 +560,10 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + ret = qm_set_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = hisi_qm_mb(qm, QM_MB_CMD_SQC_BT, qm->sqc_dma, 0, 0); > if (ret) { > dev_err(dev, "set sqc failed\n"); > @@ -589,6 +631,10 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > vf_data->vf_qm_state = QM_READY; > hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > > + ret = qm_get_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = vf_qm_read_data(vf_qm, vf_data); > if (ret) > return ret; > @@ -1186,34 +1232,47 @@ static int hisi_acc_vf_qm_init(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > { > struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > + struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > struct pci_dev *vf_dev = vdev->pdev; > > - /* > - * ACC VF dev BAR2 region consists of both functional register space > - * and migration control register space. For migration to work, we > - * need access to both. Hence, we map the entire BAR2 region here. > - * But unnecessarily exposing the migration BAR region to the Guest > - * has the potential to prevent/corrupt the Guest migration. Hence, > - * we restrict access to the migration control space from > - * Guest(Please see mmap/ioctl/read/write override functions). > - * > - * Please note that it is OK to expose the entire VF BAR if migration > - * is not supported or required as this cannot affect the ACC PF > - * configurations. > - * > - * Also the HiSilicon ACC VF devices supported by this driver on > - * HiSilicon hardware platforms are integrated end point devices > - * and the platform lacks the capability to perform any PCIe P2P > - * between these devices. > - */ > + if (pf_qm->ver == QM_HW_V3) { > + /* > + * ACC VF dev BAR2 region consists of both functional > register space > + * and migration control register space. For migration to > work, we > + * need access to both. Hence, we map the entire BAR2 > region here. > + * But unnecessarily exposing the migration BAR region to > the Guest > + * has the potential to prevent/corrupt the Guest migration. > Hence, > + * we restrict access to the migration control space from > + * Guest(Please see mmap/ioctl/read/write override > functions). > + * > + * Please note that it is OK to expose the entire VF BAR if > migration > + * is not supported or required as this cannot affect the ACC > PF > + * configurations. > + * > + * Also the HiSilicon ACC VF devices supported by this driver > on > + * HiSilicon hardware platforms are integrated end point > devices > + * and the platform lacks the capability to perform any PCIe > P2P > + * between these devices. > + */ > > - vf_qm->io_base = > - ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > - pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > - if (!vf_qm->io_base) > - return -EIO; > + vf_qm->io_base = > + ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > + pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > + if (!vf_qm->io_base) > + return -EIO; > > - vf_qm->fun_type = QM_HW_VF; > + vf_qm->fun_type = QM_HW_VF; > + vf_qm->ver = pf_qm->ver; > + } else { > + /* > + * On the new HW device, the migration function register is What is the "new HW device"? Please make it specific. Also I think you should check it specifically. One more question is what will happen if you try to migrate between a QM_HW_V3 <--> QM_HW_V4? Will it succeed or do we need to block it? Thanks, Shameer > placed > + * in the BAR2 configuration region of the PF, and each VF > device > + * occupies 8KB of configuration space. > + */ > + vf_qm->io_base = pf_qm->io_base + > QM_MIG_REGION_OFFSET + > + hisi_acc_vdev->vf_id * > QM_MIG_REGION_SIZE; > + vf_qm->fun_type = QM_HW_PF; > + } > vf_qm->pdev = vf_dev; > mutex_init(&vf_qm->mailbox_lock); > > @@ -1539,7 +1598,8 @@ static void hisi_acc_vfio_pci_close_device(struct > vfio_device *core_vdev) > hisi_acc_vf_disable_fds(hisi_acc_vdev); > mutex_lock(&hisi_acc_vdev->open_mutex); > hisi_acc_vdev->dev_opened = false; > - iounmap(vf_qm->io_base); > + if (vf_qm->ver == QM_HW_V3) > + iounmap(vf_qm->io_base); > mutex_unlock(&hisi_acc_vdev->open_mutex); > vfio_pci_core_close_device(core_vdev); > } > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > index 91002ceeebc1..348f8bb5b42c 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > @@ -59,6 +59,13 @@ > #define ACC_DEV_MAGIC_V1 0XCDCDCDCDFEEDAACC > #define ACC_DEV_MAGIC_V2 0xAACCFEEDDECADEDE > > +#define QM_MIG_REGION_OFFSET 0x180000 > +#define QM_MIG_REGION_SIZE 0x2000 > + > +#define QM_SUB_VERSION_ID 0x100210 > +#define QM_EQC_PF_DW0 0x1c00 > +#define QM_AEQC_PF_DW0 0x1c20 > + > struct acc_vf_data { > #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) > /* QM match information */ > -- > 2.24.0

1 0

Re: [PATCH v4 2/3] migration: qm updates BAR configuration
by Shameerali Kolothum Thodi 24 Jun '25

24 Jun '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, June 10, 2025 7:33 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v4 2/3] migration: qm updates BAR configuration > > On the new hardware platform, the configuration region for the > live migration function of the accelerator device is no longer > placed in the VF, but is instead placed in the PF. > > Therefore, the configuration region of the live migration function > needs to be opened when the QM driver is loaded. When the QM driver > is uninstalled, the driver needs to clear this configuration. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/crypto/hisilicon/qm.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c > index d3f5d108b898..0a8888304e15 100644 > --- a/drivers/crypto/hisilicon/qm.c > +++ b/drivers/crypto/hisilicon/qm.c > @@ -242,6 +242,9 @@ > #define QM_QOS_MAX_CIR_U 6 > #define QM_AUTOSUSPEND_DELAY 3000 > > +#define QM_MIG_REGION_SEL 0x100198 > +#define QM_MIG_REGION_EN 0x1 > + > /* abnormal status value for stopping queue */ > #define QM_STOP_QUEUE_FAIL 1 > #define QM_DUMP_SQC_FAIL 3 > @@ -3004,11 +3007,36 @@ static void qm_put_pci_res(struct hisi_qm *qm) > pci_release_mem_regions(pdev); > } > > +static void hisi_mig_region_clear(struct hisi_qm *qm) > +{ > + u32 val; > + > + /* Clear migration region set of PF */ > + if (qm->fun_type == QM_HW_PF && qm->ver > QM_HW_V3) { Is this going to be same for all future hardware's like OM_HW_V5, OM_HW_V6 etc? Otherwise it is better you make it specific to OM_HW_V4. I think the above checking is repeated throughout this series and there is no guarantee that future hardware will have the same changes only. So better make it specific. Thanks, Shameer > + val = readl(qm->io_base + QM_MIG_REGION_SEL); > + val &= ~BIT(0); > + writel(val, qm->io_base + QM_MIG_REGION_SEL); > + } > +} > + > +static void hisi_mig_region_enable(struct hisi_qm *qm) > +{ > + u32 val; > + > + /* Select migration region of PF */ > + if (qm->fun_type == QM_HW_PF && qm->ver > QM_HW_V3) { > + val = readl(qm->io_base + QM_MIG_REGION_SEL); > + val |= QM_MIG_REGION_EN; > + writel(val, qm->io_base + QM_MIG_REGION_SEL); > + } > +} > + > static void hisi_qm_pci_uninit(struct hisi_qm *qm) > { > struct pci_dev *pdev = qm->pdev; > > pci_free_irq_vectors(pdev); > + hisi_mig_region_clear(qm); > qm_put_pci_res(qm); > pci_disable_device(pdev); > } > @@ -5630,6 +5658,7 @@ int hisi_qm_init(struct hisi_qm *qm) > goto err_free_qm_memory; > > qm_cmd_init(qm); > + hisi_mig_region_enable(qm); > > return 0; > > -- > 2.24.0

1 0

Re: [PATCH v4 1/3] migration: update BAR space size
by Shameerali Kolothum Thodi 24 Jun '25

24 Jun '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, June 10, 2025 7:33 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v4 1/3] migration: update BAR space size > > On the new hardware platform, the live migration configuration region > is moved from VF to PF. The VF's own configuration space is > restored to the complete 64KB, and there is no need to divide the > size of the BAR configuration space equally. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 36 ++++++++++++++----- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 2149f49aeec7..b16115f590fd 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -1250,6 +1250,28 @@ static struct hisi_qm *hisi_acc_get_pf_qm(struct > pci_dev *pdev) > return !IS_ERR(pf_qm) ? pf_qm : NULL; > } > > +static size_t hisi_acc_get_resource_len(struct vfio_pci_core_device *vdev, > + unsigned int index) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = > + hisi_acc_drvdata(vdev->pdev); > + > + /* > + * On the old HW_V3 device, the ACC VF device BAR2 > + * region encompasses both functional register space > + * and migration control register space. > + * only the functional region should be report to Guest. > + * > + * On the new HW device, the migration control register > + * has been moved to the PF device BAR2 region. > + * The VF device BAR2 is entirely functional register space. > + */ > + if (hisi_acc_vdev->pf_qm->ver == QM_HW_V3) > + return (pci_resource_len(vdev->pdev, index) >> 1); > + > + return pci_resource_len(vdev->pdev, index); > +} > + > static int hisi_acc_pci_rw_access_check(struct vfio_device *core_vdev, > size_t count, loff_t *ppos, > size_t *new_count) > @@ -1260,8 +1282,9 @@ static int hisi_acc_pci_rw_access_check(struct > vfio_device *core_vdev, > > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = hisi_acc_get_resource_len(vdev, index); > /* Check if access is for migration control region */ > if (pos >= end) > return -EINVAL; > @@ -1282,8 +1305,9 @@ static int hisi_acc_vfio_pci_mmap(struct > vfio_device *core_vdev, > index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT); > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > u64 req_len, pgoff, req_start; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = PAGE_ALIGN(hisi_acc_get_resource_len(vdev, index)); I think I have commented on this before. The above PAGE_ALIGN will change the behavior on HW_V3 with 64K PAGE_SIZE kernel. The end will become 64K which is not what we want on HW_V3. Could you please check that again. Thanks, Shameer > req_len = vma->vm_end - vma->vm_start; > pgoff = vma->vm_pgoff & > ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1); > @@ -1330,7 +1354,6 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (cmd == VFIO_DEVICE_GET_REGION_INFO) { > struct vfio_pci_core_device *vdev = > container_of(core_vdev, struct vfio_pci_core_device, > vdev); > - struct pci_dev *pdev = vdev->pdev; > struct vfio_region_info info; > unsigned long minsz; > > @@ -1345,12 +1368,7 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (info.index == VFIO_PCI_BAR2_REGION_INDEX) { > info.offset = > VFIO_PCI_INDEX_TO_OFFSET(info.index); > > - /* > - * ACC VF dev BAR2 region consists of both > functional > - * register space and migration control register > space. > - * Report only the functional region to Guest. > - */ > - info.size = pci_resource_len(pdev, info.index) / 2; > + info.size = hisi_acc_get_resource_len(vdev, > info.index); > > info.flags = VFIO_REGION_INFO_FLAG_READ | > VFIO_REGION_INFO_FLAG_WRITE | > -- > 2.24.0

1 0

Re: [PATCH v7 6/6] hisi_acc_vfio_pci: update function return values.
by Shameerali Kolothum Thodi 15 Apr '25

15 Apr '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Friday, April 11, 2025 4:59 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v7 6/6] hisi_acc_vfio_pci: update function return values. > > In this driver file, many functions call sub-functions and use ret > to store the error code of the sub-functions. > However, instead of directly returning ret to the caller, they use a > converted error code, which prevents the end-user from clearly > understanding the root cause of the error. > Therefore, the code needs to be modified to directly return the error > code from the sub-functions. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index d12a350440d3..c63e302ac092 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -392,7 +392,7 @@ static int vf_qm_check_match(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > ret = vf_qm_version_check(vf_data, dev); > if (ret) { > dev_err(dev, "failed to match ACC_DEV_MAGIC\n"); > - return -EINVAL; > + return ret; > } > > if (vf_data->dev_id != hisi_acc_vdev->vf_dev->device) { > @@ -404,7 +404,7 @@ static int vf_qm_check_match(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > ret = qm_get_vft(vf_qm, &vf_qm->qp_base); > if (ret <= 0) { > dev_err(dev, "failed to get vft qp nums\n"); > - return -EINVAL; > + return ret; > } > > if (ret != vf_data->qp_num) { > @@ -501,7 +501,7 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > ret = qm_write_regs(qm, QM_VF_STATE, &vf_data->vf_qm_state, 1); > if (ret) { > dev_err(dev, "failed to write QM_VF_STATE\n"); > - return -EINVAL; > + return ret; > } > hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > > @@ -542,7 +542,7 @@ static int vf_qm_read_data(struct hisi_qm *vf_qm, > struct acc_vf_data *vf_data) > > ret = qm_get_regs(vf_qm, vf_data); > if (ret) > - return -EINVAL; > + return ret; > > /* Every reg is 32 bit, the dma address is 64 bit. */ > vf_data->eqe_dma = vf_data->qm_eqc_dw[QM_XQC_ADDR_HIGH]; > @@ -556,13 +556,13 @@ static int vf_qm_read_data(struct hisi_qm > *vf_qm, struct acc_vf_data *vf_data) > ret = qm_get_sqc(vf_qm, &vf_data->sqc_dma); > if (ret) { > dev_err(dev, "failed to read SQC addr!\n"); > - return -EINVAL; > + return ret; > } > > ret = qm_get_cqc(vf_qm, &vf_data->cqc_dma); > if (ret) { > dev_err(dev, "failed to read CQC addr!\n"); > - return -EINVAL; > + return ret; > } > > return 0; > @@ -588,7 +588,7 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > > ret = vf_qm_read_data(vf_qm, vf_data); > if (ret) > - return -EINVAL; > + return ret; > > migf->total_length = sizeof(struct acc_vf_data); > /* Save eqc and aeqc interrupt information */ > @@ -1379,7 +1379,7 @@ static int hisi_acc_vf_debug_check(struct seq_file > *seq, struct vfio_device *vde > ret = qm_wait_dev_not_ready(vf_qm); > if (ret) { > seq_puts(seq, "VF device not ready!\n"); > - return -EBUSY; > + return ret; > } > > return 0; Any reason you avoided few other instances here? 1. qemu_set_regs() --> hisi_qm_wait_mb_ready() -->ret -EBUSY; 2. vf_qm_cache_wb() ret -EINVAL on -ETIMEOUT. With the above addressed, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer

1 0

Re: [PATCH v7 5/6] hisi_acc_vfio_pci: bugfix live migration function without VF device driver
by Shameerali Kolothum Thodi 15 Apr '25

15 Apr '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Friday, April 11, 2025 4:59 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v7 5/6] hisi_acc_vfio_pci: bugfix live migration function > without VF device driver > > If the VF device driver is not loaded in the Guest OS and we attempt to > perform device data migration, the address of the migrated data will > be NULL. > The live migration recovery operation on the destination side will > access a null address value, which will cause access errors. > > Therefore, live migration of VMs without added VF device drivers > does not require device data migration. > In addition, when the queue address data obtained by the destination > is empty, device queue recovery processing will not be performed. > > Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > Reviewed-by: Shameer Kolothum > <shameerali.kolothum.thodi(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 22 +++++++++++++------ > 1 file changed, 15 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 cadc82419dca..d12a350440d3 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -426,13 +426,6 @@ static int vf_qm_check_match(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return -EINVAL; > } > > - ret = qm_write_regs(vf_qm, QM_VF_STATE, &vf_data->vf_qm_state, > 1); > - if (ret) { > - dev_err(dev, "failed to write QM_VF_STATE\n"); > - return ret; > - } > - > - hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > hisi_acc_vdev->match_done = true; > return 0; > } > @@ -498,6 +491,20 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > if (migf->total_length < sizeof(struct acc_vf_data)) > return -EINVAL; > > + if (!vf_data->eqe_dma || !vf_data->aeqe_dma || > + !vf_data->sqc_dma || !vf_data->cqc_dma) { > + dev_info(dev, "resume dma addr is NULL!\n"); > + hisi_acc_vdev->vf_qm_state = QM_NOT_READY; > + return 0; > + } I am still not fully understood why we need the above check. The only case as far as I can think of where this will happen is when the source VM Guest has not loaded the ACC driver. And we do take care of that already using vf_qm_state and checking total_length == QM_MATCH_SIZE. Have you seen this happening in any other scenario during your tests? Thanks, Shameer > + > + ret = qm_write_regs(qm, QM_VF_STATE, &vf_data->vf_qm_state, 1); > + if (ret) { > + dev_err(dev, "failed to write QM_VF_STATE\n"); > + return -EINVAL; > + } > + hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > + > qm->eqe_dma = vf_data->eqe_dma; > qm->aeqe_dma = vf_data->aeqe_dma; > qm->sqc_dma = vf_data->sqc_dma; > @@ -1531,6 +1538,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct > vfio_device *core_vdev) > hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1; > hisi_acc_vdev->pf_qm = pf_qm; > hisi_acc_vdev->vf_dev = pdev; > + hisi_acc_vdev->vf_qm_state = QM_NOT_READY; > mutex_init(&hisi_acc_vdev->state_mutex); > mutex_init(&hisi_acc_vdev->open_mutex); > > -- > 2.24.0

1 0

Re: [PATCH v5 5/5] hisi_acc_vfio_pci: bugfix live migration function without VF device driver
by Shameerali Kolothum Thodi 14 Mar '25

14 Mar '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, March 13, 2025 7:20 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v5 5/5] hisi_acc_vfio_pci: bugfix live migration function > without VF device driver > > If the VF device driver is not loaded in the Guest OS and we attempt to > perform device data migration, the address of the migrated data will > be NULL. > The live migration recovery operation on the destination side will > access a null address value, which will cause access errors. > > Therefore, live migration of VMs without added VF device drivers > does not require device data migration. > In addition, when the queue address data obtained by the destination > is empty, device queue recovery processing will not be performed. > > Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 25 +++++++++++++------ > 1 file changed, 18 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 cadc82419dca..44fa2d16bbcc 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -426,13 +426,6 @@ static int vf_qm_check_match(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return -EINVAL; > } > > - ret = qm_write_regs(vf_qm, QM_VF_STATE, &vf_data->vf_qm_state, > 1); > - if (ret) { > - dev_err(dev, "failed to write QM_VF_STATE\n"); > - return ret; > - } > - > - hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > hisi_acc_vdev->match_done = true; > return 0; > } > @@ -498,6 +491,13 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > if (migf->total_length < sizeof(struct acc_vf_data)) > return -EINVAL; > > + ret = qm_write_regs(qm, QM_VF_STATE, &vf_data->vf_qm_state, 1); > + if (ret) { > + dev_err(dev, "failed to write QM_VF_STATE\n"); > + return -EINVAL; > + } > + hisi_acc_vdev->vf_qm_state = vf_data->vf_qm_state; > + > qm->eqe_dma = vf_data->eqe_dma; > qm->aeqe_dma = vf_data->aeqe_dma; > qm->sqc_dma = vf_data->sqc_dma; > @@ -506,6 +506,12 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > qm->qp_base = vf_data->qp_base; > qm->qp_num = vf_data->qp_num; > > + if (!vf_data->eqe_dma || !vf_data->aeqe_dma || > + !vf_data->sqc_dma || !vf_data->cqc_dma) { > + dev_err(dev, "resume dma addr is NULL!\n"); > + return -EINVAL; > + } > + > ret = qm_set_regs(qm, vf_data); > if (ret) { > dev_err(dev, "set VF regs failed\n"); > @@ -726,8 +732,12 @@ static int hisi_acc_vf_load_state(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > { > struct device *dev = &hisi_acc_vdev->vf_dev->dev; > struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev- > >resuming_migf; > + struct acc_vf_data *vf_data = &migf->vf_data; > int ret; > > + if (vf_data->vf_qm_state != QM_READY) > + return 0; I don't think we need to check the above. In vf_qm_satte_save(), If vf_qm_state != QM_READY, we set the migf->total_length = QM_MATCH_SIZE. Hence it will return 0 in the below vf_qm_load_data() anyway. With that corrected, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer > + > /* Recover data to VF */ > ret = vf_qm_load_data(hisi_acc_vdev, migf); > if (ret) { > @@ -1531,6 +1541,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct > vfio_device *core_vdev) > hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1; > hisi_acc_vdev->pf_qm = pf_qm; > hisi_acc_vdev->vf_dev = pdev; > + hisi_acc_vdev->vf_qm_state = QM_NOT_READY; > mutex_init(&hisi_acc_vdev->state_mutex); > mutex_init(&hisi_acc_vdev->open_mutex); > > -- > 2.24.0

1 0

Re: [PATCH v5 1/5] hisi_acc_vfio_pci: fix XQE dma address error
by Shameerali Kolothum Thodi 14 Mar '25

14 Mar '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, March 13, 2025 7:20 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v5 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(a)huawei.com> LGTM, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 41 ++++++++++++++++--- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 14 ++++++- > 2 files changed, 47 insertions(+), 8 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..304dbdfa0e95 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -350,6 +350,32 @@ 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_version_check(struct acc_vf_data *vf_data, struct device > *dev) > +{ > + switch (vf_data->acc_magic) { > + case ACC_DEV_MAGIC_V2: > + if (vf_data->major_ver != ACC_DRV_MAJOR_VER) { > + dev_info(dev, "migration driver version<%u.%u> not > match!\n", > + vf_data->major_ver, vf_data->minor_ver); > + return -EINVAL; > + } > + 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 +389,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_version_check(vf_data, dev); > + if (ret) { > dev_err(dev, "failed to match ACC_DEV_MAGIC\n"); > return -EINVAL; > } > @@ -418,7 +445,9 @@ 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; > + vf_data->major_ver = ACC_DRV_MAJOR_VER; > + vf_data->minor_ver = ACC_DRV_MINOR_VER; > /* Save device id */ > vf_data->dev_id = hisi_acc_vdev->vf_dev->device; > > @@ -496,12 +525,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..91002ceeebc1 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,15 @@ > #define QM_EQC_DW0 0X8000 > #define QM_AEQC_DW0 0X8020 > > +#define ACC_DRV_MAJOR_VER 1 > +#define ACC_DRV_MINOR_VER 0 > + > +#define ACC_DEV_MAGIC_V1 0XCDCDCDCDFEEDAACC > +#define ACC_DEV_MAGIC_V2 0xAACCFEEDDECADEDE > + > struct acc_vf_data { > #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) > /* QM match information */ > -#define ACC_DEV_MAGIC 0XCDCDCDCDFEEDAACC > u64 acc_magic; > u32 qp_num; > u32 dev_id; > @@ -61,7 +69,9 @@ struct acc_vf_data { > u32 qp_base; > u32 vf_qm_state; > /* QM reserved match information */ > - u32 qm_rsv_state[3]; > + u16 major_ver; > + u16 minor_ver; > + u32 qm_rsv_state[2]; > > /* QM RW regs */ > u32 aeq_int_mask; > -- > 2.24.0

1 0

Re: [PATCH v2 0/3] update live migration configuration region
by Shameerali Kolothum Thodi 04 Mar '25

04 Mar '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, March 4, 2025 12:05 PM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 0/3] update live migration configuration region > > On the new hardware platform, the configuration register space > of the live migration function is set on the PF, while on the > old platform, this part is placed on the VF. > > Change v1 -> v2 > Delete the vf_qm_state read operation in Pre_Copy If I understand correctly, previously this was discussed in your bug fix series here, https://lore.kernel.org/all/fa8cd8c1cdbe4849b445ffd8f4894515@huawei.com/ And why we are having it here in this new hardware support series now? Could we please move all the existing bug fixes in one series and support for new platform in another series, please. Thanks, Shameer

1 0

Re: [PATCH v4 5/5] hisi_acc_vfio_pci: bugfix live migration function without VF device driver
by Shameerali Kolothum Thodi 26 Feb '25

26 Feb '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, February 25, 2025 6:28 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v4 5/5] hisi_acc_vfio_pci: bugfix live migration function > without VF device driver > > If the driver of the VF device is not loaded in the Guest OS, > then perform device data migration. The migrated data address will > be NULL. May be rephrase: If the VF device driver is not loaded in the Guest OS and we attempt to perform device data migration, the address of the migrated data will be NULL. > The live migration recovery operation on the destination side will > access a null address value, which will cause access errors. > Therefore, live migration of VMs without added VF device drivers > does not require device data migration. > In addition, when the queue address data obtained by the destination > is empty, device queue recovery processing will not be performed. > > Fixes: b0eed085903e ("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 3f0bcd855839..77872fc4cd34 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -440,6 +440,7 @@ static int vf_qm_get_match_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > struct acc_vf_data *vf_data) > { > struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > + struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > struct device *dev = &pf_qm->pdev->dev; > int vf_id = hisi_acc_vdev->vf_id; > int ret; > @@ -466,6 +467,13 @@ static int vf_qm_get_match_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + /* Get VF driver insmod state */ > + ret = qm_read_regs(vf_qm, QM_VF_STATE, &vf_data->vf_qm_state, > 1); We already have qm_wait_dev_not_ready() function that checks the QM_VF_STATE. Why can't we use that here? Also we are getting this vf_qm_state already in vf_qm_state_save(). And you don't seem to check the vf_qm_state in vf_qm_check_match(). So why it is read early in this function? Thanks, Shameer > + if (ret) { > + dev_err(dev, "failed to read QM_VF_STATE!\n"); > + return ret; > + } > + > return 0; > } > > @@ -505,6 +513,12 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > qm->qp_base = vf_data->qp_base; > qm->qp_num = vf_data->qp_num; > > + if (!vf_data->eqe_dma || !vf_data->aeqe_dma || > + !vf_data->sqc_dma || !vf_data->cqc_dma) { > + dev_err(dev, "resume dma addr is NULL!\n"); > + return -EINVAL; > + } > + > ret = qm_set_regs(qm, vf_data); > if (ret) { > dev_err(dev, "set VF regs failed\n"); > @@ -727,6 +741,9 @@ static int hisi_acc_vf_load_state(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > struct hisi_acc_vf_migration_file *migf = hisi_acc_vdev- > >resuming_migf; > int ret; > > + if (hisi_acc_vdev->vf_qm_state != QM_READY) > + return 0; > + > /* Recover data to VF */ > ret = vf_qm_load_data(hisi_acc_vdev, migf); > if (ret) { > @@ -1530,6 +1547,7 @@ static int hisi_acc_vfio_pci_migrn_init_dev(struct > vfio_device *core_vdev) > hisi_acc_vdev->vf_id = pci_iov_vf_id(pdev) + 1; > hisi_acc_vdev->pf_qm = pf_qm; > hisi_acc_vdev->vf_dev = pdev; > + hisi_acc_vdev->vf_qm_state = QM_NOT_READY; > mutex_init(&hisi_acc_vdev->state_mutex); > mutex_init(&hisi_acc_vdev->open_mutex); > > -- > 2.24.0

1 0

Re: [PATCH v4 1/5] hisi_acc_vfio_pci: fix XQE dma address error
by Shameerali Kolothum Thodi 26 Feb '25

26 Feb '25

> -----Original Message----- > From: Alex Williamson <alex.williamson(a)redhat.com> > Sent: Wednesday, February 26, 2025 12:10 AM > To: liulongfang <liulongfang(a)huawei.com> > Cc: jgg(a)nvidia.com; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com>; kvm(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > Subject: Re: [PATCH v4 1/5] hisi_acc_vfio_pci: fix XQE dma address error > > On Tue, 25 Feb 2025 14:27:53 +0800 > Longfang Liu <liulongfang(a)huawei.com> wrote: > > > 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(a)huawei.com> > > --- > > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 40 ++++++++++++++++--- > > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 14 ++++++- > > 2 files changed, 46 insertions(+), 8 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..35316984089b 100644 > > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > > @@ -350,6 +350,31 @@ 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_version_check(struct acc_vf_data *vf_data, struct device > *dev) > > +{ > > + switch (vf_data->acc_magic) { > > + case ACC_DEV_MAGIC_V2: > > + if (vf_data->major_ver < ACC_DRV_MAJOR_VER || > > + vf_data->minor_ver < ACC_DRV_MINOR_VER) > > + dev_info(dev, "migration driver version not > match!\n"); > > + return -EINVAL; > > + break; > > What's your major/minor update strategy? > > Note that minor_ver is a u16 and ACC_DRV_MINOR_VER is defined as 0, so > testing less than 0 against an unsigned is useless. > > Arguably testing major and minor independently is pretty useless too. > > You're defining what a future "old" driver version will accept, which > is very nearly anything, so any breaking change *again* requires a new > magic, so we're accomplishing very little here. > > Maybe you want to consider a strategy where you'd increment the major > number for a breaking change and minor for a compatible feature. In > that case you'd want to verify the major_ver matches > ACC_DRV_MAJOR_VER > exactly and minor_ver would be more of a feature level. Agree. I think the above check should be just major_ver != ACC_DRV_MAJOR_VER and we can make use of minor version whenever we need a specific handling for a feature support. Also I think it would be good to print the version info above in case of mismatch. > > > + 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 +388,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_version_check(vf_data, dev); > > + if (ret) { > > dev_err(dev, "failed to match ACC_DEV_MAGIC\n"); > > return -EINVAL; > > } > > @@ -418,7 +444,9 @@ 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; > > + vf_data->major_ver = ACC_DRV_MAR; > > + vf_data->minor_ver = ACC_DRV_MIN; > > Where are "MAR" and "MIN" defined? I can't see how this would even > compile. Thanks, Yes. Please make sure to do a compile test and run basic sanity tested even if you think the changes are minor. Chances are there that you will miss out things like this. Thanks, Shameer

1 0

Re: [PATCH 3/3] migration: adapt to new migration configuration
by Shameerali Kolothum Thodi 18 Feb '25

18 Feb '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, February 18, 2025 2:15 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH 3/3] migration: adapt to new migration configuration > > On the new hardware platform, the migration region is moved from > VF to PF. the driver also needs to be modified accordingly to adapt > to the new hardware device. > > Use PF's io base directly on the new hardware platform. and no mmap > operation is required. > If it is on an old platform, the driver needs to be compatible with > the old solution. I see that you have used "new hardware platform" everywhere and I think it is better we make it more specific, like QM_HW_V4? Also it is not clear to me, what happens if you try migration between QM_HW_V3 <--> QM_HW_V4? Is that going to be success or we are failing it somewhere? I see only the dev_id check in vf_qm_check_match(). Thanks, Shameer > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 165 ++++++++++++------ > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 7 + > 2 files changed, 119 insertions(+), 53 deletions(-) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 599905dbb707..cf5a807c2199 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -125,6 +125,72 @@ static int qm_get_cqc(struct hisi_qm *qm, u64 > *addr) > return 0; > } > > +static int qm_get_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_read_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_read_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to read QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int qm_set_xqc_regs(struct hisi_acc_vf_core_device *hisi_acc_vdev, > + struct acc_vf_data *vf_data) > +{ > + struct hisi_qm *qm = &hisi_acc_vdev->vf_qm; > + struct device *dev = &qm->pdev->dev; > + u32 eqc_addr, aeqc_addr; > + int ret; > + > + if (qm->ver == QM_HW_V3) { > + eqc_addr = QM_EQC_DW0; > + aeqc_addr = QM_AEQC_DW0; > + } else { > + eqc_addr = QM_EQC_PF_DW0; > + aeqc_addr = QM_AEQC_PF_DW0; > + } > + > + /* QM_EQC_DW has 7 regs */ > + ret = qm_write_regs(qm, eqc_addr, vf_data->qm_eqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_EQC_DW\n"); > + return ret; > + } > + > + /* QM_AEQC_DW has 7 regs */ > + ret = qm_write_regs(qm, aeqc_addr, vf_data->qm_aeqc_dw, 7); > + if (ret) { > + dev_err(dev, "failed to write QM_AEQC_DW\n"); > + return ret; > + } > + > + return 0; > +} > + > static int qm_get_regs(struct hisi_qm *qm, struct acc_vf_data *vf_data) > { > struct device *dev = &qm->pdev->dev; > @@ -167,20 +233,6 @@ static int qm_get_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to read QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_read_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to read QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -238,20 +290,6 @@ static int qm_set_regs(struct hisi_qm *qm, struct > acc_vf_data *vf_data) > return ret; > } > > - /* QM_EQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_EQC_DW0, vf_data->qm_eqc_dw, 7); > - if (ret) { > - dev_err(dev, "failed to write QM_EQC_DW\n"); > - return ret; > - } > - > - /* QM_AEQC_DW has 7 regs */ > - ret = qm_write_regs(qm, QM_AEQC_DW0, vf_data->qm_aeqc_dw, > 7); > - if (ret) { > - dev_err(dev, "failed to write QM_AEQC_DW\n"); > - return ret; > - } > - > return 0; > } > > @@ -470,6 +508,10 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + ret = qm_set_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return ret; > + > ret = hisi_qm_mb(qm, QM_MB_CMD_SQC_BT, qm->sqc_dma, 0, 0); > if (ret) { > dev_err(dev, "set sqc failed\n"); > @@ -544,6 +586,10 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + ret = qm_get_xqc_regs(hisi_acc_vdev, vf_data); > + if (ret) > + return -EINVAL; > + > ret = vf_qm_read_data(vf_qm, vf_data); > if (ret) > return -EINVAL; > @@ -1131,34 +1177,46 @@ static int hisi_acc_vf_qm_init(struct > hisi_acc_vf_core_device *hisi_acc_vdev) > { > struct vfio_pci_core_device *vdev = &hisi_acc_vdev->core_device; > struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > + struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > struct pci_dev *vf_dev = vdev->pdev; > > - /* > - * ACC VF dev BAR2 region consists of both functional register space > - * and migration control register space. For migration to work, we > - * need access to both. Hence, we map the entire BAR2 region here. > - * But unnecessarily exposing the migration BAR region to the Guest > - * has the potential to prevent/corrupt the Guest migration. Hence, > - * we restrict access to the migration control space from > - * Guest(Please see mmap/ioctl/read/write override functions). > - * > - * Please note that it is OK to expose the entire VF BAR if migration > - * is not supported or required as this cannot affect the ACC PF > - * configurations. > - * > - * Also the HiSilicon ACC VF devices supported by this driver on > - * HiSilicon hardware platforms are integrated end point devices > - * and the platform lacks the capability to perform any PCIe P2P > - * between these devices. > - */ > + if (pf_qm->ver == QM_HW_V3) { > + /* > + * ACC VF dev BAR2 region consists of both functional > register space > + * and migration control register space. For migration to > work, we > + * need access to both. Hence, we map the entire BAR2 > region here. > + * But unnecessarily exposing the migration BAR region to > the Guest > + * has the potential to prevent/corrupt the Guest migration. > Hence, > + * we restrict access to the migration control space from > + * Guest(Please see mmap/ioctl/read/write override > functions). > + * > + * Please note that it is OK to expose the entire VF BAR if > migration > + * is not supported or required as this cannot affect the ACC > PF > + * configurations. > + * > + * Also the HiSilicon ACC VF devices supported by this driver > on > + * HiSilicon hardware platforms are integrated end point > devices > + * and the platform lacks the capability to perform any PCIe > P2P > + * between these devices. > + */ > > - vf_qm->io_base = > - ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > - pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > - if (!vf_qm->io_base) > - return -EIO; > + vf_qm->io_base = > + ioremap(pci_resource_start(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX), > + pci_resource_len(vf_dev, > VFIO_PCI_BAR2_REGION_INDEX)); > + if (!vf_qm->io_base) > + return -EIO; > > - vf_qm->fun_type = QM_HW_VF; > + vf_qm->fun_type = QM_HW_VF; > + vf_qm->ver = pf_qm->ver; > + } else { > + /* > + * In the new HW platform, the migration function register > space is in BAR2 of PF, > + * and each VF occupies 8KB address space. > + */ > + vf_qm->io_base = pf_qm->io_base + > QM_MIG_REGION_OFFSET + > + hisi_acc_vdev->vf_id * > QM_MIG_REGION_SIZE; > + vf_qm->fun_type = QM_HW_PF; > + } > vf_qm->pdev = vf_dev; > mutex_init(&vf_qm->mailbox_lock); > > @@ -1488,7 +1546,8 @@ static void hisi_acc_vfio_pci_close_device(struct > vfio_device *core_vdev) > > mutex_lock(&hisi_acc_vdev->open_mutex); > hisi_acc_vdev->dev_opened = false; > - iounmap(vf_qm->io_base); > + if (vf_qm->ver == QM_HW_V3) > + iounmap(vf_qm->io_base); > mutex_unlock(&hisi_acc_vdev->open_mutex); > vfio_pci_core_close_device(core_vdev); > } > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > index 245d7537b2bc..b01eb54525d3 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > @@ -50,6 +50,13 @@ > #define QM_EQC_DW0 0X8000 > #define QM_AEQC_DW0 0X8020 > > +#define QM_MIG_REGION_OFFSET 0x180000 > +#define QM_MIG_REGION_SIZE 0x2000 > + > +#define QM_SUB_VERSION_ID 0x100210 > +#define QM_EQC_PF_DW0 0x1c00 > +#define QM_AEQC_PF_DW0 0x1c20 > + > struct acc_vf_data { > #define QM_MATCH_SIZE offsetofend(struct acc_vf_data, qm_rsv_state) > /* QM match information */ > -- > 2.24.0

1 0

Re: [PATCH 1/3] migration: update BAR space size
by Shameerali Kolothum Thodi 18 Feb '25

18 Feb '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, February 18, 2025 2:15 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH 1/3] migration: update BAR space size > > On the new hardware platform, the live migration configuration region > is moved from VF to PF. The VF's own configuration space is > restored to the complete 64KB, and there is no need to divide the > size of the BAR configuration space equally. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 41 +++++++++++++++---- > 1 file changed, 32 insertions(+), 9 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..599905dbb707 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -1195,6 +1195,33 @@ static struct hisi_qm *hisi_acc_get_pf_qm(struct > pci_dev *pdev) > return !IS_ERR(pf_qm) ? pf_qm : NULL; > } > > +static size_t hisi_acc_get_resource_len(struct vfio_pci_core_device *vdev, > + unsigned int index) > +{ > + struct hisi_acc_vf_core_device *hisi_acc_vdev = > + hisi_acc_drvdata(vdev->pdev); > + > + /* > + * ACC VF dev 64KB BAR2 region consists of both functional > + * register space and migration control register space, each > + * uses 32KB BAR2 region, on the system with more than 64KB > + * page size, even if the migration control register space > + * is written by VM, it will only affects the VF. > + * > + * In order to support the live migration function in the > + * system with a page size above 64KB, the driver needs > + * to ensure that the VF region size is aligned with the > + * system page size. I didn't get this. Are you referring to kernel with 64K page size? And this is for new hardware or QM_HW_V3 one? > + * > + * On the new hardware platform, the live migration control register > + * has been moved from VF to PF. > + */ > + if (hisi_acc_vdev->pf_qm->ver == QM_HW_V3) > + return (pci_resource_len(vdev->pdev, index) >> 1); > + > + return pci_resource_len(vdev->pdev, index); > +} > + > static int hisi_acc_pci_rw_access_check(struct vfio_device *core_vdev, > size_t count, loff_t *ppos, > size_t *new_count) > @@ -1205,8 +1232,9 @@ static int hisi_acc_pci_rw_access_check(struct > vfio_device *core_vdev, > > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = hisi_acc_get_resource_len(vdev, index); > /* Check if access is for migration control region */ > if (pos >= end) > return -EINVAL; > @@ -1227,8 +1255,9 @@ static int hisi_acc_vfio_pci_mmap(struct > vfio_device *core_vdev, > index = vma->vm_pgoff >> (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT); > if (index == VFIO_PCI_BAR2_REGION_INDEX) { > u64 req_len, pgoff, req_start; > - resource_size_t end = pci_resource_len(vdev->pdev, index) / > 2; > + resource_size_t end; > > + end = PAGE_ALIGN(hisi_acc_get_resource_len(vdev, index)); So here, the whole BAR2 will be mapped to Guest in case of QM_HW_V3 && 64K kernel as well, right? Thanks, Shameer > req_len = vma->vm_end - vma->vm_start; > pgoff = vma->vm_pgoff & > ((1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1); > @@ -1275,7 +1304,6 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (cmd == VFIO_DEVICE_GET_REGION_INFO) { > struct vfio_pci_core_device *vdev = > container_of(core_vdev, struct vfio_pci_core_device, > vdev); > - struct pci_dev *pdev = vdev->pdev; > struct vfio_region_info info; > unsigned long minsz; > > @@ -1290,12 +1318,7 @@ static long hisi_acc_vfio_pci_ioctl(struct > vfio_device *core_vdev, unsigned int > if (info.index == VFIO_PCI_BAR2_REGION_INDEX) { > info.offset = > VFIO_PCI_INDEX_TO_OFFSET(info.index); > > - /* > - * ACC VF dev BAR2 region consists of both > functional > - * register space and migration control register > space. > - * Report only the functional region to Guest. > - */ > - info.size = pci_resource_len(pdev, info.index) / 2; > + info.size = hisi_acc_get_resource_len(vdev, > info.index); > > info.flags = VFIO_REGION_INFO_FLAG_READ | > VFIO_REGION_INFO_FLAG_WRITE | > -- > 2.24.0

1 0

Re: [PATCH v2 1/5] hisi_acc_vfio_pci: fix XQE dma address error
by Shameerali Kolothum Thodi 14 Jan '25

14 Jan '25

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Tuesday, January 14, 2025 3:18 AM > To: Alex Williamson <alex.williamson(a)redhat.com> > Cc: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi(a)huawei.com>; jgg(a)nvidia.com; Jonathan > Cameron <jonathan.cameron(a)huawei.com>; kvm(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > Subject: Re: [PATCH v2 1/5] hisi_acc_vfio_pci: fix XQE dma address error > [...] > > @@ -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? The suggestion here is to improve my previous plan.. ie, instead of overloading the V2 MAGIC with version info, introduce version(major:minor) separately. Something like, Rename old MAGIC as MAGIC_V1 Introduce new MAGIC as MAGIC_V2 Repurpose any padding or reserved fields in struct vf_data for major:minor version fields. The idea of introducing these major:minor is for future use where instead of coming up with a new MAGIC data every time we can increment the version for bug fixes and features if required. Hope this is clear now. Thanks, Shameer

1 0

Re: [PATCH v2 0/5] bugfix some driver issues
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

Hi Longfang, > -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 0/5] bugfix some driver issues > > As the test scenarios for the live migration function become > more and more extensive. Some previously undiscovered driver > issues were found. > Update and fix through this patchset. > A changes history will be beneficial to understand what changed from v1. Also I have a request.... As part of this series, could you please include the below fix to this driver as well as we do have the same potential problem. https://lore.kernel.org/all/20241021123843.42979-1-giovanni.cabiddu@intel.c… Thanks, Shameer > Longfang Liu (5): > hisi_acc_vfio_pci: fix XQE dma address error > hisi_acc_vfio_pci: add eq and aeq interruption restore > hisi_acc_vfio_pci: bugfix cache write-back issue > hisi_acc_vfio_pci: bugfix the problem of uninstalling driver > hisi_acc_vfio_pci: bugfix live migration function without VF device > driver > > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 83 ++++++++++++++++--- > .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 9 +- > 2 files changed, 78 insertions(+), 14 deletions(-) > > -- > 2.24.0

1 0

Re: [PATCH v2 5/5] hisi_acc_vfio_pci: bugfix live migration function without VF device driver
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 5/5] hisi_acc_vfio_pci: bugfix live migration function > without VF device driver > > If the driver of the VF device is not loaded in the Guest OS, > then perform device data migration. The migrated data address will > be NULL. > The live migration recovery operation on the destination side will > access a null address value, which will cause access errors. > > Therefore, live migration of VMs without added VF device drivers > does not require device data migration. > In addition, when the queue address data obtained by the destination > is empty, device queue recovery processing will not be performed. > > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> Why this doesn't need a Fixes tag? > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 8d9e07ebf4fd..9a5f7e9bc695 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -436,6 +436,7 @@ static int vf_qm_get_match_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > struct acc_vf_data *vf_data) > { > struct hisi_qm *pf_qm = hisi_acc_vdev->pf_qm; > + struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > struct device *dev = &pf_qm->pdev->dev; > int vf_id = hisi_acc_vdev->vf_id; > int ret; > @@ -460,6 +461,13 @@ static int vf_qm_get_match_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return ret; > } > > + /* Get VF driver insmod state */ > + ret = qm_read_regs(vf_qm, QM_VF_STATE, &vf_data->vf_qm_state, > 1); > + if (ret) { > + dev_err(dev, "failed to read QM_VF_STATE!\n"); > + return ret; > + } > + > return 0; > } > > @@ -499,6 +507,12 @@ static int vf_qm_load_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > qm->qp_base = vf_data->qp_base; > qm->qp_num = vf_data->qp_num; > > + if (!vf_data->eqe_dma || !vf_data->aeqe_dma || > + !vf_data->sqc_dma || !vf_data->cqc_dma) { > + dev_err(dev, "resume dma addr is NULL!\n"); > + return -EINVAL; > + } > + So this is to cover the corner case where the Guest has loaded the driver (QM_READY set) but not configured the DMA address? When this will happen? I thought we are setting QM_READY in guest after all configurations. Thanks, Shameer

1 0

Re: [PATCH v2 4/5] hisi_acc_vfio_pci: bugfix the problem of uninstalling driver
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 4/5] hisi_acc_vfio_pci: bugfix the problem of uninstalling > driver > > In a live migration scenario. If the number of VFs at the destination is > greater than the source, the recovery operation will fail and qemu will not > be able to complete the process and exit after shutting down the device FD. > > This will cause the driver to be unable to be unloaded normally due to > abnormal reference counting of the live migration driver caused by the > abnormal closing operation of fd. > > Fixes:b0eed085903e("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index c057c0e24693..8d9e07ebf4fd 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -1501,6 +1501,7 @@ static void hisi_acc_vfio_pci_close_device(struct > vfio_device *core_vdev) > struct hisi_acc_vf_core_device *hisi_acc_vdev = > hisi_acc_get_vf_dev(core_vdev); > struct hisi_qm *vf_qm = &hisi_acc_vdev->vf_qm; > > + hisi_acc_vf_disable_fds(hisi_acc_vdev); > mutex_lock(&hisi_acc_vdev->open_mutex); > hisi_acc_vdev->dev_opened = false; > iounmap(vf_qm->io_base); Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer

1 0

Re: [PATCH v2 3/5] hisi_acc_vfio_pci: bugfix cache write-back issue
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 3/5] hisi_acc_vfio_pci: bugfix cache write-back issue > > At present, cache write-back is placed in the device data > copy stage after stopping the device operation. > Writing back to the cache at this stage will cause the data > obtained by the cache to be written back to be empty. > > In order to ensure that the cache data is written back > successfully, the data needs to be written back into the > stop device stage. > > Fixes:b0eed085903e("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 14 +++++++------- > 1 file changed, 7 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 4c8f1ae5b636..c057c0e24693 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -559,7 +559,6 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > { > 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))) { > @@ -573,12 +572,6 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > 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 = vf_qm_read_data(vf_qm, vf_data); > if (ret) > return -EINVAL; > @@ -1005,6 +998,13 @@ static int hisi_acc_vf_stop_device(struct > hisi_acc_vf_core_device *hisi_acc_vdev > dev_err(dev, "failed to check QM INT state!\n"); > return ret; > } > + > + ret = vf_qm_cache_wb(vf_qm); > + if (ret) { > + dev_err(dev, "failed to writeback QM cache!\n"); > + return ret; > + } > + > return 0; > } Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer

1 0

Re: [PATCH v2 2/5] hisi_acc_vfio_pci: add eq and aeq interruption restore
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > Subject: [PATCH v2 2/5] hisi_acc_vfio_pci: add eq and aeq interruption > restore > > In order to ensure that the task packets of the accelerator > device are not lost during the migration process, it is necessary > to send an EQ and AEQ command to the device after the live migration > is completed and to update the completion position of the task queue. > > Let the device recheck the completed tasks data and if there are > uncollected packets, device resend a task completion interrupt > to the software. > > Fixes:b0eed085903e("hisi_acc_vfio_pci: Add support for VFIO live > migration") > Signed-off-by: Longfang Liu <liulongfang(a)huawei.com> > --- > drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > index 8518efea3a52..4c8f1ae5b636 100644 > --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > @@ -463,6 +463,19 @@ static int vf_qm_get_match_data(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return 0; > } > > +static void vf_qm_xeqc_save(struct hisi_qm *qm, > + struct hisi_acc_vf_migration_file *migf) > +{ > + struct acc_vf_data *vf_data = &migf->vf_data; > + u16 eq_head, aeq_head; > + > + eq_head = vf_data->qm_eqc_dw[0] & 0xFFFF; > + qm_db(qm, 0, QM_DOORBELL_CMD_EQ, eq_head, 0); > + > + aeq_head = vf_data->qm_aeqc_dw[0] & 0xFFFF; > + qm_db(qm, 0, QM_DOORBELL_CMD_AEQ, aeq_head, 0); > +} > + > static int vf_qm_load_data(struct hisi_acc_vf_core_device *hisi_acc_vdev, > struct hisi_acc_vf_migration_file *migf) > { > @@ -571,6 +584,9 @@ static int vf_qm_state_save(struct > hisi_acc_vf_core_device *hisi_acc_vdev, > return -EINVAL; > > migf->total_length = sizeof(struct acc_vf_data); > + /* Save eqc and aeqc interrupt information */ > + vf_qm_xeqc_save(vf_qm, migf); > + > return 0; > } Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Thanks, Shameer

1 0

Re: [PATCH v2 1/5] hisi_acc_vfio_pci: fix XQE dma address error
by Shameerali Kolothum Thodi 19 Dec '24

19 Dec '24

> -----Original Message----- > From: liulongfang <liulongfang(a)huawei.com> > Sent: Thursday, December 19, 2024 9:18 AM > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > <jonathan.cameron(a)huawei.com> > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > 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(a)huawei.com> > --- > .../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. Thanks, Shameer

1 0

Re: [PATCH v15 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon migration driver
by Shameerali Kolothum Thodi 13 Nov '24

13 Nov '24

> -----Original Message----- > From: Alex Williamson <alex.williamson(a)redhat.com> > Sent: Tuesday, November 12, 2024 9:51 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> > Cc: liulongfang <liulongfang(a)huawei.com>; jgg(a)nvidia.com; Jonathan > Cameron <jonathan.cameron(a)huawei.com>; kvm(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org; linuxarm(a)openeuler.org > Subject: Re: [PATCH v15 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > migration driver > > On Tue, 12 Nov 2024 08:40:03 +0000 > Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com> > wrote: > > > > -----Original Message----- > > > From: liulongfang <liulongfang(a)huawei.com> > > > Sent: Tuesday, November 12, 2024 7:33 AM > > > To: alex.williamson(a)redhat.com; jgg(a)nvidia.com; Shameerali Kolothum > > > Thodi <shameerali.kolothum.thodi(a)huawei.com>; Jonathan Cameron > > > <jonathan.cameron(a)huawei.com> > > > Cc: kvm(a)vger.kernel.org; linux-kernel(a)vger.kernel.org; > > > linuxarm(a)openeuler.org; liulongfang <liulongfang(a)huawei.com> > > > Subject: [PATCH v15 3/4] hisi_acc_vfio_pci: register debugfs for hisilicon > > > migration driver > > > > > > > > > +static void hisi_acc_vfio_debug_init(struct hisi_acc_vf_core_device > > > *hisi_acc_vdev) > > > +{ > > > + struct vfio_device *vdev = &hisi_acc_vdev->core_device.vdev; > > > + struct hisi_acc_vf_migration_file *migf = NULL; > > > + struct dentry *vfio_dev_migration = NULL; > > > + struct dentry *vfio_hisi_acc = NULL; > > > > Nit, I think we can get rid of these NULL initializations. > > Yup, all three are unnecessary. > > > If you have time, please consider respin (sorry, missed this in earlier > reviews.) > > If that's the only comment, I can fix that on commit if you want to add > an ack/review conditional on that change. Thanks, Thanks Alex. With the above nits addressed, Reviewed-by: Shameer Kolothum <shameerali.kolothum.thodi(a)huawei.com> Shameer

1 0