Re: [Xen-devel] [RFC XEN PATCH 03/16] xen/x86: add a hypercall XENPF_pmem_add to report host pmem regions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.16 at 02:32, <haozhong.zhang@xxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/arch/x86/pmem.c

I wonder whether this should really be x86-specific: It's all ACPI
based, isn't it? I notice that you already place pmem.h that way.

> +static int is_included(unsigned long s1, unsigned long e1,
> +                       unsigned long s2, unsigned long e2)
> +{
> +    return s1 <= s2 && s2 < e2 && e2 <= e1;
> +}

Here and elsewhere, please properly use bool/true/false for
boolean values and (return) types.

> +static int is_overlaped(unsigned long s1, unsigned long e1,
> +                        unsigned long s2, unsigned long e2)
> +{
> +    return (s1 <= s2 && s2 < e1) || (s2 < s1 && s1 < e2);
> +}

is_overlapped() and there's an asymmetry here and conventionally
two comparisons suffice (s1 <= e2 && s2 <= e1, perhaps with the
<= exchanged for <).

> +static int pmem_list_add(unsigned long spfn, unsigned long epfn,
> +                         unsigned long rsv_spfn, unsigned long rsv_epfn,
> +                         unsigned long data_spfn, unsigned long data_epfn)
> +{
> +    struct list_head *cur;
> +    struct pmem *new_pmem;
> +    int ret = 0;
> +
> +    spin_lock(&pmem_list_lock);
> +
> +    list_for_each_prev(cur, &pmem_list)
> +    {
> +        struct pmem *cur_pmem = list_entry(cur, struct pmem, link);
> +        unsigned long cur_spfn = cur_pmem->spfn;
> +        unsigned long cur_epfn = cur_pmem->epfn;
> +
> +        if ( (cur_spfn <= spfn && spfn < cur_epfn) ||
> +             (spfn <= cur_spfn && cur_spfn < epfn) )

is_overlapped()?

> +        {
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +
> +        if ( cur_spfn < spfn )
> +            break;
> +    }
> +
> +    new_pmem = xmalloc(struct pmem);

Please try to avoid allocations with a lock held, unless doing so is
significantly harming code readability.

> +int pmem_add(unsigned long spfn, unsigned long epfn,
> +             unsigned long rsv_spfn, unsigned long rsv_epfn,
> +             unsigned long data_spfn, unsigned long data_epfn)
> +{
> +    int ret;
> +
> +    if ( !pmem_add_check(spfn, epfn, rsv_spfn, rsv_epfn, data_spfn, 
> data_epfn) )
> +        return -EINVAL;
> +
> +    ret = pmem_setup(spfn, epfn, rsv_spfn, rsv_epfn, data_spfn, data_epfn);
> +    if ( ret )
> +        goto out;
> +
> +    ret = iomem_deny_access(current->domain, rsv_spfn, rsv_epfn);
> +    if ( ret )
> +        goto out;
> +
> +    ret = pmem_list_add(spfn, epfn, rsv_spfn, rsv_epfn, data_spfn, 
> data_epfn);
> +    if ( ret )
> +        goto out;
> +
> +    printk(XENLOG_INFO
> +           "pmem: pfns     0x%lx - 0x%lx\n"
> +           "      reserved 0x%lx - 0x%lx\n"
> +           "      data     0x%lx - 0x%lx\n",

%#lx

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel