From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57183) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5BYx-0007Q5-4T for qemu-devel@nongnu.org; Tue, 24 May 2016 08:35:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b5BYr-0004Ql-NS for qemu-devel@nongnu.org; Tue, 24 May 2016 08:35:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57329) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b5BYr-0004Qe-Er for qemu-devel@nongnu.org; Tue, 24 May 2016 08:35:49 -0400 Date: Tue, 24 May 2016 20:35:44 +0800 From: Peter Xu Message-ID: <20160524123544.GH8247@pxdev.xzpeter.org> References: <1463912514-12658-1-git-send-email-davidkiarie4@gmail.com> <1463912514-12658-2-git-send-email-davidkiarie4@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1463912514-12658-2-git-send-email-davidkiarie4@gmail.com> Subject: Re: [Qemu-devel] [V11 1/4] hw/i386: Introduce AMD IOMMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: qemu-devel@nongnu.org, jan.kiszka@web.de, mst@redhat.com, marcel@redhat.com, valentine.sinitsyn@gmail.com 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