Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/03/2014 08:16, Wu, Feng ha scritto:
> --------------------------------------------------------------------------------------------------------------------
> /*
>  * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
>  *
>  * If CPL = 3, SMAP applies to all supervisor-mode data accesses
>  * (these are implicit supervisor accesses) regardless of the value
>  * of EFLAGS.AC.
>  *
>  * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
>  * the result in X86_EFLAGS_AC. We then insert it in place of
>  * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
>  * but it will be one in index if SMAP checks are being overridden.
>  * It is important to keep this branchless.
>  */
> smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
> index =
>         (pfec >> 1) +
>         (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
>
> return (mmu->permissions[index] >> pte_access) & 1;
>
> The direction of PFERR_RSVD_MASK is the opposite compared to your code.
> -------------------------------------------------------------------------------------------------------------------

Correct.

> I am a little confused about some points of the above code:
> 1. "smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);"
> "smap" equals 1 when it is overridden and it is 0 when being enforced.

Actually, smap equals X86_EFLAGS_AC when it is overridden.  Perhaps this 
is the source of the confusion.  Note that I'm using &, not &&.

> So "index"
> will be (pfec >> 1) when SMAP is enforced, but in my understanding of this case, we
> should use the index with PFERR_RSVD_MASK bit being 1 in mmu-> permissions[]
> to check the fault.
> 2. " smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)"
> I am not quite understand this line. BTW, I cannot find the definition of "PFERR_RSVD_BIT",
> Do you mean PFERR_RSVD_BIT equals 3?

Yes.  You can similarly add PFERR_PRESENT_BIT (equal to 0) etc.

> I think the basic idea is using group 0 to check permission faults when !((cpl - 3) & (rflags & X86_EFLAGS_AC)), that is SMAP is overridden
> while using group 1 to check faults when (cpl - 3) & (rflags & X86_EFLAGS_AC), that is SMAP is enforced.
>
> Here is the code base on your proposal in my understanding:
>
> -------------------------------------------------------------------------------------------------------------------
> smap = !((cpl - 3) & (rflags & X86_EFLAGS_AC));
> index =
>         (pfec >> 1) + (smap << (PFERR_RSVD_BIT - 1)); /*assuming PFERR_RSVD_BIT == 3*/
>
> return (mmu->permissions[index] >> pte_access) & 1;
> -------------------------------------------------------------------------------------------------------------------
>
> Could you please have a look at it? Appreciate your help! :)

It is faster if you avoid the "!" and shift right from the AC bit into 
position PFERR_RSVD_BIT - 1.  In update_permission_bitmask you can 
invert the direction of the bit when you extract it from pfec.

Paolo