From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH v7 12/14] x86: intel-iommu: add dmar test Date: Wed, 7 Dec 2016 13:09:13 +0800 Message-ID: <20161207050913.GD3469@pxdev.xzpeter.org> References: <1480393550-12385-1-git-send-email-peterx@redhat.com> <1480393550-12385-13-git-send-email-peterx@redhat.com> <20161205093205.GA22576@agordeev.lab.eng.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, drjones@redhat.com, jan.kiszka@web.de, rkrcmar@redhat.com, pbonzini@redhat.com To: Alexander Gordeev Return-path: Received: from mx1.redhat.com ([209.132.183.28]:39470 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752345AbcLGFK1 (ORCPT ); Wed, 7 Dec 2016 00:10:27 -0500 Content-Disposition: inline In-Reply-To: <20161205093205.GA22576@agordeev.lab.eng.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Dec 05, 2016 at 10:32:06AM +0100, Alexander Gordeev wrote: [...] > > +static void vtd_install_pte(vtd_pte_t *root, iova_t iova, > > + phys_addr_t pa, int level_target) > > +{ > > + int level; > > + unsigned int offset; > > + void *page; > > + > > + for (level = VTD_PAGE_LEVEL; level > level_target; level--) { > > + offset = PGDIR_OFFSET(iova, level); > > + if (!(root[offset] & VTD_PTE_RW)) { > > + page = alloc_page(); > > + memset(page, 0, PAGE_SIZE); > > + root[offset] = virt_to_phys(page) | VTD_PTE_RW; > > + } > > + root = (uint64_t *)(root[offset] & VTD_PTE_ADDR); > > Physical to virtual translation is missed. Exactly. Thanks. :) > Also, PAGE_SHIFT implied instead of VTD_PAGE_SHIFT (see below). > > > + } > > + > > + offset = PGDIR_OFFSET(iova, level); > > + root[offset] = pa | VTD_PTE_RW; > > + if (level != 1) { > > + /* This is huge page */ > > + root[offset] |= VTD_PTE_HUGE; > > + } > > +} > > + > > +#define VTD_FETCH_VIRT_ADDR(x) \ > > + ((void *)(((uint64_t)phys_to_virt(x)) >> PAGE_SHIFT)) > > Just a nit. A fetch somehow implies pulling data, while here > we have a translation. What about VTD_PHYS_TO_VIRT? Sure. Will rename. [...] > > + if (!ce->present) { > > + slptptr = alloc_page(); > > + memset(slptptr, 0, PAGE_SIZE); > > + memset(ce, 0, sizeof(*ce)); > > + /* To make it simple, domain ID is the same as SID */ > > + ce->domain_id = sid; > > + /* We only test 39 bits width case (3-level paging) */ > > + ce->addr_width = VTD_CE_AW_39BIT; > > + ce->slptptr = virt_to_phys(slptptr) >> PAGE_SHIFT; > > It seems left shift is needed here, not the right one. I still think the right shift is what we want. Here ce->slptptr is a bitfield. It stores the bits HAW:12 (maximum is 63:12) of second level page table pointer address, so I need a right shift, no? > > Also, using PAGE_SHIFT (and possible other memory constants throughout > the source) looks wrong to me. Instead it should be VTD_PAGE_SHIFT (and > alike) - no matter they are equal to the memory constants at the moment. Hmm sure. I can define one for vt-d. [...] > > +#define VTD_PTE_R (1 << 0) > > +#define VTD_PTE_W (1 << 1) > > +#define VTD_PTE_RW (VTD_PTE_R | VTD_PTE_W) > > +#define VTD_PTE_ADDR GENMASK_ULL(51, 12) > > Are we safe to include 51:HAW lines? They are marked as Reserved > and we might write 1s to these bits occasionally. I do not really > believe it could cause trouble ever - just need some clarity here. Actually here I *guess* what I wanted was: GENMASK_ULL(63, 12) Luckily 51 is enough for us (because currently we only support 3 level page table, this will work as long as this number is bigger than 39). However I think I should use 63 here... Regarding to the reserved bits you mentioned - IMO it's okay to include them since the reserved bits should be set to zero by guest software. Thanks, -- peterx