Re: [Xen-ia64-devel] [PATCH] Fix some IPF Xen VT-d bugs

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Dec 24, 2008 at 01:11:03PM +0800, Cui, Dexuan wrote:
> Isaku Yamahata wrote:
> >> diff -r 008b68ff6095 xen/arch/ia64/xen/domain.c
> >> --- a/xen/arch/ia64/xen/domain.c   Tue Nov 18 10:33:55 2008 +0900
> >> +++ b/xen/arch/ia64/xen/domain.c   Mon Dec 15 18:41:52 2008 +0800
> >> @@ -602,10 +602,8 @@ int arch_domain_create(struct domain *d,
> >>    if ((d->arch.mm.pgd = pgd_alloc(&d->arch.mm)) == NULL)              goto
> >> fail_nomem; 
> >> 
> >> -  if ( iommu_enabled && (is_hvm_domain(d) || need_iommu(d)) ){
> >> -          if(iommu_domain_init(d) != 0)
> >> -                  goto fail_iommu;
> >> -  }
> >> +  if(iommu_domain_init(d) != 0)
> >> +          goto fail_iommu;
> >> 
> >>    /*
> >>     * grant_table_create() can't fully initialize grant table for
> >> domain 
> > 
> > Please don't drop is_hvm_domain(d) check.
> > At this moment ia64 doesn't support iommu for PV domain because
> Oh, thanks for the reminder. Here I neglected this.
> 
> Do you mean this:
> if ( is_hvm_domain(d) )
>     if(iommu_domain_init(d) != 0)
>         goto fail_iommu;
> This is also not ok since we must ensure iommu_domain_init() is invoked for 
> Dom0 -- we need the function invoked to enable DMA remapping.
>
> So how about changing the logic to:
> if ( (d->domain_id == 0) || is_hvm_domain(d) )
>     if(iommu_domain_init(d) != 0)
>         goto fail_iommu;
> 
> If you agree this, I'll post a new patch.

Do you mean if ( d->domain_id == 0 ) clause in 
he function, intel_iommu_domain_init()?

Is iommu map/unmap for dom0 is necessary?
  intel_iommu_domain_init() maps all the pages excect ones xen uses
  to dom0. I suppose this is what you want.
  However later pages is mapped/unmapped even for dom0 because
  need_iommu(dom0) returns true due ot iommu_domain_init(dom0).
  Since dom0 is PV, so iommu mapping/unmapping causes race on ia64.
  Only setting up iommu tables at the dom0 creation is necessary,
  all "if ( iommu_enabled && (is_hvm_domain(d) || need_iommu(d)) )"
  would be "if ( iommu_enabled && is_hvm_domain(d) && need_iommu(d)) )"

intel_iommu_domain_init() and dom0 memory size
  calc_dom0_size() in xen/arch/ia64/domain.c calculates default dom0
  memory size. You should take memory for iommu page table
  into account because the memory size for iommu page table wouldn't
  be neglectable.
  probably iommu_pages = (max phys addr) / PTRS_PER_PTE_4K + (some spare)
  where PTRS_PER_PTE_4K = (1 << (PAGE_SHIFT_4K - 3))

intel_iommu_domain_init() and sparse memory.
  To be honest, I'm not sure how it matters in practice.
  On ia64 memory might be located sparsely. So iommu page table
  should also sparse instead of [0, max_page] - (xen page).
  You want to use efi_memmap_walk() instead of for loop.

thanks,
-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel