[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> 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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.