Re: [PATCH v1] hw/i386/amd_iommu: clean up broken event logging
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
---
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
Re: [PATCH v1] hw/i386/amd_iommu: clean up broken event logging
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
Re: [PATCH v1] hw/i386/amd_iommu: clean up broken event logging
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.
[PATCH v1] hw/i386/amd_iommu: clean up broken event logging
- 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
---
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
- */
-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
