Re: [PATCH v7 12/14] x86: intel-iommu: add dmar test

[Peter Xu <peterx@redhat.com>]

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