Re: [Xen-devel] [XEN PATCH v14 8/8] Add xentrace to vmware_port

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2020 18:52, Don Slutz wrote:
> From: Don Slutz <dslutz@xxxxxxxxxxx>
> 
> Also added missing TRAP_DEBUG & VLAPIC.
> 
> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> CC: Don Slutz <don.slutz@xxxxxxxxx>
> ---
> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> v14:
>   Reworked to current code.
>   Added VMPORT_SEND because I wanted to see it during testing.
> 
> v13:
>     Please do this by extending the existing infrastructure rather
>     than special-casing 7 on the side.  (i.e. extend ND to take 7
>     parameters, and introduce HVMTRACE_7D)
>     = { d1, d2, d3, d4, d5, d6, d7 } will be far shorter, linewise.

I think this would have wanted to split into two patches right
at the time: One for the extension, and another for the new
VMware logic. But see below.

> @@ -62,6 +63,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t 
> bytes, uint32_t *val)
>      if ( port == BDOOR_PORT && regs->eax == BDOOR_MAGIC )
>      {
>          uint32_t new_eax = ~0u;
> +        uint16_t cmd = regs->ecx;
>          uint64_t value;
>          struct vcpu *curr = current;
>          struct domain *currd = curr->domain;
> @@ -72,7 +74,7 @@ static int vmport_ioport(int dir, uint32_t port, uint32_t 
> bytes, uint32_t *val)
>           * leaving the high 32-bits unchanged, unlike what one would
>           * expect to happen.
>           */
> -        switch ( regs->ecx & 0xffff )
> +        switch ( cmd )
>          {
>          case BDOOR_CMD_GETMHZ:
>              new_eax = currd->arch.tsc_khz / 1000;
> @@ -147,14 +149,22 @@ static int vmport_ioport(int dir, uint32_t port, 
> uint32_t bytes, uint32_t *val)
>              break;
>  
>          default:
> +            HVMTRACE_6D(VMPORT_SEND, cmd, regs->ebx, regs->ecx,
> +                        regs->edx, regs->esi, regs->edi);

With cmd derived from regs->ecx, why pass the same value twice here?

>              /* Let backing DM handle */
>              return X86EMUL_UNHANDLEABLE;
>          }
> +        HVMTRACE_7D(VMPORT_HANDLED, cmd, new_eax, regs->ebx, regs->ecx,
> +                    regs->edx, regs->esi, regs->edi);

None of the cases making it here consumes or alter regs->edi. Why
record / report its value? Without this, the entire widening to 7
parameters becomes unnecessary for now, afaics.

Jan