This removes most of the opencoded bit logic on the exit qualification.
Unfortunately, size is 1-based not 0-based, so need adjusting in a separate
variable.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 00b531f76cbf..a96c601efded 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4560,23 +4560,37 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         break;
 
     case EXIT_REASON_IO_INSTRUCTION:
-        __vmread(EXIT_QUALIFICATION, &exit_qualification);
-        if ( exit_qualification & 0x10 )
+    {
+        union {
+            unsigned long raw;
+            struct {
+                uint32_t size:3;
+                bool     in:1;
+                bool     str:1;
+                bool     rep:1;
+                bool     imm:1;
+                uint32_t :9;
+                uint16_t port;
+            };
+        } io_qual;
+        unsigned int bytes;
+
+        __vmread(EXIT_QUALIFICATION, &io_qual.raw);
+        bytes = io_qual.size + 1;
+
+        if ( io_qual.str )
         {
-            /* INS, OUTS */
             if ( !hvm_emulate_one_insn(x86_insn_is_portio, "port I/O") )
                 hvm_inject_hw_exception(TRAP_gp_fault, 0);
         }
         else
         {
-            /* IN, OUT */
-            uint16_t port = (exit_qualification >> 16) & 0xFFFF;
-            int bytes = (exit_qualification & 0x07) + 1;
-            int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
-            if ( handle_pio(port, bytes, dir) )
+            if ( handle_pio(io_qual.port, bytes,
+                            io_qual.in ? IOREQ_READ : IOREQ_WRITE) )
                 update_guest_eip(); /* Safe: IN, OUT */
         }
         break;
+    }
 
     case EXIT_REASON_INVD:
     case EXIT_REASON_WBINVD:
-- 
2.30.2

On 20.03.2023 19:20, Andrew Cooper wrote:
> This removes most of the opencoded bit logic on the exit qualification.
> Unfortunately, size is 1-based not 0-based, so need adjusting in a separate
> variable.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but ...

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4560,23 +4560,37 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>          break;
>  
>      case EXIT_REASON_IO_INSTRUCTION:
> -        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> -        if ( exit_qualification & 0x10 )
> +    {
> +        union {
> +            unsigned long raw;
> +            struct {
> +                uint32_t size:3;
> +                bool     in:1;
> +                bool     str:1;
> +                bool     rep:1;
> +                bool     imm:1;
> +                uint32_t :9;
> +                uint16_t port;

... I'm not sure this is sufficiently portable: Whether a bitfield of type
uint32_t followed by a non-bitfield is padded to fill the rest of the
containing 32-bit field is left unspecified by C99; this particular aspect
isn't even "implementation defined" (afaics). Therefore I think it would
be better if either uint32_t was replaced by uint16_t, or if port also was
made a bit field (and then perhaps also of type uint32_t, or unsigned int).

Jan

On 21/03/2023 7:59 am, Jan Beulich wrote:
> On 20.03.2023 19:20, Andrew Cooper wrote:
>> This removes most of the opencoded bit logic on the exit qualification.
>> Unfortunately, size is 1-based not 0-based, so need adjusting in a separate
>> variable.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but ...
>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -4560,23 +4560,37 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>          break;
>>  
>>      case EXIT_REASON_IO_INSTRUCTION:
>> -        __vmread(EXIT_QUALIFICATION, &exit_qualification);
>> -        if ( exit_qualification & 0x10 )
>> +    {
>> +        union {
>> +            unsigned long raw;
>> +            struct {
>> +                uint32_t size:3;
>> +                bool     in:1;
>> +                bool     str:1;
>> +                bool     rep:1;
>> +                bool     imm:1;
>> +                uint32_t :9;
>> +                uint16_t port;
> ... I'm not sure this is sufficiently portable: Whether a bitfield of type
> uint32_t followed by a non-bitfield is padded to fill the rest of the
> containing 32-bit field is left unspecified by C99; this particular aspect
> isn't even "implementation defined" (afaics). Therefore I think it would
> be better if either uint32_t was replaced by uint16_t, or if port also was
> made a bit field (and then perhaps also of type uint32_t, or unsigned int).

Port is a naturally aligned uint16_t field so I'd prefer to keep it as is.

I'll drop the others to uint16_t.  Furthermore, as it's simple cleanup
specifically for another pending patch, I'll commit this now and follow
up on the other thread.

~Andrew