Re: [Xen-devel] [v3][PATCH 02/16] xen/x86/p2m: introduce set_identity_p2m_entry

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> wrote:
> We will create this sort of identity mapping as follows:
> 
> If the gfn space is unoccupied, we just set the mapping. If the space
> is already occupied by 1:1 mappings, do nothing. Failed for any
> other cases.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>

First of all you continue to be copying each patch to every
maintainer involved in some part of the series. Please limit the
Cc list of each patch to the actual list of people needing to be
Cc-ed on it (or you know explicitly wanting a copy).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -898,6 +898,41 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>      return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
>  }
>  
> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> +                           p2m_access_t p2ma)
> +{
> +    p2m_type_t p2mt;
> +    p2m_access_t a;
> +    mfn_t mfn;
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int ret;
> +
> +    if ( paging_mode_translate(p2m->domain) )
> +    {
> +        gfn_lock(p2m, gfn, 0);
> +
> +        mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
> +
> +        if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN )

I'm not fundamentally opposed to this extra INVALID_MFN check, but
could you please clarify for which P2M type you saw INVALID_MFN
coming back here, and for which p2m_invalid cases you didn't also
see INVALID_MFN? I.e. I'd really prefer a single check to be used
when that can cover all cases.

> +            ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> +                                p2m_mmio_direct, p2ma);
> +        else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> +            ret = 0;
> +        else
> +        {
> +            ret = -EBUSY;
> +            printk(XENLOG_G_WARNING
> +                   "Cannot identity map d%d:%lx, already mapped to %lx.\n",
> +                   d->domain_id, gfn, mfn_x(mfn));
> +        }
> +
> +        gfn_unlock(p2m, gfn, 0);
> +        return ret;
> +    }
> +
> +    return 0;
> +}

Either have a single return point, or invert the original if() and bail
early (reducing the indentation level on the main body of the code).

Jan


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