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

["Xu, Anthony" <anthony.xu@xxxxxxxxx>]

Isaku Yamahata wrote:
> 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 didn't remove all mm.c small patches,
I just removed the difficult part, which is related to atomic operation

Please, check in this patch first, I had tested it by booting linux guest.





>
> 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.

I didn't oppose lockless, acctually I use lockless in vhpt and vtlb code.
There are some redundant codes inside mm.c,
It's better to figure a way to merge it.

Thank,
Anthony

>
>
>> 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?

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