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

[Jan Kiszka]

On 2016-05-09 14:15, David Kiarie wrote:
> +static void amdvi_page_walk(AMDVIAddressSpace *as, uint64_t *dte,
> +                           IOMMUTLBEntry *ret, unsigned perms,
> +                           hwaddr addr)
> +{
> +    unsigned level, present, pte_perms;
> +    uint64_t pte = dte[0], pte_addr;
> +
> +    /* make sure the DTE has TV = 1 */
> +    if (pte & AMDVI_DEV_TRANSLATION_VALID) {
> +        level = get_pte_translation_mode(pte);
> +        if (level >= 7) {
> +            AMDVI_DPRINTF(MMU, "error: translation level 0x%"PRIu8 " 
> detected"
> +                    " while translating 0x%"PRIx64, level, addr);
> +            return;
> +        }
> +        if (level == 0) {
> +            goto no_remap;
> +        }
> +
> +        while (level > 0) {
> +            pte_perms = amdvi_get_perms(pte);
> +            present = pte & 1;
> +            if (!present || perms != (perms & pte_perms)) {
> +                amdvi_page_fault(as->iommu_state, as->devfn, addr, perms);
> +                AMDVI_DPRINTF(MMU, "error: page fault accessing virtual addr 
> "
> +                              "0x%"PRIx64, addr);
> +                return;
> +            }
> +
> +            /* go to the next lower level */
> +            pte_addr = pte & AMDVI_DEV_PT_ROOT_MASK;
> +            /* add offset and load pte */
> +            pte_addr += ((addr >> (3 + 9 * level)) & 0x1FF) << 3;
> +            pte = ldq_phys(&address_space_memory, pte_addr);
> +            level = get_pte_translation_mode(pte);
> +        }
> +        /* get access permissions from pte */

That comment is only addressing the last assignment of the followings.

> +        ret->iova = addr & AMDVI_PAGE_MASK_4K;
> +        ret->translated_addr = (pte & AMDVI_DEV_PT_ROOT_MASK) &
> +                                AMDVI_PAGE_MASK_4K;
> +        ret->addr_mask = ~AMDVI_PAGE_MASK_4K;

This does not take huge pages (2M, 1G, ...) into account. Jailhouse
creates them, and its Linux guest goes mad. You need to use the correct
page size here, analogously to intel_iommu.c.

> +        ret->perm = amdvi_get_perms(pte);
> +        return;
> +    }
> +
> +no_remap:
> +    ret->iova = addr & AMDVI_PAGE_MASK_4K;
> +    ret->translated_addr = addr & AMDVI_PAGE_MASK_4K;
> +    ret->addr_mask = ~AMDVI_PAGE_MASK_4K;
> +    ret->perm = amdvi_get_perms(pte);
> +
> +}
> +
> +/* TODO : Mark addresses as Accessed and Dirty */
> +static void amdvi_do_translate(AMDVIAddressSpace *as, hwaddr addr,
> +                               bool is_write, IOMMUTLBEntry *ret)
> +{
> +    AMDVIState *s = as->iommu_state;
> +    uint16_t devid = PCI_DEVID(as->bus_num, as->devfn);
> +    AMDVIIOTLBEntry *iotlb_entry = amdvi_iotlb_lookup(s, addr, as->devfn);
> +    uint64_t entry[4];
> +
> +    if (iotlb_entry) {
> +        AMDVI_DPRINTF(CACHE, "hit  iotlb devid: %02x:%02x.%x gpa 0x%"PRIx64
> +                      " hpa 0x%"PRIx64, PCI_BUS_NUM(devid), PCI_SLOT(devid),
> +                      PCI_FUNC(devid), addr, iotlb_entry->translated_addr);
> +        ret->iova = addr & AMDVI_PAGE_MASK_4K;
> +        ret->translated_addr = iotlb_entry->translated_addr;
> +        ret->addr_mask = ~AMDVI_PAGE_MASK_4K;
> +        ret->perm = iotlb_entry->perms;
> +        return;
> +    }
> +
> +    /* devices with V = 0 are not translated */
> +    if (!amdvi_get_dte(s, devid, entry)) {
> +        goto out;
> +    }
> +
> +    amdvi_page_walk(as, entry, ret,
> +                    is_write ? AMDVI_PERM_WRITE : AMDVI_PERM_READ, addr);
> +
> +    amdvi_update_iotlb(s, as->devfn, addr, ret->translated_addr,
> +                       ret->perm, entry[1] & AMDVI_DEV_DOMID_ID_MASK);
> +    return;
> +
> +out:
> +    ret->iova = addr & AMDVI_PAGE_MASK_4K;
> +    ret->translated_addr = addr & AMDVI_PAGE_MASK_4K;
> +    ret->addr_mask = ~AMDVI_PAGE_MASK_4K;
> +    ret->perm = IOMMU_RW;
> +}
> +
> +static inline bool amdvi_is_interrupt_addr(hwaddr addr)
> +{
> +    return addr >= AMDVI_INT_ADDR_FIRST && addr <= AMDVI_INT_ADDR_LAST;
> +}
> +
> +static IOMMUTLBEntry amdvi_translate(MemoryRegion *iommu, hwaddr addr,
> +                                     bool is_write)
> +{
> +    AMDVI_DPRINTF(GENERAL, "");

Not a very helpful instrumentation, I would say.

> +
> +    AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
> +    AMDVIState *s = as->iommu_state;
> +    IOMMUTLBEntry ret = {
> +        .target_as = &address_space_memory,
> +        .iova = addr,
> +        .translated_addr = 0,
> +        .addr_mask = ~(hwaddr)0,
> +        .perm = IOMMU_NONE
> +    };
> +
> +    if (!s->enabled || amdvi_is_interrupt_addr(addr)) {
> +        /* AMDVI disabled - corresponds to iommu=off not
> +         * failure to provide any parameter
> +         */
> +        ret.iova = addr & AMDVI_PAGE_MASK_4K;
> +        ret.translated_addr = addr & AMDVI_PAGE_MASK_4K;
> +        ret.addr_mask = ~AMDVI_PAGE_MASK_4K;
> +        ret.perm = IOMMU_RW;
> +        return ret;
> +    }
> +
> +    amdvi_do_translate(as, addr, is_write, &ret);
> +    AMDVI_DPRINTF(MMU, "devid: %02x:%02x.%x gpa 0x%"PRIx64 " hpa 0x%"PRIx64,
> +                  as->bus_num, PCI_SLOT(as->devfn), PCI_FUNC(as->devfn), 
> addr,
> +                  ret.translated_addr);

Tracing permission here in addition would be good.

Jan

signature.asc
Description: OpenPGP digital signature