Re: [Xen-devel] [RFC 0 PATCH 2/3] PVH dom0: move some pv specific code to static functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.13 at 23:03, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> +static __init void setup_pv_p2m_table(

Let's call this setup_pv_physmap() or some such; avoid including p2m
in the name in particular.

> +    struct domain *d, struct vcpu *v, struct elf_dom_parms *parms,
> +    unsigned long v_start, unsigned long vphysmap_start,
> +    unsigned long vphysmap_end, unsigned long v_end, unsigned long nr_pages)

Parameters could be grouped a little more logically.

> +{
> +    struct page_info *page = NULL;
> +    l4_pgentry_t *l4tab = NULL, *l4start = NULL;
> +    l3_pgentry_t *l3tab = NULL;
> +    l2_pgentry_t *l2tab = NULL;
> +    l1_pgentry_t *l1tab = NULL;
> +
> +    l4start = map_domain_page(pagetable_get_pfn(v->arch.guest_table));

This appears to be the only use of "v", so it'd be more logical to
pass pagetable_get_pfn(v->arch.guest_table) or
v->arch.guest_table.

Also, with l4start being initialized here, there's no point in setting
it to NULL in its declaration.

> +    l3tab = NULL;
> +    l2tab = NULL;
> +    l1tab = NULL;

Their declarations above have initializers, so what's the point of
these?

> +
> +    /* Set up the phys->machine table if not part of the initial mapping. */
> +    if ( parms->p2m_base != UNSET_ADDR )

This check could now be easily left at the call site, thus reducing
indentation of the entire body below. Which would perhaps allow
not passing parms to this function at all.

> +    {
> +        unsigned long va = vphysmap_start;

Now that you would no longer clobber vphysmap_start (in the caller),
there's no need for a separate variable here. 

> +
> +        if ( v_start <= vphysmap_end && vphysmap_start <= v_end )
> +            panic("DOM0 P->M table overlaps initial mapping");
> +
> +        while ( va < vphysmap_end )
> +        {
> +            if ( d->tot_pages + ((round_pgup(vphysmap_end) - va)
> +                                 >> PAGE_SHIFT) + 3 > nr_pages )
> +                panic("Dom0 allocation too small for initial P->M table.\n");
> +
> +            if ( l1tab )
> +            {
> +                unmap_domain_page(l1tab);
> +                l1tab = NULL;
> +            }
> +            if ( l2tab )
> +            {
> +                unmap_domain_page(l2tab);
> +                l2tab = NULL;
> +            }
> +            if ( l3tab )
> +            {
> +                unmap_domain_page(l3tab);
> +                l3tab = NULL;
> +            }
> +            l4tab = l4start + l4_table_offset(va);

The way this l4tab gets used here (and l[123]tab below) makes
these names bogus - they should be pl[1234]e instead. The
original uses were just attributed to the desire of re-using
existing variables, which is mute with this stuff getting moved
into its own function.

This also applies to the similarly named variables in
mark_pv_pt_pages_rdonly().

Jan


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