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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2019 16:15, Roger Pau Monné  wrote:
> On Tue, Oct 01, 2019 at 01:40:57PM +0200, Jan Beulich wrote:
>> 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/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) ||
> 
> Would be nice to have some syntactic sugar like vm_event_enabled or
> some such.

I'll leave that to the VM event maintainers.

>>>> +#ifdef CONFIG_HVM
>>>> +                  (is_hvm_domain(d) &&
>>>> +                   (d->arch.hvm.mem_sharing_enabled ||
>>>> +                    d->arch.hvm.p2m_ram_ro_used)) ||
>>>> +#endif
> 
> Do you really need the CONFIG_HVM guards? is_hvm_domain already has a
> IS_ENABLED(CONFIG_HVM).

Strictly speaking at this point in time it wouldn't be needed.
But eventually I think we will want such, as there's no point
having a bigger than necessary struct arch_domain (and struct
arch_vcpu) in a !HVM build. Achieving that would likely imply
though that the entire d->arch.hvm disappears, and hence
without some kind of guards the above would then fail to
compile. (I have accumulated quite a bit of related work
already, which is probably why I felt like adding the #ifdef-s
here.)

>>>>                    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.
> 
> Oh right, I was missing the whole point then. So we still keep the
> iommu enabled together with introspection or ram_ro as long as there
> are no devices assigned.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

Jan

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