From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [v8][PATCH 11/17] xen/x86/p2m: reject populating
 for reserved device memory mapping
Date: Thu, 04 Dec 2014 16:42:09 +0000
Message-ID: <54809CF1020000780004CDC6@mail.emea.novell.com>
References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com>
	<1417425875-9634-12-git-send-email-tiejun.chen@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
In-Reply-To: <1417425875-9634-12-git-send-email-tiejun.chen@intel.com>
Content-Disposition: inline
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Tiejun Chen <tiejun.chen@intel.com>
Cc: kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com

>>> On 01.12.14 at 10:24, <tiejun.chen@intel.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -556,6 +556,40 @@ guest_physmap_remove_page(struct domain *d, unsigned long gfn,
>      gfn_unlock(p2m, gfn, page_order);
>  }
>  
> +/* Check if we are accessing rdm. */

If a comment doesn't do anything but re-state a function name,
it's imo superfluous.

> +int p2m_check_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
> +                                     u32 id, void *ctxt)
> +{
> +    xen_pfn_t end = start + nr;
> +    unsigned int i;
> +    u32 sbdf;
> +    struct p2m_get_reserved_device_memory *pgrdm = ctxt;
> +    struct domain *d = pgrdm->domain;
> +
> +    if ( d->arch.hvm_domain.pci_force )
> +    {
> +        if ( pgrdm->gfn >= start && pgrdm->gfn < end )
> +            return 1;
> +    }
> +    else
> +    {
> +        for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ )
> +        {
> +            sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg,
> +                             d->arch.hvm_domain.pcidevs[i].bus,
> +                             d->arch.hvm_domain.pcidevs[i].devfn);
> +
> +            if ( sbdf == id )
> +            {
> +                if ( pgrdm->gfn >= start && pgrdm->gfn < end )
> +                    return 1;
> +            }

Please join together if()s like these.

> @@ -686,8 +721,28 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>      /* Now, actually do the two-way mapping */
>      if ( mfn_valid(_mfn(mfn)) ) 
>      {
> -        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
> -                           p2m->default_access);
> +        pgrdm.gfn = gfn;
> +        pgrdm.domain = d;
> +        if ( !is_hardware_domain(d) && iommu_use_hap_pt(d) )
> +        {
> +            rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory,
> +                                                  &pgrdm);
> +            /* We always avoid populating reserved device memory. */
> +            if ( rc == 1 )
> +            {
> +                rc = -EBUSY;
> +                goto out;

Did I overlook something in the earlier tool stack patches? How does
the tool stack avoid populating these areas?

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -709,6 +709,15 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
>      return flags;
>  }
>  
> +struct p2m_get_reserved_device_memory {
> +    unsigned long gfn;
> +    struct domain *domain;
> +};
> +
> +/* Check if we are accessing rdm. */
> +extern int p2m_check_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
> +                                            u32 id, void *ctxt);

Are subsequent patches going to make use of this outside of p2m.c?
If not, these declarations don't belong here. And even if the
function was going to be used elsewhere, the structure wouldn't
need defining here.

Jan