Re: [Xen-devel] [v8][PATCH 11/17] xen/x86/p2m: reject populating for reserved device memory mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.14 at 10:24, <tiejun.chen@xxxxxxxxx> 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


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