Re: [PATCH v2] x86/svm: do not try to handle recalc NPT faults immediately

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.06.2020 18:56, Igor Druzhinin wrote:
> 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 on AMD:
> at a moment log-dirty is enabled globally, recalculation is requested
> for the whole guest memory including directly mapped MMIO regions of vGPU
> which causes a page fault being raised at the first access to those;
> but due to MMIO P2M type not having any explicit handling in
> hvm_hap_nested_page_fault() a domain is erroneously crashed with unhandled
> SVM violation.
> 
> Instead of trying to be opportunistic - use safer approach and handle
> P2M recalculation in a separate NPT fault by attempting to retry after
> making the necessary adjustments. This is aligned with Intel behavior
> where there are separate VMEXITs for recalculation and EPT violations
> (faults) and only faults are handled in hvm_hap_nested_page_fault().
> Do it by also unifying do_recalc return code with Intel implementation
> where returning 1 means P2M was actually changed.
> 
> Since there was no case previously where p2m_pt_handle_deferred_changes()
> could return a positive value - it's safe to replace ">= 0" with just "== 0"
> in VMEXIT_NPF handler. finish_type_change() is also not affected by the
> change as being able to deal with >0 return value of p2m->recalc from
> EPT implementation.
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit preferably with ...

> @@ -448,12 +451,14 @@ static int do_recalc(struct p2m_domain *p2m, unsigned 
> long gfn)
>              clear_recalc(l1, e);
>          err = p2m->write_p2m_entry(p2m, gfn, pent, e, level + 1);
>          ASSERT(!err);
> +
> +        recalc_done = true;
>      }
>  
>   out:
>      unmap_domain_page(table);
>  
> -    return err;
> +    return err ?: (recalc_done ? 1 : 0);

... this shrunk to

    return err ?: recalc_done;

(easily doable while committing).

Also Cc Paul.

Jan