qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: qemu-devel@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Jason Wang <jasowang@redhat.com>, Peter Xu <peterx@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Subject: [PULL 23/31] intel_iommu: refine iotlb hash calculation
Date: Tue, 25 Apr 2023 03:46:12 -0400	[thread overview]
Message-ID: <ec1a78cee97001b0ed25b5866e92dae058eb5877.1682408661.git.mst@redhat.com> (raw)
In-Reply-To: <cover.1682408661.git.mst@redhat.com>

From: Jason Wang <jasowang@redhat.com>

Commit 1b2b12376c8 ("intel-iommu: PASID support") takes PASID into
account when calculating iotlb hash like:

static guint vtd_iotlb_hash(gconstpointer v)
{
    const struct vtd_iotlb_key *key = v;

    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
           (key->level) << VTD_IOTLB_LVL_SHIFT |
           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
}

This turns out to be problematic since:

- the shift will lose bits if not converting to uint64_t
- level should be off by one in order to fit into 2 bits
- VTD_IOTLB_PASID_SHIFT is 30 but PASID is 20 bits which will waste
  some bits
- the hash result is uint64_t so we will lose bits when converting to
  guint

So this patch fixes them by

- converting the keys into uint64_t before doing the shift
- off level by one to make it fit into two bits
- change the sid, lvl and pasid shift to 26, 42 and 44 in order to
  take the full width of uint64_t
- perform an XOR to the top 32bit with the bottom 32bit for the final
  result to fit guint

Fixes: Coverity CID 1508100
Fixes: 1b2b12376c8 ("intel-iommu: PASID support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20230412073510.7158-1-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu_internal.h | 6 +++---
 hw/i386/intel_iommu.c          | 9 +++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index f090e61e11..2e61eec2f5 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -114,9 +114,9 @@
                                      VTD_INTERRUPT_ADDR_FIRST + 1)
 
 /* The shift of source_id in the key of IOTLB hash table */
-#define VTD_IOTLB_SID_SHIFT         20
-#define VTD_IOTLB_LVL_SHIFT         28
-#define VTD_IOTLB_PASID_SHIFT       30
+#define VTD_IOTLB_SID_SHIFT         26
+#define VTD_IOTLB_LVL_SHIFT         42
+#define VTD_IOTLB_PASID_SHIFT       44
 #define VTD_IOTLB_MAX_SIZE          1024    /* Max size of the hash table */
 
 /* IOTLB_REG */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a62896759c..94d52f4205 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -64,8 +64,8 @@ struct vtd_as_key {
 struct vtd_iotlb_key {
     uint64_t gfn;
     uint32_t pasid;
-    uint32_t level;
     uint16_t sid;
+    uint8_t level;
 };
 
 static void vtd_address_space_refresh_all(IntelIOMMUState *s);
@@ -221,10 +221,11 @@ static gboolean vtd_iotlb_equal(gconstpointer v1, gconstpointer v2)
 static guint vtd_iotlb_hash(gconstpointer v)
 {
     const struct vtd_iotlb_key *key = v;
+    uint64_t hash64 = key->gfn | ((uint64_t)(key->sid) << VTD_IOTLB_SID_SHIFT) |
+        (uint64_t)(key->level - 1) << VTD_IOTLB_LVL_SHIFT |
+        (uint64_t)(key->pasid) << VTD_IOTLB_PASID_SHIFT;
 
-    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
-           (key->level) << VTD_IOTLB_LVL_SHIFT |
-           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
+    return (guint)((hash64 >> 32) ^ (hash64 & 0xffffffffU));
 }
 
 static gboolean vtd_as_equal(gconstpointer v1, gconstpointer v2)
-- 
MST



WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: qemu-devel@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>,
	Jason Wang <jasowang@redhat.com>, Peter Xu <peterx@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Subject: [PULL 23/31] intel_iommu: refine iotlb hash calculation
Date: Tue, 25 Apr 2023 04:04:15 -0400	[thread overview]
Message-ID: <ec1a78cee97001b0ed25b5866e92dae058eb5877.1682408661.git.mst@redhat.com> (raw)
Message-ID: <20230425080415.R9cjwZv06nDeZ0PoGCKqC6u8vV9Zjx2uAZ77bIUAaW0@z> (raw)
In-Reply-To: <cover.1682408661.git.mst@redhat.com>

From: Jason Wang <jasowang@redhat.com>

Commit 1b2b12376c8 ("intel-iommu: PASID support") takes PASID into
account when calculating iotlb hash like:

static guint vtd_iotlb_hash(gconstpointer v)
{
    const struct vtd_iotlb_key *key = v;

    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
           (key->level) << VTD_IOTLB_LVL_SHIFT |
           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
}

This turns out to be problematic since:

- the shift will lose bits if not converting to uint64_t
- level should be off by one in order to fit into 2 bits
- VTD_IOTLB_PASID_SHIFT is 30 but PASID is 20 bits which will waste
  some bits
- the hash result is uint64_t so we will lose bits when converting to
  guint

So this patch fixes them by

- converting the keys into uint64_t before doing the shift
- off level by one to make it fit into two bits
- change the sid, lvl and pasid shift to 26, 42 and 44 in order to
  take the full width of uint64_t
- perform an XOR to the top 32bit with the bottom 32bit for the final
  result to fit guint

Fixes: Coverity CID 1508100
Fixes: 1b2b12376c8 ("intel-iommu: PASID support")
Signed-off-by: Jason Wang <jasowang@redhat.com>
Message-Id: <20230412073510.7158-1-jasowang@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu_internal.h | 6 +++---
 hw/i386/intel_iommu.c          | 9 +++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index f090e61e11..2e61eec2f5 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -114,9 +114,9 @@
                                      VTD_INTERRUPT_ADDR_FIRST + 1)
 
 /* The shift of source_id in the key of IOTLB hash table */
-#define VTD_IOTLB_SID_SHIFT         20
-#define VTD_IOTLB_LVL_SHIFT         28
-#define VTD_IOTLB_PASID_SHIFT       30
+#define VTD_IOTLB_SID_SHIFT         26
+#define VTD_IOTLB_LVL_SHIFT         42
+#define VTD_IOTLB_PASID_SHIFT       44
 #define VTD_IOTLB_MAX_SIZE          1024    /* Max size of the hash table */
 
 /* IOTLB_REG */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index a62896759c..94d52f4205 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -64,8 +64,8 @@ struct vtd_as_key {
 struct vtd_iotlb_key {
     uint64_t gfn;
     uint32_t pasid;
-    uint32_t level;
     uint16_t sid;
+    uint8_t level;
 };
 
 static void vtd_address_space_refresh_all(IntelIOMMUState *s);
@@ -221,10 +221,11 @@ static gboolean vtd_iotlb_equal(gconstpointer v1, gconstpointer v2)
 static guint vtd_iotlb_hash(gconstpointer v)
 {
     const struct vtd_iotlb_key *key = v;
+    uint64_t hash64 = key->gfn | ((uint64_t)(key->sid) << VTD_IOTLB_SID_SHIFT) |
+        (uint64_t)(key->level - 1) << VTD_IOTLB_LVL_SHIFT |
+        (uint64_t)(key->pasid) << VTD_IOTLB_PASID_SHIFT;
 
-    return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
-           (key->level) << VTD_IOTLB_LVL_SHIFT |
-           (key->pasid) << VTD_IOTLB_PASID_SHIFT;
+    return (guint)((hash64 >> 32) ^ (hash64 & 0xffffffffU));
 }
 
 static gboolean vtd_as_equal(gconstpointer v1, gconstpointer v2)
-- 
MST



  parent reply	other threads:[~2023-04-25  7:50 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-25  7:44 [PULL 00/31] virtio,pc,pci: fixes, features, cleanups Michael S. Tsirkin
2023-04-25  7:44 ` [PULL 01/31] virtio: refresh vring region cache after updating a virtqueue size Michael S. Tsirkin
2023-04-26 16:32   ` Michael Tokarev
2023-04-25  7:44 ` [PULL 02/31] Add my old and new work email mapping and use work email to support biosbits Michael S. Tsirkin
2023-04-25  7:44 ` [PULL 03/31] vdpa: accept VIRTIO_NET_F_SPEED_DUPLEX in SVQ Michael S. Tsirkin
2023-04-25  7:44 ` [PULL 04/31] meson_options.txt: Enable qom-cast-debug by default again Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 05/31] vhost: Drop unused eventfd_add|del hooks Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 06/31] docs: vhost-user: Define memory region separately Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 07/31] docs: vhost-user: Add Xen specific memory mapping support Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 08/31] virtio-balloon: optimize the virtio-balloon on the ARM platform Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 09/31] MAINTAINERS: Mark AMD-Vi emulation as orphan Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 10/31] hw/i386/amd_iommu: Explicit use of AMDVI_BASE_ADDR in amdvi_init Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 11/31] hw/i386/amd_iommu: Remove intermediate AMDVIState::devid field Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 12/31] hw/i386/amd_iommu: Move capab_offset from AMDVIState to AMDVIPCIState Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 13/31] hw/i386/amd_iommu: Set PCI static/const fields via PCIDeviceClass Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 14/31] hw/i386/amd_iommu: Factor amdvi_pci_realize out of amdvi_sysbus_realize Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 15/31] hw: Add compat machines for 8.1 Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 16/31] pci: avoid accessing slot_reserved_mask directly outside of pci.c Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 17/31] vhost-user-blk-server: notify client about disk resize Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 18/31] Add my old and new work email mapping and use work email to support acpi Michael S. Tsirkin
2023-04-25  8:22   ` Ani Sinha
2023-04-25  7:45 ` [PULL 19/31] hw/acpi: limit warning on acpi table size to pc machines older than version 2.3 Michael S. Tsirkin
2023-04-25  7:45 ` [PULL 20/31] tests: bios-tables-test: replace memset with initializer Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 21/31] MAINTAINERS: Add Eugenio Pérez as vhost-shadow-virtqueue reviewer Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 22/31] docs/cxl: Fix sentence Michael S. Tsirkin
2023-04-25  7:46 ` Michael S. Tsirkin [this message]
2023-04-25  8:04   ` [PULL 23/31] intel_iommu: refine iotlb hash calculation Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 25/31] virtio: i2c: Check notifier helpers for VIRTIO_CONFIG_IRQ_IDX Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 26/31] acpi: pcihp: allow repeating hot-unplug requests Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 27/31] docs/specs/pci-ids: Convert from txt to rST Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 28/31] docs/specs: Convert pci-serial.txt to rst Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 29/31] docs/specs: Convert pci-testdev.txt " Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 30/31] hw/pci-bridge: pci_expander_bridge fix type in pxb_cxl_dev_reset() Michael S. Tsirkin
2023-04-25  7:46 ` [PULL 31/31] hw/pci-bridge: Make PCIe and CXL PXB Devices inherit from TYPE_PXB_DEV Michael S. Tsirkin
2023-04-25  8:04 ` [PULL 24/31] docs: Remove obsolete descriptions of SR-IOV support Michael S. Tsirkin
2023-04-25 11:16 ` [PULL 00/31] virtio,pc,pci: fixes, features, cleanups Richard Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ec1a78cee97001b0ed25b5866e92dae058eb5877.1682408661.git.mst@redhat.com \
    --to=mst@redhat.com \
    --cc=eduardo@habkost.net \
    --cc=jasowang@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=peterx@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).