Re: [Xen-ia64-devel] [PATCH][RFC][TAKE4] the P2M/VP patches

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

Hi Kevin. Thanks for your comments.


On Fri, Apr 07, 2006 at 02:52:43PM +0800, Tian, Kevin wrote:

> >9519:d6c77b041a70_page_to_bus.patch
> >        introduce page_to_bus() and used for pci-dma-xen.c and
> >swiotlb.c
> >        On xen/ia64 with the P2M/VP model pseudo physical address
> >is fully
> >        virtualized so it defines as
> >        xen_features(XENFEAT_auto_translated_physmap) = 1.
> >        In this case page_to_phys(sg[i].page) should return pseudo
> >physical
> >        address like pfn_to_mfn() and its families.
> >        However dma is not virtualized, it can't be used for
> >pci-dma-xen.c,
> >        swiotlb.c.
> >
> >        If this patch is rejected, we can introduce xenLinux/ia64-specific
> >        pci-dma-xen.c swiotlb.c. But such a divergence is not desirable.
> 
> Seems reasonable. Just one small comment, since xen/ia64 currently
> depends on auto_translated mode, is it possible to make your solution
> common, instead of only special case for xen/ia64? By your patch, x86
> still can't take auto_translated mode as a backend. Not sure whether
> easy/clean to make it, maybe a submode of auto_translated?

Yes, it's possible. And it might not so difficult to make a patch.
You can see what's needed by 'grep _for_dma *.h' under
linux-sparse/include/asm-ia64/.
I'll post a quick hack patch for xenLinux/x86 (but only compile-test)
when I post this patch.

A new conversion function needs to be introduced.
unsigned long pfn_to_mfn_for_dma(unsigned long pfn)
{
        if (xen_feature(XENFEAT_auto_translated_dmaaddr))
                return pfn;
        return real_pfn_to_mfn(pfn);
}

unsigned long pfn_to_mfn(unsigned long pfn)
{
        if (xen_feature(XENFEAT_auto_translated_physmap))
                return pfn;
        return real_pfn_to_mfn(pfn);
}


> >9526:f93f01f21f37_arch_specific_xen_feature.patch
> >        allow arch to define arch-specific xen_feature()
> >        Xen/ia64 with the P2M/VP model define
> >        xen_features(XENFEAT_auto_translated_physmap) = 1.
> >        It is NOT run-time feature. It is desirable to code it explicitly
> >        like
> >        static inline int
> >        xen_feature(int flag)
> >        {
> >               switch (flag)
> >               {
> >               case XENFEAT_auto_translated_physmap:
> >                       return 1;
> >               default:
> >                       return xen_features[flag];
> >               }
> >               /* NOTREACHED */
> >        }
> >        This patch allows it.
> 
> Does this patch only save one hypercall overhead? We can always 
> tell guest the auto_translated bit is true when guest hypercalls to 
> query feature bits into xen_feature array.

No.
I agree that we can go without this patch by telling the bit always 1.
The reasons why I wrote this patch are as follows

- It is more explicit than telling auto_translated bit always 1.
  Perhaps code readers expect that the auto_translated feature
  can be determined at run-time even for xen/ia64, not requirement.
  This is only a readability issue.
  Telling auto_translated bit 1 makes code harder-to-read a bit.

- Compiler can optimize better.

-- 
yamahata

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