hw/i386/amd_iommu.c | 62 ++++++++++----------------------------------- 1 file changed, 14 insertions(+), 48 deletions(-)
- 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
- */
-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);
}
--
2.25.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); > } > -- Best, Valentine Sinitsyn
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.
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
© 2016 - 2024 Red Hat, Inc.