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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
