On Tue, Mar 15, 2016 at 10:52 AM, Peter Xu <pet...@redhat.com> wrote:
> On Mon, Mar 14, 2016 at 08:52:33PM +0200, Marcel Apfelbaum wrote:
> > On 03/12/2016 06:13 PM, Aviv B.D. wrote:
> > Adding (possibly) interested developers to the thread.
>
> Thanks CC.
>
> Hi, Aviv, several questions inline.
>
> [...]
>
> > >@@ -1020,14 +1037,53 @@ static void
> vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> > > hwaddr addr, uint8_t am)
> > > {
> > > VTDIOTLBPageInvInfo info;
> > >+ VFIOGuestIOMMU * giommu;
> > >+ bool flag = false;
> > > assert(am <= VTD_MAMV);
> > > info.domain_id = domain_id;
> > > info.addr = addr;
> > > info.mask = ~((1 << am) - 1);
> > >+
> > >+ QLIST_FOREACH(giommu, &(s->giommu_list), iommu_next){
> > >+ VTDAddressSpace *vtd_as = container_of(giommu->iommu,
> VTDAddressSpace, iommu);
> > >+ uint16_t vfio_source_id =
> vtd_make_source_id(pci_bus_num(vtd_as->bus), vtd_as->devfn);
> > >+ uint16_t vfio_domain_id = vtd_get_did_dev(s,
> pci_bus_num(vtd_as->bus), vtd_as->devfn);
> > >+ if (vfio_domain_id != (uint16_t)-1 &&
>
> Could you (or anyone) help explain what does vfio_domain_id != -1
> mean?
vtd_get_did_dev returns -1 if the device is not mapped to any domain
(generally, the CE is not present).
probably a better interface is to return whether the device has a domain or
not and returns the domain_id via the pointer argument.
>
> > >+ domain_id == vfio_domain_id){
> > >+ VTDIOTLBEntry *iotlb_entry = vtd_lookup_iotlb(s,
> vfio_source_id, addr);
> > >+ if (iotlb_entry != NULL){
>
> Here, shall we notify VFIO even if the address is not cached in
> IOTLB? Anyway, we need to do the unmap() of the address, am I
> correct?
>
With this code I do a unmap operation if the address was cached in the
IOTLB, if not I'm assuming that the current invalidation invalidate an
(previously) non present address and I should do a map operation (during
the map operation I'm calling s->iommu_ops.translate that caches the
address).
>
> > >+ IOMMUTLBEntry entry;
> > >+ VTD_DPRINTF(GENERAL, "Remove addr 0x%"PRIx64 " mask
> %d", addr, am);
> > >+ entry.iova = addr & VTD_PAGE_MASK_4K;
> > >+ entry.translated_addr =
> vtd_get_slpte_addr(iotlb_entry->slpte) & VTD_PAGE_MASK_4K;
> > >+ entry.addr_mask = ~VTD_PAGE_MASK_4K;
> > >+ entry.perm = IOMMU_NONE;
> > >+ memory_region_notify_iommu(giommu->iommu, entry);
> > >+ flag = true;
> > >+
> > >+ }
> > >+ }
> > >+
> > >+ }
> > >+
> > > g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page,
> &info);
> > >-}
> > >+ QLIST_FOREACH(giommu, &(s->giommu_list), iommu_next){
> > >+ VTDAddressSpace *vtd_as = container_of(giommu->iommu,
> VTDAddressSpace, iommu);
> > >+ uint16_t vfio_domain_id = vtd_get_did_dev(s,
> pci_bus_num(vtd_as->bus), vtd_as->devfn);
> > >+ if (vfio_domain_id != (uint16_t)-1 &&
> > >+ domain_id == vfio_domain_id && !flag){
> > >+ /* do vfio map */
> > >+ VTD_DPRINTF(GENERAL, "add addr 0x%"PRIx64 " mask %d",
> addr, am);
> > >+ /* call to vtd_iommu_translate */
> > >+ IOMMUTLBEntry entry =
> s->iommu_ops.translate(giommu->iommu, addr, 0);
> > >+ entry.perm = IOMMU_RW;
> > >+ memory_region_notify_iommu(giommu->iommu, entry);
> > >+ //g_vfio_iommu->n.notify(&g_vfio_iommu->n, &entry);
> > >+ }
> > >+ }
> > >+}
>
> I see that we did handled all the page invalidations. Would it
> possible that guest kernel send domain/global invalidations? Should
> we handle them too?
>
You are correct, currently this code is pretty much at POC level, and i
support only the page invalidation because this is what linux is using. The
final version should support also those invalidation ops.
>
> [...]
>
> > > static void vfio_listener_region_add(MemoryListener *listener,
> > > MemoryRegionSection *section)
> > > {
> > >@@ -344,6 +347,7 @@ static void vfio_listener_region_add(MemoryListener
> *listener,
> > > iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
> > > llend = int128_make64(section->offset_within_address_space);
> > > llend = int128_add(llend, section->size);
> > >+ llend = int128_add(llend, int128_exts64(-1));
>
> Here, -1 should fix the assertion core dump. However, shall we also
> handle all the rest places that used "llend" (possibly with variable
> "end") too? For example, at the end of current function, when we map
> dma regions:
>
> ret = vfio_dma_map(container, iova, end - iova, vaddr,
> section->readonly);
>
> To:
>
> ret = vfio_dma_map(container, iova, end + 1 - iova, vaddr,
> section->readonly);
>
> I will add this to the next version of the patch, thanks!
Thanks.
> Peter
>
