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

[Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>]

On 04/06/2020 11:50, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 04 June 2020 11:34
>> To: paul@xxxxxxx
>> Cc: 'Igor Druzhinin' <igor.druzhinin@xxxxxxxxxx>; 
>> xen-devel@xxxxxxxxxxxxxxxxxxxx;
>> andrew.cooper3@xxxxxxxxxx; wl@xxxxxxx; roger.pau@xxxxxxxxxx; 
>> george.dunlap@xxxxxxxxxx
>> Subject: Re: [PATCH for-4.14 v3] x86/svm: do not try to handle recalc NPT 
>> faults immediately
>>
>> 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.

You may replace it with 'associated' or something if you don't like this word.

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

It seems that the case of simple MMIO mappings made without IOMMU being
enabled for a domain, in fact, irrelevant to the this condition.
I take it as a separate discussion on a different topic.

Igor