Re: [Xen-devel] [PATCH for-next v3 05/22] x86/pv: clean up emulate.c

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.05.17 at 19:09, <wei.liu2@xxxxxxxxxx> wrote:
> Fix coding style issues. Replace bool_t with bool. Add spaces around
> binary ops.

For these it would probably be fine to do them while moving the code.
But you did the extra work to put this into a separate patch, so I'm
not going to object to this approach.

> @@ -209,43 +209,45 @@ static int guest_io_okay(
>              /* fallthrough */
>          case 0:  break;
>          }
> -        TOGGLE_MODE();
>  
> -        if ( (x.mask & (((1<<bytes)-1) << (port&7))) == 0 )
> -            return 1;
> +        if ( user_mode )
> +            toggle_guest_mode(v);
> +
> +        if ( (x.mask & (((1 << bytes)-1) << (port & 7))) == 0 )

You've caught the << and & here, but missed the - .

> @@ -369,7 +372,7 @@ static unsigned int check_guest_io_breakpoint(struct vcpu 
> *v,
>          }
>  
>          if ( (start < (port + len)) && ((start + width) > port) )
> -            match |= 1 << i;
> +            match |= 1u << i;

Ah, I guess that's what "Use unsigned integer for shifting" refers to.
The wording first made me assume you talk about the shift count...

> @@ -921,11 +925,11 @@ static int priv_op_read_msr(unsigned int reg, uint64_t 
> *val,
>          *val = curr->arch.pv_vcpu.gs_base_user;
>          return X86EMUL_OKAY;
>  
> -    /*
> -     * In order to fully retain original behavior, defer calling
> -     * pv_soft_rdtsc() until after emulation. This may want/need to be
> -     * reconsidered.
> -     */
> +        /*
> +         * In order to fully retain original behavior, defer calling
> +         * pv_soft_rdtsc() until after emulation. This may want/need to be
> +         * reconsidered.
> +         */
>      case MSR_IA32_TSC:
>          poc->tsc |= TSC_BASE;
>          goto normal;

This comment was intentionally indented that way - the deeper
indentation would imo only be suitable if it followed the case label.

> @@ -1745,17 +1748,17 @@ void emulate_gate_op(struct cpu_user_regs *regs)
>      {
>          unsigned int ss, esp, *stkp;
>          int rc;
> -#define push(item) do \
> -        { \
> -            --stkp; \
> -            esp -= 4; \
> -            rc = __put_user(item, stkp); \
> -            if ( rc ) \
> -            { \
> -                pv_inject_page_fault(PFEC_write_access, \
> -                                     (unsigned long)(stkp + 1) - rc); \
> -                return; \
> -            } \
> +#define push(item) do                                                   \
> +        {                                                               \
> +            --stkp;                                                     \
> +            esp -= 4;                                                   \
> +            rc = __put_user(item, stkp);                                \
> +            if ( rc )                                                   \
> +            {                                                           \
> +                pv_inject_page_fault(PFEC_write_access,                 \
> +                                     (unsigned long)(stkp + 1) - rc);   \
> +                return;                                                 \
> +            }                                                           \
>          } while ( 0 )

I know it's a matter of taste, and I could imagine others having
differing opinions, but I dislike this moving out to the far right of
the backslashes. In particular it makes later adding a line longer
than all current ones ugly - either one has to touch all other lines
or one needs to accept the one new line standing out.

Jan


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