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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2019 13:30, Roger Pau Monné  wrote:
> On Tue, Oct 01, 2019 at 11:07:55AM +0200, Jan Beulich wrote:
>> --- 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).

But the whole point of the re-basing over Paul's change is that now
the operation gets refused only if a device was actually assigned.

>> +            rc = -EXDEV;
> 
> EOPNOTSUPP might be better, since it's possible that future iommus
> support such page type?

I don't think future IOMMU behavior affects the choice of error
code. I wanted to use something half way reasonable, yet not too
common, in order to be able to easily identify the source of the
error. If you and others think this isn't a meaningful concern,
I'd be okay switching to -EOPNOTSUPP.

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

See above - all such checks should now be / get converted to check
whether devices are assigned, not whether IOMMU page tables exist.
After all we want to refuse requests only if strictly necessary.

Jan

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