Re: [PATCH v7 2/4] x86/hvm: Treat non-instruction fetch nested page faults also as read violations

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 08/19/2014 06:39 AM, Tamas K Lengyel wrote:
> As pointed out by Jan Beulich in http://lists.xen.org/archives/html/xen-devel/2014-08/msg01269.html: "Read-modify-write instructions absolutely need to be treated as read accesses, yet hardware doesn't guarantee to tell us so (they may surface as just write accesses)." This patch addresses the issue in both the VMX and the SVM side.
>
> VMX: Treat all write data access violations also as read violations (in addition to those that were already reported as read violations).
> SVM: Refine the meaning of read data access violations to distinguish between read/write and instruction fetch access violations.
>
> With this patch both VMX and SVM specific nested page fault handling code reports violations the same way, thus abstracting the hardware specific behaviour from the layers above.
>
> Suggested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
>
> ---
> v7: - Add comment blocks describing the rationale behind the specific handling of violations.
>      - Tweak the logic in the VMX code to better match the description in the manual.
> ---
>   xen/arch/x86/hvm/svm/svm.c |  5 ++++-
>   xen/arch/x86/hvm/vmx/vmx.c | 14 +++++++++++++-
>   2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 1f72e19..880e2d5 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1403,8 +1403,11 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>       p2m_access_t p2ma;
>       struct p2m_domain *p2m = NULL;
>   
> +    /* While the hardware doesn't explicitely provide a read access bit
> +     * we deduce it from the instruction fetch bit, thus marking only
> +     * read and/or write accesses as read accesses. */

This doesn't conform to Xen coding style and since this means you will 
need to resend this anyway ;-( I'd suggest something along the lines of

/*
  * Since HW doesn't explicitly provide a read access bit and we need to
  * somehow describe read-modify-write instructions we will conservatively
  * set read_access for all memory accesses that are not instruction 
fetches.
  */

Since I will not be available after today until September and I assume Jan
will want an ack from SVM side for the series here is my

     Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

assuming the text above is updated to Jan's liking.

-boris


>       struct npfec npfec = {
> -        .read_access = 1, /* All NPFs count as reads */
> +        .read_access = !(pfec & PFEC_insn_fetch),
>           .write_access = !!(pfec & PFEC_write_access),
>           .insn_fetch = !!(pfec & PFEC_insn_fetch)
>       };
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 656ce61..2c631ed 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2353,8 +2353,20 @@ static void ept_handle_violation(unsigned long qualification, paddr_t gpa)
>       p2m_type_t p2mt;
>       int ret;
>       struct domain *d = current->domain;
> +
> +    /* We treat all write violations also as read violations.
> +     * The reason why this is required is the following warning:
> +     * "An EPT violation that occurs during as a result of execution of a
> +     * read-modify-write operation sets bit 1 (data write). Whether it also
> +     * sets bit 0 (data read) is implementation-specific and, for a given
> +     * implementation, may differ for different kinds of read-modify-write
> +     * operations."
> +     * - Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> +     *   Volume 3C: System Programming Guide, Part 3
> +     */
>       struct npfec npfec = {
> -        .read_access = !!(qualification & EPT_READ_VIOLATION),
> +        .read_access = !!(qualification & EPT_READ_VIOLATION) ||
> +                       !!(qualification & EPT_WRITE_VIOLATION),
>           .write_access = !!(qualification & EPT_WRITE_VIOLATION),
>           .insn_fetch = !!(qualification & EPT_EXEC_VIOLATION)
>       };