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