Re: [Xen-ia64-devel] [PATCH 2/3] p2m table expsosure linux side

[Alex Williamson <alex.williamson@xxxxxx>]

Hi Isaku,

   A few more comments...

On Tue, 2006-10-03 at 18:41 +0900, Isaku Yamahata wrote:

>         BUG_ON(privcmd_resource_min >= privcmd_resource_max);
> +
> +       // XXX this should be somewhere appropriate
> +       (void)p2m_expose_init();
> +

   Maybe it's own initcall?  Perhaps a device_initcall() or
subsys_initcall() would work(?)

> +#ifdef notyet
> +void
> +p2m_expose_cleanup(void)
> +{
> +       BUG_ON(!p2m_initialized);
> +       release_resrouce(&p2m_resource);
                  ^^^^^^^^ typo

> +       if (likely(__get_user(pteval, (unsigned long __user *)pte) ==
> 0) &&
> +           likely(pte_present(__pte(pteval))) &&
> +           likely(pte_pfn(__pte(pteval)) != (INVALID_MFN >>
> PAGE_SHIFT)))
> +               mfn = (pteval & _PFN_MASK) >> PAGE_SHIFT;

   I don't think the compiler is going to be able to figure this out
like you intend it to.  It should probably be:

if (likely(_get_user(pteval, (unsigned long __user *)pte) == 0 &&
           pte_present(__pte(pteval)) &&
           pte_pfn(__pte(pteval)) != (INVALID_MFN >> PAGE_SHIFT))

> +static inline unsigned long
> +HYPERVISOR_expose_p2m(unsigned long conv_start_gpfn,
> +                     unsigned long assign_start_gpfn,
> +                     unsigned long expose_size, unsigned long
> granule_pfn)
> +{
> +       unsigned long ret = 0;
> +       if (is_running_on_xen()) {

   This seems like a strange place for a is_running_on_xen() check.
Shouldn't the caller of this be the one aware of running on xen or not?

   Why is p2m_expose_init() exported?  Only for the test module?  The
test module seems like a nice benchmarking utility, but it doesn't seem
like something that would get into the upstream kernel.  Do we really
want to expose this level of detail of the p2m operations to everyone
via these exports?  Thanks,

        Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.


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