From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C5A951531C2 for ; Mon, 13 Jan 2025 13:34:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736775300; cv=none; b=sKTzAeWYOkLtnn9ZAfDSATqSNU8JcdumDgr78S8HlpWnM94IwNkHbHwpFeZkiqYOTF5plTBNk2XufFX/WWPlywWi6iwtOEkHe9dQQAgVDjkmTu18L4IbDR1fEGJmgToo2XV7vxzD8xBhFLeeUFERpGgHrqlzMi7b/G7mbsbET0s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736775300; c=relaxed/simple; bh=tyY/mwl+w0TM6APM6p8YfZXtcSdDWEejjltd7q1H1o4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=sQFKiH3DmNW3M2PqgkuWXfZmhz4dUwAB8qcYfZdkq/Pkq2K6PTvNgvoncD9NWK6h5cP/+GNNOkPlDcUvNrHwzQ9bFZ+dYRNW482PSoIwAGpPyyMG/5D/shLVsunnoBfAPZaKxJvgbF24lnFDx0pckBuaMOc5NXiIR49HVY1jhSE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=UW0ZBHdH; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="UW0ZBHdH" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1736775297; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7Wh+orv/LYf7JgyoDOHd4i4UK6ygNBWZmqX04TPWjF0=; b=UW0ZBHdHP0G9YR1ipE17ghFxcJ1Ln01OWZaD9WQRm8jYZc/ft2Rh0ZiRK8dJ+3iliHlrks LzysO7UtNc4tAq4XbDB6cht3bB62JJXWYPvpQ6S90f0wzK+mGlxWyjnyh2z54HXEn+07TA oJRy1fEvC2Bwv4Y0Zhsz/ZvIFIRrrCo= Received: from mail-io1-f71.google.com (mail-io1-f71.google.com [209.85.166.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-675-w4JdpGF3OMC49k619OWs_A-1; Mon, 13 Jan 2025 08:34:54 -0500 X-MC-Unique: w4JdpGF3OMC49k619OWs_A-1 X-Mimecast-MFC-AGG-ID: w4JdpGF3OMC49k619OWs_A Received: by mail-io1-f71.google.com with SMTP id ca18e2360f4ac-844df5d5edfso18665539f.1 for ; Mon, 13 Jan 2025 05:34:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736775294; x=1737380094; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=7Wh+orv/LYf7JgyoDOHd4i4UK6ygNBWZmqX04TPWjF0=; b=Uj57En41Pd4xN955BcIdegcTSC0JS8ILzswq901bAdpNplw+/Eig0kS5H1emI2J38W La7j6LdedRMGgei0DOUThsWBu1pjHTRM0C00L8hBgo1anqpFJV3EpFxEIrIZWFwi9a2S fMpmai3AVChI71db3S5vh5pssYf+69IBqwu1mA0R2Q6dhHDTEYpBj2pfcdOjY15XORvu NY3vaHjWbMFbb8pCOQrhIbPnkhEnC2DYl7OcuA0huOMBWuKJUaktVk3wTTsKVhXkd7hp /hWsisjRwHekGDtXJhDlwOwr38/b5brmpxLu35hfbC+6yjPx1UePujS59l/Ugz9AhpHk TiFQ== X-Forwarded-Encrypted: i=1; AJvYcCXujDYlbZteAu4JYi+D1/n7r8+J2OxuBbFp1Fdb2brjjaPO0wJs58mqMYbWDG96falRQnHrpV1ljQ3+dtw=@vger.kernel.org X-Gm-Message-State: AOJu0YwFKaqTfyVoIyQ2ryj2FM/pA600/Zhr5/yxm9d60wj8Dbde3yJw FaaD7l3a2q/S+4lfXxyOqyy3eVGL2NC4rqatRfD3K+Su6nP9aqpbjQIuhv8H/HviN9Lb6VW8Sbi KIV7c+W0C4pkbaO2pgC4HmfvdwcKq60z0zNF0wnoWq3/KL1BCMes23ymVyxUhsA== X-Gm-Gg: ASbGnct7XIwbf7oDSYmHL0XEvBTCgqmSEoGOAw/cteylqrq0fyHCqb77ykxvNcaIxDx a2zm3diMBVtssvV6wRbnERzxDxsocA6HmKsMJ7m96mIeea2yJKAmqLgl9WAXtDm0g7VL+fs3BfM fVqM+Qbj+DddTAH/Vq1GuGDBzDyg2So9UDruZia9pHfw33q4bPEbvvPj4v1w1iUD3ldL3468vmj a7j8xjvMzi5V0PXLWRHFzaE5+C/CTV81HtwsY2rFNrVlXlzl47U/tTiKD46 X-Received: by 2002:a05:6602:27cb:b0:83a:e6b2:be3 with SMTP id ca18e2360f4ac-84cdfde7bc7mr529500539f.0.1736775293750; Mon, 13 Jan 2025 05:34:53 -0800 (PST) X-Google-Smtp-Source: AGHT+IEjnk3QgDRJCgvEwyINp4QXsbEBSoc6YKN9QzOcCBG9wNe41jclREALhrCqtuAgkYV8fx2jyQ== X-Received: by 2002:a05:6602:27cb:b0:83a:e6b2:be3 with SMTP id ca18e2360f4ac-84cdfde7bc7mr529499939f.0.1736775293426; Mon, 13 Jan 2025 05:34:53 -0800 (PST) Received: from redhat.com ([38.15.36.11]) by smtp.gmail.com with ESMTPSA id ca18e2360f4ac-84d61fa3d8asm261995839f.31.2025.01.13.05.34.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jan 2025 05:34:52 -0800 (PST) Date: Mon, 13 Jan 2025 08:34:41 -0500 From: Alex Williamson To: liulongfang Cc: Shameerali Kolothum Thodi , "jgg@nvidia.com" , Jonathan Cameron , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linuxarm@openeuler.org" Subject: Re: [PATCH v2 1/5] hisi_acc_vfio_pci: fix XQE dma address error Message-ID: <20250113083441.0fb7afa3.alex.williamson@redhat.com> In-Reply-To: <4d48db10-23e1-1bb4-54ea-4c659ab85f19@huawei.com> References: <20241219091800.41462-1-liulongfang@huawei.com> <20241219091800.41462-2-liulongfang@huawei.com> <099e0e1215f34d64a4ae698b90ee372c@huawei.com> <20250102153008.301730f3.alex.williamson@redhat.com> <4d48db10-23e1-1bb4-54ea-4c659ab85f19@huawei.com> Organization: Red Hat Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 13 Jan 2025 17:43:09 +0800 liulongfang wrote: > On 2025/1/3 6:30, Alex Williamson wrote: > > On Thu, 19 Dec 2024 10:01:03 +0000 > > Shameerali Kolothum Thodi wrote: > > > >>> -----Original Message----- > >>> From: liulongfang > >>> Sent: Thursday, December 19, 2024 9:18 AM > >>> To: alex.williamson@redhat.com; jgg@nvidia.com; Shameerali Kolothum > >>> Thodi ; Jonathan Cameron > >>> > >>> Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; > >>> linuxarm@openeuler.org; liulongfang > >>> 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 > >>> --- > >>> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.c | 34 +++++++++++++++---- > >>> .../vfio/pci/hisilicon/hisi_acc_vfio_pci.h | 9 ++++- > >>> 2 files changed, 36 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > >>> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > >>> index 451c639299eb..8518efea3a52 100644 > >>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > >>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c > >>> @@ -350,6 +350,27 @@ static int vf_qm_func_stop(struct hisi_qm *qm) > >>> return hisi_qm_mb(qm, QM_MB_CMD_PAUSE_QM, 0, 0, 0); > >>> } > >>> > >>> +static int vf_qm_magic_check(struct acc_vf_data *vf_data) > >>> +{ > >>> + switch (vf_data->acc_magic) { > >>> + case ACC_DEV_MAGIC_V2: > >>> + break; > >>> + case ACC_DEV_MAGIC_V1: > >>> + /* Correct dma address */ > >>> + vf_data->eqe_dma = vf_data- > >>>> qm_eqc_dw[QM_XQC_ADDR_HIGH]; > >>> + vf_data->eqe_dma <<= QM_XQC_ADDR_OFFSET; > >>> + vf_data->eqe_dma |= vf_data- > >>>> qm_eqc_dw[QM_XQC_ADDR_LOW]; > >>> + vf_data->aeqe_dma = vf_data- > >>>> qm_aeqc_dw[QM_XQC_ADDR_HIGH]; > >>> + vf_data->aeqe_dma <<= QM_XQC_ADDR_OFFSET; > >>> + vf_data->aeqe_dma |= vf_data- > >>>> qm_aeqc_dw[QM_XQC_ADDR_LOW]; > >>> + break; > >>> + default: > >>> + return -EINVAL; > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int vf_qm_check_match(struct hisi_acc_vf_core_device > >>> *hisi_acc_vdev, > >>> struct hisi_acc_vf_migration_file *migf) > >>> { > >>> @@ -363,7 +384,8 @@ static int vf_qm_check_match(struct > >>> hisi_acc_vf_core_device *hisi_acc_vdev, > >>> if (migf->total_length < QM_MATCH_SIZE || hisi_acc_vdev- > >>>> match_done) > >>> return 0; > >>> > >>> - if (vf_data->acc_magic != ACC_DEV_MAGIC) { > >>> + ret = vf_qm_magic_check(vf_data); > >>> + if (ret) { > >>> dev_err(dev, "failed to match ACC_DEV_MAGIC\n"); > >>> return -EINVAL; > >>> } > >>> @@ -418,7 +440,7 @@ static int vf_qm_get_match_data(struct > >>> hisi_acc_vf_core_device *hisi_acc_vdev, > >>> int vf_id = hisi_acc_vdev->vf_id; > >>> int ret; > >>> > >>> - vf_data->acc_magic = ACC_DEV_MAGIC; > >>> + vf_data->acc_magic = ACC_DEV_MAGIC_V2; > >>> /* Save device id */ > >>> vf_data->dev_id = hisi_acc_vdev->vf_dev->device; > >>> > >>> @@ -496,12 +518,12 @@ static int vf_qm_read_data(struct hisi_qm > >>> *vf_qm, struct acc_vf_data *vf_data) > >>> return -EINVAL; > >>> > >>> /* Every reg is 32 bit, the dma address is 64 bit. */ > >>> - vf_data->eqe_dma = vf_data->qm_eqc_dw[1]; > >>> + vf_data->eqe_dma = vf_data->qm_eqc_dw[QM_XQC_ADDR_HIGH]; > >>> vf_data->eqe_dma <<= QM_XQC_ADDR_OFFSET; > >>> - vf_data->eqe_dma |= vf_data->qm_eqc_dw[0]; > >>> - vf_data->aeqe_dma = vf_data->qm_aeqc_dw[1]; > >>> + vf_data->eqe_dma |= vf_data->qm_eqc_dw[QM_XQC_ADDR_LOW]; > >>> + vf_data->aeqe_dma = vf_data- > >>>> qm_aeqc_dw[QM_XQC_ADDR_HIGH]; > >>> vf_data->aeqe_dma <<= QM_XQC_ADDR_OFFSET; > >>> - vf_data->aeqe_dma |= vf_data->qm_aeqc_dw[0]; > >>> + vf_data->aeqe_dma |= vf_data- > >>>> qm_aeqc_dw[QM_XQC_ADDR_LOW]; > >>> > >>> /* Through SQC_BT/CQC_BT to get sqc and cqc address */ > >>> ret = qm_get_sqc(vf_qm, &vf_data->sqc_dma); > >>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > >>> b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > >>> index 245d7537b2bc..2afce68f5a34 100644 > >>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > >>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.h > >>> @@ -39,6 +39,9 @@ > >>> #define QM_REG_ADDR_OFFSET 0x0004 > >>> > >>> #define QM_XQC_ADDR_OFFSET 32U > >>> +#define QM_XQC_ADDR_LOW 0x1 > >>> +#define QM_XQC_ADDR_HIGH 0x2 > >>> + > >>> #define QM_VF_AEQ_INT_MASK 0x0004 > >>> #define QM_VF_EQ_INT_MASK 0x000c > >>> #define QM_IFC_INT_SOURCE_V 0x0020 > >>> @@ -50,10 +53,14 @@ > >>> #define QM_EQC_DW0 0X8000 > >>> #define QM_AEQC_DW0 0X8020 > >>> > >>> +enum acc_magic_num { > >>> + ACC_DEV_MAGIC_V1 = 0XCDCDCDCDFEEDAACC, > >>> + ACC_DEV_MAGIC_V2 = 0xAACCFEEDDECADEDE, > >> > >> > >> I think we have discussed this before that having some kind of > >> version info embed into magic_num will be beneficial going > >> forward. ie, may be use the last 4 bytes for denoting version. > >> > >> ACC_DEV_MAGIC_V2 = 0xAACCFEEDDECA0002 > >> > >> The reason being, otherwise we have to come up with a random > >> magic each time when a fix like this is required in future. > > > > Overloading the magic value like this is a bit strange. Typically > > the magic value should be the cookie that identifies the data blob as > > generated by this driver and a separate version field would identify > > any content or format changes. In the mtty driver I included a magic > > field along with major and minor versions to provide this flexibility. > > I wonder why we wouldn't do something similar here rather than create > > this combination magic/version field. It's easy enough to append a > > couple fields onto the structure or redefine a v2 structure to use > > going forward. There's even a padding field in the structure already > > that could be repurposed. > > > > If we add the major and minor version numbers like mtty driver, the current > data structure needs to be changed, and this change also needs to be compatible > with the old and new versions. > And the old version does not have this information. The new version cannot > be adapted when it is imported. Are you suggesting that we cannot use a different data structure based on the magic value? That's rather the point of the magic value. > Now in this patch to fix this problem, we only need to update a magic number, > and then the problem is corrected. Migration between old and new versions can > also be fixed: > > Old --> Old, wrong, but we can't stop it; > Old --> New, corrected migrated incorrect data, recovery successful; > New --> New, correct, recovery successful; > New --> Old, correct, recovery successful. > > 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