diff mbox series

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

Message ID 20211117144641.837227-1-d-tatianin@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series [v1] hw/i386/amd_iommu: clean up broken event logging | expand

Commit Message

Daniil Tatianin Nov. 17, 2021, 2:46 p.m. UTC 
- 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(-)

Comments

Valentin Sinitsyn Nov. 17, 2021, 6:13 p.m. UTC | #1 
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);
>   }
>
Roman Kagan Nov. 17, 2021, 7:03 p.m. UTC | #2 
On Wed, Nov 17, 2021 at 11:13:27PM +0500, Valentin Sinitsyn wrote:
> On 17.11.2021 19:46, Daniil Tatianin wrote:
> > -/*
> > - * 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.

These lines are being removed by this patch.  And yes, they were wrong
WRT the spec.

Roman.
Valentin Sinitsyn Nov. 17, 2021, 7:19 p.m. UTC | #3 
On 18.11.2021 00:03, Roman Kagan wrote:
> On Wed, Nov 17, 2021 at 11:13:27PM +0500, Valentin Sinitsyn wrote:
>> On 17.11.2021 19:46, Daniil Tatianin wrote:
>>> -/*
>>> - * 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.
> 
> These lines are being removed by this patch.  And yes, they were wrong
> WRT the spec.
Oops I missed a minus. Sorry for that.

Valentin
diff mbox series

Patch

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
- */
-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);
 }