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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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

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

  Paul