Re: [Xen-ia64-devel][PATCH][VTD] small patches for VTD

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Oct 22, 2008 at 05:35:36PM +0800, Xu, Anthony wrote:
> The new one,
> 
> 
> >> -    if (phy_pte.ma != VA_MATTR_NATPAGE)
> >> +    /* if a device is assigned to a domain through VTD, the MMIO
> >> for this +     * device needs to retain to UC attribute
> >> +     */
> >> +    if (phy_pte.ma == VA_MATTR_WC)
> >>          phy_pte.ma = VA_MATTR_WB;
> >>
> >
> > Doesn't this break the intention of the c/s 15134:466f71b1e831?a
> > To be honest, I'm not sure. Kyouya or Akio, do you have any comments?
> >
> This section is not included, need kyouya or akio confirmation.
> 
> Patches about mm.c is not inculded,
> I'll send out a separate patch.

Sounds good. the stuff in mm.c seems very tough.
However the following patch still touches mm.c.
Did you forget to remove it accidently?

I had given it consideration a bit.
I suppose the lockless implementation is possible.
In fact tlb insert case is handled by retry using the p2m entry
as change indicator. See ia64_do_page_fault(), vcpu_itc_i() and
vcpu_itc_d().
iommu case could be handled similary.


> diff -r 02c8733e2d91 xen/arch/ia64/vmx/viosapic.c
> --- a/xen/arch/ia64/vmx/viosapic.c    Wed Oct 22 17:20:15 2008 +0900
> +++ b/xen/arch/ia64/vmx/viosapic.c    Wed Oct 22 17:08:32 2008 +0800
> @@ -121,6 +121,13 @@
>                       redir_num, vector);
>          return;
>      }
> +    if ( iommu_enabled )
> +    {
> +        spin_unlock(&viosapic->lock);
> +        hvm_dpci_eoi(current->domain, redir_num, 
> &viosapic->redirtbl[redir_num]);
> +        spin_lock(&viosapic->lock);
> +    }
> +
>      service_iosapic(viosapic);
>      spin_unlock(&viosapic->lock);
>  }

viosapic->isr and irr must handled atomically.
So unlocking and locking again breaks the requirement.
(I haven't looked the viosapic code very closely, though.
So I may be wrong.)


> diff -r 02c8733e2d91 xen/arch/ia64/xen/mm.c
> --- a/xen/arch/ia64/xen/mm.c  Wed Oct 22 17:20:15 2008 +0900
> +++ b/xen/arch/ia64/xen/mm.c  Wed Oct 22 17:08:32 2008 +0800
> @@ -1427,6 +1427,8 @@
>      if (mfn == INVALID_MFN) {
>          // clear pte
>          old_pte = ptep_get_and_clear(mm, mpaddr, pte);
> +        if(!pte_mem(old_pte))
> +            return;
>          mfn = pte_pfn(old_pte);
>      } else {
>          unsigned long old_arflags;
> @@ -1463,6 +1465,13 @@
>      perfc_incr(zap_domain_page_one);
>      if(!mfn_valid(mfn))
>          return;
> +
> +    {
> +        int i, j;
> +        j = 1 << (PAGE_SHIFT-PAGE_SHIFT_4K);
> +        for(i = 0 ; i < j; i++)
> +            iommu_unmap_page(d, (mpaddr>>PAGE_SHIFT)*j + i);
> +    }
>  
>      page = mfn_to_page(mfn);
>      BUG_ON((page->count_info & PGC_count_mask) == 0);
> @@ -2844,10 +2853,16 @@
>  __guest_physmap_add_page(struct domain *d, unsigned long gpfn,
>                           unsigned long mfn)
>  {
> +    int i, j;
> +
>      set_gpfn_from_mfn(mfn, gpfn);
>      smp_mb();
>      assign_domain_page_replace(d, gpfn << PAGE_SHIFT, mfn,
>                                 ASSIGN_writable | ASSIGN_pgc_allocated);
> +    j = 1 << (PAGE_SHIFT-PAGE_SHIFT_4K);
> +    for(i = 0 ; i < j; i++)
> +        iommu_map_page(d, gpfn*j + i, mfn*j + i);
> +
>  }
>  
>  int

The same loop logic. Introducing a helper function would help?

-- 
yamahata

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