On Sun, May 15, 2016 at 10:29 PM, Jan Kiszka <jan.kis...@web.de> 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 >