Re: [Xen-devel] [PATCH v4 1/3] x86: Consolidate boolean inputs in hvm and p2m into their own respective bitmaps.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.14 at 14:30, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2722,18 +2722,15 @@ void hvm_inject_page_fault(int errcode, unsigned long 
> cr2)
>      hvm_inject_trap(&trap);
>  }
>  
> -int hvm_hap_nested_page_fault(paddr_t gpa,
> -                              bool_t gla_valid,
> -                              unsigned long gla,
> -                              bool_t access_r,
> -                              bool_t access_w,
> -                              bool_t access_x)
> +int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> +                              struct npf_info npfec)
>  {
>      unsigned long gfn = gpa >> PAGE_SHIFT;
>      p2m_type_t p2mt;
>      p2m_access_t p2ma;
>      mfn_t mfn;
>      struct vcpu *v = current;
> +    struct p2m_access_check_info p2mcheck = {0};

Please put this in the smallest scope it is needed in, and then ideally
with a full initializer right away.

> @@ -2793,38 +2793,39 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>  
>      p2m = p2m_get_hostp2m(v->domain);
>      mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
> -                              P2M_ALLOC | (access_w ? P2M_UNSHARE : 0), 
> NULL);
> +                              P2M_ALLOC | npfec.write_access ? P2M_UNSHARE : 
> 0,
> +                              NULL);
>  
>      /* 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 
> */
>          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);

Any strong reason to add the unnecessary parentheses here and
below?

> @@ -1403,10 +1403,12 @@ static void svm_do_nested_pgfault(struct vcpu *v,
>      p2m_access_t p2ma;
>      struct p2m_domain *p2m = NULL;
>  
> -    ret = hvm_hap_nested_page_fault(gpa, 0, ~0ul, 
> -                                    1, /* All NPFs count as reads */
> -                                    npfec & PFEC_write_access, 
> -                                    npfec & PFEC_insn_fetch);
> +    struct npf_info npfec = {0};
> +    npfec.read_access = 1; /* All NPFs count as reads */
> +    npfec.write_access = !!(pfec & PFEC_write_access);
> +    npfec.insn_fetch = !!(pfec & PFEC_insn_fetch);

Please make these a proper initializer, the more that you omit the
required blank line after a declaration anyway.

> +struct npf_info {
> +    uint8_t read_access:1;
> +    uint8_t write_access:1;
> +    uint8_t insn_fetch:1;
> +    uint8_t gla_valid:1;
> +    uint8_t have_extra_fault_info:1;
> +    uint8_t extra_fault_info:1;

The last two fields don't get populated here - please either add the
fields only when actually making use of them, or populate them
properly right away.

And please s/uint8_t/unsigned int/ for better parameter passing
code to be generated.

> +};
> +
> +#define NPFEC_fault_in_gpt      0U
> +#define NPFEC_fault_with_gla    1U

I'm _guessing_ that these apply to the extra_fault_info field? That
ought to be spelled out. But then again, rather than having two
fields (have_ and the actual value), why don't you make this a
tristate (for now), with an enum declaring the possible values.

> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -101,6 +101,20 @@ typedef enum {
>      /* NOTE: Assumed to be only 4 bits right now */
>  } p2m_access_t;
>  
> +/*
> + * Information used to perform mem access checks.
> + */
> +struct p2m_access_check_info {
> +    uint8_t read_access:1;
> +    uint8_t write_access:1;
> +    uint8_t insn_fetch:1;
> +    uint8_t gla_valid:1;
> +    uint8_t have_extra_fault_info:1;
> +    uint8_t extra_fault_info:1;
> +};
> +#define P2M_FAULT_IN_GPT      0u
> +#define P2M_FAULT_WITH_GLA    1u

Similar comments here; the enum of course could be re-used.

Jan

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