Re: [Xen-devel] [v7][RFC][PATCH 08/13] xen/x86/p2m: set p2m_access_n for reserved device memory mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -686,6 +686,30 @@ guest_physmap_add_entry(struct domain *d, unsigned long 
> gfn,
>      /* Now, actually do the two-way mapping */
>      if ( mfn_valid(_mfn(mfn)) ) 
>      {
> +
> +        if ( !is_hardware_domain(d) )
> +        {
> +            rc = 
> iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
> +                                                  &gfn);

Okay, no I see what that function is needed for. It being an inline
function is of course very questionable looking at this use site.

> +            if ( rc )

And the return value from the called function is of type int -
non-zero may not just mean "true" but (when negative) also
"error". You need to distinguish these cases.

> +            {
> +                /*
> +                 * Just set p2m_access_n in case of shared-ept
> +                 * or non-shared ept but 1:1 mapping.
> +                 */
> +                if ( iommu_use_hap_pt(d) ||
> +                     (!iommu_use_hap_pt(d) && mfn == gfn) )

How would, other than by chance, mfn equal gfn here? Also the
double use of iommu_use_hap_pt(d) is pointless here.

> +                {
> +                    rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
> +                                       p2m_access_n);
> +                    if ( rc )
> +                        gdprintk(XENLOG_WARNING, "set rdm p2m failed: 
> (%#lx)\n",
> +                                 gfn);

Such messages are (due to acting on a foreign domain) relatively
useless without also logging the domain that is affected. Conversely,
logging the current domain and vCPU (due to using gdprintk()) is
rather pointless. Also please drop either the colon or the
parentheses in the message.

Jan


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