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 12:47
> 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 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.

It is, but there are point solutions in some devices and, if not in the device, 
in the emulator managing the device. This is why migration with assigned h/w is 
currently feasible even without IOMMU faulting.

  Paul

> 
> Jan