[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [V11 1/4] hw/i386: Introduce AMD IOMMU
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [V11 1/4] hw/i386: Introduce AMD IOMMU |
Date: |
Tue, 24 May 2016 20:35:44 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Sun, May 22, 2016 at 01:21:51PM +0300, David Kiarie wrote:
[...]
> +#define DEBUG_AMD_AMDVI
> +#ifdef DEBUG_AMD_AMDVI
> +enum {
> + DEBUG_GENERAL, DEBUG_CAPAB, DEBUG_MMIO, DEBUG_ELOG,
> + DEBUG_CACHE, DEBUG_COMMAND, DEBUG_MMU, DEBUG_CUSTOM
> +};
> +
> +#define AMDVI_DBGBIT(x) (1 << DEBUG_##x)
> +static int iommu_dbgflags = AMDVI_DBGBIT(CUSTOM) | AMDVI_DBGBIT(MMU);
> +
> +#define AMDVI_DPRINTF(what, fmt, ...) do { \
> + if (iommu_dbgflags & AMDVI_DBGBIT(what)) { \
> + fprintf(stderr, "(amd-iommu)%s: " fmt "\n", __func__, \
> + ## __VA_ARGS__); } \
> + } while (0)
> +#else
> +#define AMDVI_DPRINTF(what, fmt, ...) do {} while (0)
> +#endif
(actually I was considering whether it would be cool that both Intel
and AMD IOMMU codes start to leverage trace utilities. Re-compiling
for debugging every time is not convenient, and also not aligned
with other part of QEMU. However I guess this is in-all-cases too
late for a v11 patchset... So just raise this question up in the
brackets)
[...]
> +static void amdvi_log_event(AMDVIState *s, uint16_t *evt)
> +{
> + /* event logging not enabled */
> + if (!s->evtlog_enabled || *(uint64_t *)&s->mmior[AMDVI_MMIO_STATUS] |
> + AMDVI_MMIO_STATUS_EVT_OVF) {
I see that there are lots of places in this patch that used
something like:
*(uint64_t *)s->mmior[X] | Y
Or:
*(uint64_t *)s->mmior[X] |= Y
So... would it be a good idea that we provide several more helpers,
like amdvi_orq() and amdvi_readq()?
> + return;
> + }
> +
> + /* event log buffer full */
> + if (s->evtlog_tail >= s->evtlog_len) {
> + *(uint64_t *)&s->mmior[AMDVI_MMIO_STATUS] |=
> AMDVI_MMIO_STATUS_EVT_OVF;
Yet another example...
> + /* generate interrupt */
> + amdvi_generate_msi_interrupt(s);
> + return;
> + }
> +
> + if (dma_memory_write(&address_space_memory, s->evtlog_len +
> s->evtlog_tail,
> + &evt, AMDVI_EVENT_LEN)) {
> + AMDVI_DPRINTF(ELOG, "error: fail to write at address 0x%"PRIx64
> + " + offset 0x%"PRIx32, s->evtlog, s->evtlog_tail);
> + }
> +
> + s->evtlog_tail += AMDVI_EVENT_LEN;
> + *(uint64_t *)&s->mmior[AMDVI_MMIO_STATUS] |= AMDVI_MMIO_STATUS_COMP_INT;
Another one. (will stop finding examples)
> + amdvi_generate_msi_interrupt(s);
> +}
[...]
> +/* extract device id */
> +static inline uint16_t devid_extract(uint8_t *cmd)
> +{
> + return (uint16_t)cmd[2] & AMDVI_INVAL_DEV_ID_MASK;
Here the mask is defined as:
#define AMDVI_INVAL_DEV_ID_MASK (~((1UL << AMDVI_INVAL_DEV_ID_SHIFT) - 1))
I think it should be 0xffffffff00000000 for 64 bit systems. However
here cmd[2] is type uint8_t. Is there anything wrong?...
Also, I see many places that we manipulate arbitary elements in
cmd[] directly with some masks. Not sure whether we can make it more
readable (which is optional though).
[...]
> +static void amdvi_update_iotlb(AMDVIState *s, uint16_t devid,
> + uint64_t gpa, IOMMUTLBEntry to_cache,
> + uint16_t domid)
> +{
> + AMDVIIOTLBEntry *entry = g_malloc(sizeof(*entry));
> + uint64_t *key = g_malloc(sizeof(key));
> + uint64_t gfn = gpa >> AMDVI_PAGE_SHIFT_4K;
> +
> + /* don't cache erroneous translations */
> + if (to_cache.perm != IOMMU_NONE) {
> + AMDVI_DPRINTF(CACHE, " update iotlb domid 0x%"PRIx16" devid: "
> + "%02x:%02x.%xgpa 0x%"PRIx64 " hpa 0x%"PRIx64, domid,
> + PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> + gpa, to_cache.translated_addr);
> +
> + if (g_hash_table_size(s->iotlb) >= AMDVI_IOTLB_MAX_SIZE) {
> + AMDVI_DPRINTF(CACHE, "iotlb exceeds size limit - reset");
> + amdvi_iotlb_reset(s);
I remembered that you have mentioned about one of your work that
re-implemented another iotlb?
[...]
> +/* log error without aborting since linux seems to be using reserved bits */
> +static void amdvi_inval_devtab_entry(AMDVIState *s, uint8_t *cmd,
> + uint8_t type)
> +{
> + /* This command should invalidate internal caches of which there isn't */
> + if (*(uint64_t *)&cmd[0] & AMDVI_CMD_INVAL_DEV_RSVD ||
> + *(uint64_t *)&cmd[1]) {
Indentation not aligned with the rest of the codes.
> + amdvi_log_illegalcom_error(s, type, s->cmdbuf + s->cmdbuf_head);
> + }
> +#ifdef DEBUG_AMD_AMDVI
> + uint16_t devid = devid_extract(cmd);
> +#endif
> + AMDVI_DPRINTF(COMMAND, "device table entry for devid: %02x:%02x.%x"
> + " invalidated", PCI_BUS_NUM(devid), PCI_SLOT(devid),
> + PCI_FUNC(devid));
> +}
> +
> +static void amdvi_completion_wait(AMDVIState *s, uint8_t *cmd, uint8_t type)
> +{
> + if (*(uint32_t *)&cmd[1] & AMDVI_COMPLETION_WAIT_RSVD) {
> + amdvi_log_illegalcom_error(s, type, s->cmdbuf + s->cmdbuf_head);
> + }
> + /* pretend to wait for command execution to complete */
> + AMDVI_DPRINTF(COMMAND, "completion wait requested with store address 0x%"
> + PRIx64 " and store data 0x%"PRIx64, (cmd[0] &
> + AMDVI_COM_STORE_ADDRESS_MASK), *(uint64_t *)(cmd + 8));
> + amdvi_completion_wait_exec(s, cmd);
> +}
> +
> +static void amdvi_complete_ppr(AMDVIState *s, uint8_t *cmd, uint8_t type)
> +{
> + if ((*(uint64_t *)&cmd[0] & AMDVI_COMPLETE_PPR_RQ_RSVD) ||
> + *(uint64_t *)&cmd[1] & AMDVI_COMPLETE_PPR_HIGH_RSVD) {
> + amdvi_log_illegalcom_error(s, type, s->cmdbuf + s->cmdbuf_head);
> + }
Could you help explain why this command can be skipped with no-op?
Thanks.
> +
> + AMDVI_DPRINTF(COMMAND, "Execution of PPR queue requested");
> +}
[...]
> +/* not honouring reserved bits is regarded as an illegal command */
> +static void amdvi_cmdbuf_exec(AMDVIState *s)
> +{
> + uint8_t type;
> + uint8_t cmd[AMDVI_COMMAND_SIZE];
> +
> + memset(cmd, 0, AMDVI_COMMAND_SIZE);
> +
> + if (dma_memory_read(&address_space_memory, s->cmdbuf + s->cmdbuf_head,
> cmd,
> + AMDVI_COMMAND_SIZE)) {
> + AMDVI_DPRINTF(COMMAND, "error: fail to access memory at 0x%"PRIx64
> + " + 0x%"PRIu8, s->cmdbuf, s->cmdbuf_head);
> + amdvi_log_command_error(s, s->cmdbuf + s->cmdbuf_head);
> + return;
> + }
> +
> + *cmd = le64_to_cpu(*(uint64_t *)cmd);
Here we are assigning uint64_t to an uint8_t? Is this what we want?
Thanks,
-- peterx