[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 1/3] x86: Consolidate boolean inputs in hvm and p2m into a shared bitmap.



>>> On 11.08.14 at 13:42, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> This patch consolidates the boolean input parameters of 
> hvm_hap_nested_page_fault and p2m_mem_access_check into a common bitmap and 
> defines the bitmap members accordingly.
> 
> v5: Shared structure in mm.h, style fixes and moving gla fault type 
> additions into next patch in the series.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
if a couple of items would get taken care of (the couple of stylistic
issues aren't strictly needed, but at least the structure naming -
see below - should get changed imo):

>      /* Check access permissions first, then handle faults */
>      if ( mfn_x(mfn) != INVALID_MFN )
>      {
> -        int violation = 0;
> +        bool_t violation = 0;
>          /* If the access is against the permissions, then send to mem_event 
> */

Adjustments like this should also be accompanied by inserting the
so far missing blank line after the declaration.

>          switch (p2ma) 
>          {
>          case p2m_access_n:
>          case p2m_access_n2rwx:
>          default:
> -            violation = access_r || access_w || access_x;
> +            violation = npfec.read_access || npfec.write_access || 
> npfec.insn_fetch;
>              break;
>          case p2m_access_r:
> -            violation = access_w || access_x;
> +            violation = npfec.write_access || npfec.insn_fetch;
>              break;
>          case p2m_access_w:
> -            violation = access_r || access_x;
> +            violation = npfec.read_access || npfec.insn_fetch;
>              break;
>          case p2m_access_x:
> -            violation = access_r || access_w;
> +            violation = npfec.read_access || npfec.write_access;
>              break;
>          case p2m_access_rx:
>          case p2m_access_rx2rw:
> -            violation = access_w;
> +            violation = npfec.write_access;
>              break;
>          case p2m_access_wx:
> -            violation = access_r;
> +            violation = npfec.read_access;
>              break;
>          case p2m_access_rw:
> -            violation = access_x;
> +            violation = npfec.insn_fetch;
>              break;
>          case p2m_access_rwx:
>              break;

Considering that all but this path set "violation", the initializer above
is rather pointless (value will be overwritten often anyway), and
hence would better be converted to setting of the value here.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2353,6 +2353,11 @@ static void ept_handle_violation(unsigned long 
> qualification, paddr_t gpa)
>      p2m_type_t p2mt;
>      int ret;
>      struct domain *d = current->domain;
> +    struct mem_access_check npfec = {
> +        .read_access = !!(qualification & EPT_READ_VIOLATION),

While I can see that you want this patch to not have any functional
change, I think correcting the mistake here (like I also did in the
patch that I sent you) would be rather desirable: 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).

> +int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> +                              struct mem_access_check npfec);

The name of the structure isn't really ideal in this context, the more
that mem-access really is a secondary feature, i.e. the main purpose
of the structure is to convey fault information. Why don't you just
call it "struct npfec"?

Jan


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.