Re: [Xen-devel] [PATCH v3] x86/HVM: p2m_ram_ro is incompatible with device pass-through

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Oct 01, 2019 at 11:07:55AM +0200, Jan Beulich wrote:
> The write-discard property of the type can't be represented in IOMMU
> page table entries. Make sure the respective checks / tracking can't
> race, by utilizing the domain lock. The other sides of the sharing/
> paging/log-dirty exclusion checks should subsequently perhaps also be
> put under that lock then.
> 
> This also fixes an unguarded d->arch.hvm access.
> 
> Take the opportunity and also convert neighboring bool_t to bool in
> struct hvm_domain.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> v3: Re-base.
> v2: Don't set p2m_ram_ro_used when failing the request.
> 
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -255,16 +255,33 @@ static int set_mem_type(struct domain *d
>  
>      mem_type = array_index_nospec(data->mem_type, ARRAY_SIZE(memtype));
>  
> -    if ( mem_type == HVMMEM_ioreq_server )
> +    switch ( mem_type )
>      {
>          unsigned int flags;
>  
> +    case HVMMEM_ioreq_server:
>          if ( !hap_enabled(d) )
>              return -EOPNOTSUPP;
>  
>          /* Do not change to HVMMEM_ioreq_server if no ioreq server mapped. */
>          if ( !p2m_get_ioreq_server(d, &flags) )
>              return -EINVAL;
> +
> +        break;
> +
> +    case HVMMEM_ram_ro:
> +        /* p2m_ram_ro can't be represented in IOMMU mappings. */
> +        domain_lock(d);
> +        if ( has_arch_pdevs(d) )

I would use is_iommu_enabled because I think it's clearer in this
context (giving the comment above explicitly refers to having iommu
mappings).

> +            rc = -EXDEV;

EOPNOTSUPP might be better, since it's possible that future iommus
support such page type?

> +        else
> +            d->arch.hvm.p2m_ram_ro_used = true;
> +        domain_unlock(d);
> +
> +        if ( rc )
> +            return rc;
> +
> +        break;
>      }
>  
>      while ( iter < data->nr )
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1486,15 +1486,33 @@ static int assign_device(struct domain *
>      if ( !is_iommu_enabled(d) )
>          return 0;
>  
> -    /* Prevent device assign if mem paging or mem sharing have been 
> -     * enabled for this domain */
> -    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
> -                  vm_event_check_ring(d->vm_event_paging) ||
> +    domain_lock(d);
> +
> +    /*
> +     * Prevent device assignment if any of
> +     * - mem paging
> +     * - mem sharing
> +     * - the p2m_ram_ro type
> +     * - global log-dirty mode
> +     * are in use by this domain.
> +     */
> +    if ( unlikely(vm_event_check_ring(d->vm_event_paging) ||
> +#ifdef CONFIG_HVM
> +                  (is_hvm_domain(d) &&
> +                   (d->arch.hvm.mem_sharing_enabled ||
> +                    d->arch.hvm.p2m_ram_ro_used)) ||
> +#endif
>                    p2m_get_hostp2m(d)->global_logdirty) )

Is such check needed anymore?

With the enabling of the iommu right at domain creation it shouldn't
be possible to enable any of the above features at all anymore.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel