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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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@xxxxxxxx>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>
> ---
> 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@xxxxxxxxxx>

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)
>       };

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel