Re: [Qemu-devel] [V12 3/4] hw/i386: Introduce AMD IOMMU

[Jan Kiszka]

On 2016-07-04 07:06, David Kiarie wrote:
> On Wed, Jun 22, 2016 at 11:24 PM, Jan Kiszka <address@hidden> wrote:
>> On 2016-06-15 14:21, David Kiarie wrote:
>>> +static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned size)
>>> +{
>>> +    AMDVIState *s = opaque;
>>> +
>>> +    uint64_t val = -1;
>>> +    if (addr + size > AMDVI_MMIO_SIZE) {
>>> +        trace_amdvi_mmio_read("error: addr outside region: max ",
>>> +                (uint64_t)AMDVI_MMIO_SIZE, addr, size);
>>> +        return (uint64_t)-1;
>>> +    }
>>> +
>>> +    if (size == 2) {
>>> +        val = amdvi_readw(s, addr);
>>> +    } else if (size == 4) {
>>> +        val = amdvi_readl(s, addr);
>>> +    } else if (size == 8) {
>>> +        val = amdvi_readq(s, addr);
>>> +    }
>>> +
>>> +    switch (addr & ~0x07) {
>>> +    case AMDVI_MMIO_DEVICE_TABLE:
>>> +        trace_amdvi_mmio_read("MMIO_DEVICE_TABLE", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_BASE:
>>> +        trace_amdvi_mmio_read("MMIO_COMMAND_BASE", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_BASE:
>>> +        trace_amdvi_mmio_read("MMIO_EVENT_BASE", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_CONTROL:
>>> +        trace_amdvi_mmio_read("MMIO_MMIO_CONTROL", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EXCL_BASE:
>>> +        trace_amdvi_mmio_read("MMIO_EXCL_BASE", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EXCL_LIMIT:
>>> +        trace_amdvi_mmio_read("MMIO_EXCL_LIMIT", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_HEAD:
>>> +        trace_amdvi_mmio_read("MMIO_COMMAND_HEAD", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_TAIL:
>>> +        trace_amdvi_mmio_read("MMIO_COMMAND_TAIL", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_HEAD:
>>> +        trace_amdvi_mmio_read("MMIO_EVENT_HEAD", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_TAIL:
>>> +        trace_amdvi_mmio_read("MMIO_EVENT_TAIL", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_STATUS:
>>> +        trace_amdvi_mmio_read("MMIO_STATUS", addr, size, addr & ~0x07);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EXT_FEATURES:
>>> +        trace_amdvi_mmio_read("MMIO_EXT_FEATURES", addr, size, addr & 
>>> ~0x07);
>>> +        break;
>>
>> What about a lookup table for that name?
> 
> I can't find an obvious way to index a table given the register address.

Well, you would need a low ((addr & 0x2000) == 0) and a high table (addr
& 0x2000), and then do the indexing based on (addr & ~0x2000) / 8.

> 
>>
>>> +
>>> +    default:
>>> +        trace_amdvi_mmio_read("UNHANDLED READ", addr, size, addr & ~0x07);
>>> +    }
>>> +    return val;
>>> +}
>>> +
>>> +static void amdvi_handle_control_write(AMDVIState *s)
>>> +{
>>> +    /*
>>> +     * read whatever is already written in case
>>> +     * software is writing in chucks less than 8 bytes
>>> +     */
>>> +    unsigned long control = amdvi_readq(s, AMDVI_MMIO_CONTROL);
>>> +    s->enabled = !!(control & AMDVI_MMIO_CONTROL_AMDVIEN);
>>> +
>>> +    s->ats_enabled = !!(control & AMDVI_MMIO_CONTROL_HTTUNEN);
>>> +    s->evtlog_enabled = s->enabled && !!(control &
>>> +                        AMDVI_MMIO_CONTROL_EVENTLOGEN);
>>> +
>>> +    s->evtlog_intr = !!(control & AMDVI_MMIO_CONTROL_EVENTINTEN);
>>> +    s->completion_wait_intr = !!(control & 
>>> AMDVI_MMIO_CONTROL_COMWAITINTEN);
>>> +    s->cmdbuf_enabled = s->enabled && !!(control &
>>> +                        AMDVI_MMIO_CONTROL_CMDBUFLEN);
>>> +
>>> +    /* update the flags depending on the control register */
>>> +    if (s->cmdbuf_enabled) {
>>> +        amdvi_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_CMDBUF_RUN);
>>> +    } else {
>>> +        amdvi_and_assignq(s, AMDVI_MMIO_STATUS, 
>>> ~AMDVI_MMIO_STATUS_CMDBUF_RUN);
>>> +    }
>>> +    if (s->evtlog_enabled) {
>>> +        amdvi_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVT_RUN);
>>> +    } else {
>>> +        amdvi_and_assignq(s, AMDVI_MMIO_STATUS, 
>>> ~AMDVI_MMIO_STATUS_EVT_RUN);
>>> +    }
>>> +
>>> +    trace_amdvi_control_status(control);
>>> +
>>> +    amdvi_cmdbuf_run(s);
>>> +}
>>> +
>>> +static inline void amdvi_handle_devtab_write(AMDVIState *s)
>>> +
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_DEVICE_TABLE);
>>> +    s->devtab = (val & AMDVI_MMIO_DEVTAB_BASE_MASK);
>>> +
>>> +    /* set device table length */
>>> +    s->devtab_len = ((val & AMDVI_MMIO_DEVTAB_SIZE_MASK) + 1 *
>>> +                    (AMDVI_MMIO_DEVTAB_SIZE_UNIT /
>>> +                     AMDVI_MMIO_DEVTAB_ENTRY_SIZE));
>>> +}
>>> +
>>> +static inline void amdvi_handle_cmdhead_write(AMDVIState *s)
>>> +{
>>> +    s->cmdbuf_head = amdvi_readq(s, AMDVI_MMIO_COMMAND_HEAD)
>>> +                     & AMDVI_MMIO_CMDBUF_HEAD_MASK;
>>> +    amdvi_cmdbuf_run(s);
>>> +}
>>> +
>>> +static inline void amdvi_handle_cmdbase_write(AMDVIState *s)
>>> +{
>>> +    s->cmdbuf = amdvi_readq(s, AMDVI_MMIO_COMMAND_BASE)
>>> +                & AMDVI_MMIO_CMDBUF_BASE_MASK;
>>> +    s->cmdbuf_len = 1UL << (s->mmior[AMDVI_MMIO_CMDBUF_SIZE_BYTE]
>>> +                    & AMDVI_MMIO_CMDBUF_SIZE_MASK);
>>> +    s->cmdbuf_head = s->cmdbuf_tail = 0;
>>> +}
>>> +
>>> +static inline void amdvi_handle_cmdtail_write(AMDVIState *s)
>>> +{
>>> +    s->cmdbuf_tail = amdvi_readq(s, AMDVI_MMIO_COMMAND_TAIL)
>>> +                     & AMDVI_MMIO_CMDBUF_TAIL_MASK;
>>> +    amdvi_cmdbuf_run(s);
>>> +}
>>> +
>>> +static inline void amdvi_handle_excllim_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_EXCL_LIMIT);
>>> +    s->excl_limit = (val & AMDVI_MMIO_EXCL_LIMIT_MASK) |
>>> +                    AMDVI_MMIO_EXCL_LIMIT_LOW;
>>> +}
>>> +
>>> +static inline void amdvi_handle_evtbase_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_EVENT_BASE);
>>> +    s->evtlog = val & AMDVI_MMIO_EVTLOG_BASE_MASK;
>>> +    s->evtlog_len = 1UL << (*(uint64_t 
>>> *)&s->mmior[AMDVI_MMIO_EVTLOG_SIZE_BYTE]
>>> +                    & AMDVI_MMIO_EVTLOG_SIZE_MASK);
>>> +}
>>> +
>>> +static inline void amdvi_handle_evttail_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_EVENT_TAIL);
>>> +    s->evtlog_tail = val & AMDVI_MMIO_EVTLOG_TAIL_MASK;
>>> +}
>>> +
>>> +static inline void amdvi_handle_evthead_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_EVENT_HEAD);
>>> +    s->evtlog_head = val & AMDVI_MMIO_EVTLOG_HEAD_MASK;
>>> +}
>>> +
>>> +static inline void amdvi_handle_pprbase_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_PPR_BASE);
>>> +    s->ppr_log = val & AMDVI_MMIO_PPRLOG_BASE_MASK;
>>> +    s->pprlog_len = 1UL << (*(uint64_t 
>>> *)&s->mmior[AMDVI_MMIO_PPRLOG_SIZE_BYTE]
>>> +                    & AMDVI_MMIO_PPRLOG_SIZE_MASK);
>>> +}
>>> +
>>> +static inline void amdvi_handle_pprhead_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_PPR_HEAD);
>>> +    s->pprlog_head = val & AMDVI_MMIO_PPRLOG_HEAD_MASK;
>>> +}
>>> +
>>> +static inline void amdvi_handle_pprtail_write(AMDVIState *s)
>>> +{
>>> +    uint64_t val = amdvi_readq(s, AMDVI_MMIO_PPR_TAIL);
>>> +    s->pprlog_tail = val & AMDVI_MMIO_PPRLOG_TAIL_MASK;
>>> +}
>>> +
>>> +/* FIXME: something might go wrong if System Software writes in chunks
>>> + * of one byte but linux writes in chunks of 4 bytes so currently it
>>> + * works correctly with linux but will definitely be busted if software
>>> + * reads/writes 8 bytes
>>
>> What does the spec tell us about non-dword accesses? If they aren't
>> allowed, filter them out completely at the beginning.
> 
> Non-dword accesses are allowed. Spec 2.62
> 
> ".... Accesses must be aligned to the size of the access and the size
> in bytes must be a power
> of two. Software may use accesses as small as one byte..... "
> 
> Linux uses dword accesses though but even in this case a change in the
> order of access whereby the high part of the register is accessed
> first then the lower accessed could cause a problem (??)

I do not get yet what makes it tricky to support all allowed access
sizes. You are just missing byte access, and that will easy to add once
the size dispatching is done in a single function like suggested below.

> 
>>
>>> + */
>>> +static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                             unsigned size)
>>> +{
>>> +    AMDVIState *s = opaque;
>>> +    unsigned long offset = addr & 0x07;
>>> +
>>> +    if (addr + size > AMDVI_MMIO_SIZE) {
>>> +        trace_amdvi_mmio_write("error: addr outside region: max ",
>>> +                (uint64_t)AMDVI_MMIO_SIZE, size, val, offset);
>>> +        return;
>>> +    }
>>> +
>>> +    switch (addr & ~0x07) {
>>> +    case AMDVI_MMIO_CONTROL:
>>> +        trace_amdvi_mmio_write("MMIO_CONTROL", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr,  val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>
>> This pattern repeats and screams for being factored out into a helper
>> function.
>>
>>> +        amdvi_handle_control_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_DEVICE_TABLE:
>>> +        trace_amdvi_mmio_write("MMIO_DEVICE_TABLE", addr, size, val, 
>>> offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +
>>> +       /*  set device table address
>>> +        *   This also suffers from inability to tell whether software
>>> +        *   is done writing
>>> +        */
>>> +
>>> +        if (offset || (size == 8)) {
>>> +            amdvi_handle_devtab_write(s);
>>> +        }
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_HEAD:
>>> +        trace_amdvi_mmio_write("MMIO_COMMAND_HEAD", addr, size, val, 
>>> offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +
>>> +        amdvi_handle_cmdhead_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_BASE:
>>> +        trace_amdvi_mmio_write("MMIO_COMMAND_BASE", addr, size, val, 
>>> offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +
>>> +        /* FIXME - make sure System Software has finished writing incase
>>> +         * it writes in chucks less than 8 bytes in a robust way.As for
>>> +         * now, this hacks works for the linux driver
>>> +         */
>>> +        if (offset || (size == 8)) {
>>> +            amdvi_handle_cmdbase_write(s);
>>> +        }
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_COMMAND_TAIL:
>>> +        trace_amdvi_mmio_write("MMIO_COMMAND_TAIL", addr, size, val, 
>>> offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_cmdtail_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_BASE:
>>> +        trace_amdvi_mmio_write("MMIO_EVENT_BASE", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_evtbase_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_HEAD:
>>> +        trace_amdvi_mmio_write("MMIO_EVENT_HEAD", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_evthead_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EVENT_TAIL:
>>> +        trace_amdvi_mmio_write("MMIO_EVENT_TAIL", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_evttail_write(s);
>>> +        break;
>>> +
>>> +    case AMDVI_MMIO_EXCL_LIMIT:
>>> +        trace_amdvi_mmio_write("MMIO_EXCL_LIMIT", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_excllim_write(s);
>>> +        break;
>>> +
>>> +        /* PPR log base - unused for now */
>>> +    case AMDVI_MMIO_PPR_BASE:
>>> +        trace_amdvi_mmio_write("MMIO_PPR_BASE", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_pprbase_write(s);
>>> +        break;
>>> +        /* PPR log head - also unused for now */
>>> +    case AMDVI_MMIO_PPR_HEAD:
>>> +        trace_amdvi_mmio_write("MMIO_PPR_HEAD", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_pprhead_write(s);
>>> +        break;
>>> +        /* PPR log tail - unused for now */
>>> +    case AMDVI_MMIO_PPR_TAIL:
>>> +        trace_amdvi_mmio_write("MMIO_PPR_TAIL", addr, size, val, offset);
>>> +        if (size == 2) {
>>> +            amdvi_writew(s, addr, val);
>>> +        } else if (size == 4) {
>>> +            amdvi_writel(s, addr, val);
>>> +        } else if (size == 8) {
>>> +            amdvi_writeq(s, addr, val);
>>> +        }
>>> +        amdvi_handle_pprtail_write(s);
>>> +        break;
>>> +
>>> +        /* ignore write to ext_features */
>>> +    default:
>>> +        trace_amdvi_mmio_write("UNHANDLED MMIO", addr, size, val, offset);
>>> +    }
>>> +
>>> +}
>>> +
>>> +
>>> +/* PCI SIG constants */
>>> +#define PCI_BUS_MAX 256
>>> +#define PCI_SLOT_MAX 32
>>> +#define PCI_FUNC_MAX 8
>>> +#define PCI_DEVFN_MAX 256
>>
>> Shouldn't those four go to the pci header?
> 
> The macros/defines in PCI header are picked from linux while some of
> these are not picked from linux. I'v prefixed them with AMDVI_ though.

They are not AMDVI-specific, rather PCI-generic.

Jan

signature.asc
Description: OpenPGP digital signature