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

[Tamas Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

>      /* 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.

Ack.
 

>          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.

Agree.

 

> --- 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).

I wasn't sure if it/s appropriate to include that fix in this patch and the other one you had on the AMD side for instruction/read accesses. Should I add another patch for that in this series or just include it in this one?

 

> +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

Sounds good to me.

Tamas

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