Re: [Qemu-devel] [V11 1/4] hw/i386: Introduce AMD IOMMU

[Peter Xu <peterx@redhat.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