qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1] hw/i386/amd_iommu: clean up broken event logging


From: Valentin Sinitsyn
Subject: Re: [PATCH v1] hw/i386/amd_iommu: clean up broken event logging
Date: Wed, 17 Nov 2021 23:13:27 +0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

Hi,

On 17.11.2021 19:46, Daniil Tatianin wrote:
- Don't create evt buffer in every function where we want to log,
   instead make amdvi_log_event construct the buffer in-place using the
   arguments it was given.

- Correctly place address & info in the event buffer. Previously both
   would get shifted to incorrect bit offsets leading to evt containing
   uninitialized event type and address.

- Reduce buffer size from 32 to 16 bytes, which is the amount of bytes
   actually needed for event storage.

- Remove amdvi_encode_event & amdvi_setevent_bits, as they are no longer
   needed.

Signed-off-by: Daniil Tatianin <d-tatianin@yandex-team.ru>
---
  hw/i386/amd_iommu.c | 62 ++++++++++-----------------------------------
  1 file changed, 14 insertions(+), 48 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index fd75cae024..44b995be91 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -164,8 +164,14 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
      }
  }
-static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
+static void amdvi_log_event(AMDVIState *s, uint16_t devid,
+                            hwaddr addr, uint16_t info)
  {
+    uint64_t evt[2] = {
+        devid | ((uint64_t)info << 48),
+        addr
+    };
+
      /* event logging not enabled */
      if (!s->evtlog_enabled || amdvi_test_mask(s, AMDVI_MMIO_STATUS,
          AMDVI_MMIO_STATUS_EVT_OVF)) {
@@ -190,27 +196,6 @@ static void amdvi_log_event(AMDVIState *s, uint64_t *evt)
      amdvi_generate_msi_interrupt(s);
  }
-static void amdvi_setevent_bits(uint64_t *buffer, uint64_t value, int start,
-                                int length)
-{
-    int index = start / 64, bitpos = start % 64;
-    uint64_t mask = MAKE_64BIT_MASK(start, length);
-    buffer[index] &= ~mask;
-    buffer[index] |= (value << bitpos) & mask;
-}
-/*
- * AMDVi event structure
- *    0:15   -> DeviceID
- *    55:63  -> event type + miscellaneous info
- *    63:127 -> related address
Did you mean 64:127? Fields shouldn't overlap, and 65-bit address looks suspicious.

- */
-static void amdvi_encode_event(uint64_t *evt, uint16_t devid, uint64_t addr,
-                               uint16_t info)
-{
-    amdvi_setevent_bits(evt, devid, 0, 16);
-    amdvi_setevent_bits(evt, info, 55, 8);
-    amdvi_setevent_bits(evt, addr, 63, 64);
-}
  /* log an error encountered during a page walk
   *
   * @addr: virtual address in translation request
@@ -218,11 +203,8 @@ static void amdvi_encode_event(uint64_t *evt, uint16_t 
devid, uint64_t addr,
  static void amdvi_page_fault(AMDVIState *s, uint16_t devid,
                               hwaddr addr, uint16_t info)
  {
-    uint64_t evt[4];
-
      info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF;
-    amdvi_encode_event(evt, devid, addr, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, devid, addr, info);
      pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
              PCI_STATUS_SIG_TARGET_ABORT);
  }
@@ -232,14 +214,10 @@ static void amdvi_page_fault(AMDVIState *s, uint16_t 
devid,
   *  @info : error flags
   */
  static void amdvi_log_devtab_error(AMDVIState *s, uint16_t devid,
-                                   hwaddr devtab, uint16_t info)
+                                   hwaddr addr, uint16_t info)
  {
-    uint64_t evt[4];
-
      info |= AMDVI_EVENT_DEV_TAB_HW_ERROR;
-
-    amdvi_encode_event(evt, devid, devtab, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, devid, addr, info);
      pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
              PCI_STATUS_SIG_TARGET_ABORT);
  }
@@ -248,10 +226,7 @@ static void amdvi_log_devtab_error(AMDVIState *s, uint16_t 
devid,
   */
  static void amdvi_log_command_error(AMDVIState *s, hwaddr addr)
  {
-    uint64_t evt[4], info = AMDVI_EVENT_COMMAND_HW_ERROR;
-
-    amdvi_encode_event(evt, 0, addr, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, 0, addr, AMDVI_EVENT_COMMAND_HW_ERROR);
      pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
              PCI_STATUS_SIG_TARGET_ABORT);
  }
@@ -261,11 +236,8 @@ static void amdvi_log_command_error(AMDVIState *s, hwaddr 
addr)
  static void amdvi_log_illegalcom_error(AMDVIState *s, uint16_t info,
                                         hwaddr addr)
  {
-    uint64_t evt[4];
-
      info |= AMDVI_EVENT_ILLEGAL_COMMAND_ERROR;
-    amdvi_encode_event(evt, 0, addr, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, 0, addr, info);
  }
  /* log an error accessing device table
   *
@@ -276,11 +248,8 @@ static void amdvi_log_illegalcom_error(AMDVIState *s, 
uint16_t info,
  static void amdvi_log_illegaldevtab_error(AMDVIState *s, uint16_t devid,
                                            hwaddr addr, uint16_t info)
  {
-    uint64_t evt[4];
-
      info |= AMDVI_EVENT_ILLEGAL_DEVTAB_ENTRY;
-    amdvi_encode_event(evt, devid, addr, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, devid, addr, info);
  }
  /* log an error accessing a PTE entry
   * @addr : address that couldn't be accessed
@@ -288,11 +257,8 @@ static void amdvi_log_illegaldevtab_error(AMDVIState *s, 
uint16_t devid,
  static void amdvi_log_pagetab_error(AMDVIState *s, uint16_t devid,
                                      hwaddr addr, uint16_t info)
  {
-    uint64_t evt[4];
-
      info |= AMDVI_EVENT_PAGE_TAB_HW_ERROR;
-    amdvi_encode_event(evt, devid, addr, info);
-    amdvi_log_event(s, evt);
+    amdvi_log_event(s, devid, addr, info);
      pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS,
               PCI_STATUS_SIG_TARGET_ABORT);
  }


--
Best,
Valentine Sinitsyn



reply via email to

[Prev in Thread] Current Thread [Next in Thread]