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

[David Kiarie]

On Sun, May 15, 2016 at 10:29 PM, Jan Kiszka <address@hidden> wrote:
> On 2016-05-09 14:15, David Kiarie wrote:
>> +

Thanks for review and testing!

>> +            /* 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.

Yes, this was meant to work with normal pages only. Until recently
intel iommu supported 4k pages only so I figured I could as well work
with 4k pages. Anyway, will fix this.

>
>> +        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.

It was helpful in the initial stages of development, not very helpful
now. I could get rid of such.

>
>> +
>> +    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
>