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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 03 July 2019 12:36
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>; Paul Durrant 
> <Paul.Durrant@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>
> Subject: [PATCH v2] x86/HVM: p2m_ram_ro is incompatible with device 
> pass-through
> 
> 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.
> 
> Take the opportunity and also convert neighboring bool_t to bool in
> struct hvm_domain.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> 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_iommu_pt(d) )
> +            rc = -EXDEV;
> +        else
> +            d->arch.hvm.p2m_ram_ro_used = true;

Do we really want this to be a one-way trip? On the face of it, it would seem 
that keeping a count of p2m_ram_ro entries would be more desirable such that, 
once the last one is gone, devices can be assigned again?
If not maybe it's time to go all the way and make iommu page table construction 
part of domain create and then we simplify a lot of code and we don't need any 
flag/refcount like this at all.

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