Re: [PATCH for-4.14 v3] x86/svm: do not try to handle recalc NPT faults immediately

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.06.2020 12:50, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 04 June 2020 11:34
>>
>> On 04.06.2020 09:49, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
>>>> Sent: 03 June 2020 23:42
>>>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Cc: jbeulich@xxxxxxxx; andrew.cooper3@xxxxxxxxxx; wl@xxxxxxx; 
>>>> roger.pau@xxxxxxxxxx;
>>>> george.dunlap@xxxxxxxxxx; paul@xxxxxxx; Igor Druzhinin 
>>>> <igor.druzhinin@xxxxxxxxxx>
>>>> Subject: [PATCH for-4.14 v3] x86/svm: do not try to handle recalc NPT 
>>>> faults immediately
>>>>
>>>> A recalculation NPT fault doesn't always require additional handling
>>>> in hvm_hap_nested_page_fault(), moreover in general case if there is no
>>>> explicit handling done there - the fault is wrongly considered fatal.
>>>>
>>>> This covers a specific case of migration with vGPU assigned which
>>>> uses direct MMIO mappings made by XEN_DOMCTL_memory_mapping hypercall:
>>>> at a moment log-dirty is enabled globally, recalculation is requested
>>>> for the whole guest memory including those mapped MMIO regions
>>>
>>> I still think it is odd to put this in the commit comment since, as I
>>> said before, Xen ensures that this situation cannot happen at
>>> the moment.
>>
>> Aiui Igor had replaced reference to passed-through devices by reference
>> to mere handing of an MMIO range to a guest. Are you saying we suppress
>> log-dirty enabling in this case as well? I didn't think we do:
> 
> No, but the comment says "migration with vGPU *assigned*" (my emphasis), 
> which surely means has_arch_pdevs() will be true.
> 
>>
>>     if ( has_arch_pdevs(d) && log_global )
>>     {
>>         /*
>>          * Refuse to turn on global log-dirty mode
>>          * if the domain is sharing the P2M with the IOMMU.
>>          */
>>         return -EINVAL;
>>     }
>>
>> Seeing this code I wonder about the non-sharing case: If what the
>> comment says was true, the condition would need to change, but I
>> think it's the comment which is wrong, and we don't want global
>> log-dirty as long as an IOMMU is in use at all for a domain.
> 
> I think is the comment that is correct, not the condition. It is
> only when using shared EPT that enabling logdirty is clearly an
> unsafe thing to do. Using sync-ed IOMMU mappings should be ok.

Even with sync-ed IOMMU mappings dirtying happening by I/O won't be
noticed, and hence the purpose of global log-dirty is undermined.

Jan