Ever since it's introduction in commit 013351bd7ab3 ("Define new event-channel
and physdev hypercalls"), the public interface was named nr_ports while the
internal field was called iobmp_limit.

Rename the intenral field to iobmp_nr to match the public interface, and
clarify that, when nonzero, Xen will read 2 bytes.

There isn't a perfect parallel with a real TSS, but iobmp_nr being 0 is the
paravirt "no IOPB" case, and it is important that no read occurs in this case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/domain.h | 2 +-
 xen/arch/x86/physdev.c            | 2 +-
 xen/arch/x86/pv/emul-priv-op.c    | 7 ++++++-
 xen/include/public/physdev.h      | 3 +++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 811251852f79..bdcdb8de09f1 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -573,7 +573,7 @@ struct pv_vcpu
 
     /* I/O-port access bitmap. */
     XEN_GUEST_HANDLE(uint8) iobmp; /* Guest kernel vaddr of the bitmap. */
-    unsigned int iobmp_limit; /* Number of ports represented in the bitmap. */
+    unsigned int iobmp_nr;    /* Number of ports represented in the bitmap. */
 #define IOPL(val) MASK_INSR(val, X86_EFLAGS_IOPL)
     unsigned int iopl;        /* Current IOPL for this VCPU, shifted left by
                                * 12 to match the eflags register. */
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index d6dd622952a9..69fd42667c69 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -436,7 +436,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 #else
         guest_from_compat_handle(curr->arch.pv.iobmp, set_iobitmap.bitmap);
 #endif
-        curr->arch.pv.iobmp_limit = set_iobitmap.nr_ports;
+        curr->arch.pv.iobmp_nr = set_iobitmap.nr_ports;
         break;
     }
 
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index e35285d4ab69..cefa38d56138 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -167,7 +167,12 @@ static int guest_io_okay(unsigned int port, unsigned int bytes,
     if ( iopl_ok(v, regs) )
         return X86EMUL_OKAY;
 
-    if ( (port + bytes) <= v->arch.pv.iobmp_limit )
+    /*
+     * When @iobmp_nr is non-zero, Xen, like real CPUs and the TSS IOPB,
+     * always reads 2 bytes from @iobmp, which might be one byte beyond
+     * @nr_ports.
+     */
+    if ( (port + bytes) <= v->arch.pv.iobmp_nr )
     {
         const void *__user addr = v->arch.pv.iobmp.p + (port >> 3);
         uint16_t mask;
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index 45e1c18541c8..3149049a9a57 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -87,6 +87,9 @@ DEFINE_XEN_GUEST_HANDLE(physdev_set_iopl_t);
 /*
  * Set the current VCPU's I/O-port permissions bitmap.
  * @arg == pointer to physdev_set_iobitmap structure.
+ *
+ * When @nr_ports is non-zero, Xen, like real CPUs and the TSS IOPB, always
+ * reads 2 bytes from @bitmap, which might be one byte beyond @nr_ports.
  */
 #define PHYSDEVOP_set_iobitmap           7
 struct physdev_set_iobitmap {
-- 
2.39.5

On 01.10.2024 14:24, Andrew Cooper wrote:
> Ever since it's introduction in commit 013351bd7ab3 ("Define new event-channel
> and physdev hypercalls"), the public interface was named nr_ports while the
> internal field was called iobmp_limit.
> 
> Rename the intenral field to iobmp_nr to match the public interface, and
> clarify that, when nonzero, Xen will read 2 bytes.
> 
> There isn't a perfect parallel with a real TSS, but iobmp_nr being 0 is the
> paravirt "no IOPB" case, and it is important that no read occurs in this case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one cosmetic request:

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -167,7 +167,12 @@ static int guest_io_okay(unsigned int port, unsigned int bytes,
>      if ( iopl_ok(v, regs) )
>          return X86EMUL_OKAY;
>  
> -    if ( (port + bytes) <= v->arch.pv.iobmp_limit )
> +    /*
> +     * When @iobmp_nr is non-zero, Xen, like real CPUs and the TSS IOPB,
> +     * always reads 2 bytes from @iobmp, which might be one byte beyond
> +     * @nr_ports.
> +     */
> +    if ( (port + bytes) <= v->arch.pv.iobmp_nr )

If you use @nr_ports in the comment here, then I think some connection wants
establishing as to where this comes from. Otherwise use @iobmp_nr a 2nd time.

Jan

On 01/10/2024 2:35 pm, Jan Beulich wrote:
> On 01.10.2024 14:24, Andrew Cooper wrote:
>> Ever since it's introduction in commit 013351bd7ab3 ("Define new event-channel
>> and physdev hypercalls"), the public interface was named nr_ports while the
>> internal field was called iobmp_limit.
>>
>> Rename the intenral field to iobmp_nr to match the public interface, and
>> clarify that, when nonzero, Xen will read 2 bytes.
>>
>> There isn't a perfect parallel with a real TSS, but iobmp_nr being 0 is the
>> paravirt "no IOPB" case, and it is important that no read occurs in this case.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> with one cosmetic request:
>
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -167,7 +167,12 @@ static int guest_io_okay(unsigned int port, unsigned int bytes,
>>      if ( iopl_ok(v, regs) )
>>          return X86EMUL_OKAY;
>>  
>> -    if ( (port + bytes) <= v->arch.pv.iobmp_limit )
>> +    /*
>> +     * When @iobmp_nr is non-zero, Xen, like real CPUs and the TSS IOPB,
>> +     * always reads 2 bytes from @iobmp, which might be one byte beyond
>> +     * @nr_ports.
>> +     */
>> +    if ( (port + bytes) <= v->arch.pv.iobmp_nr )
> If you use @nr_ports in the comment here, then I think some connection wants
> establishing as to where this comes from. Otherwise use @iobmp_nr a 2nd time.

Oops, that was a copy&paste error from the header.  I'll switch it back
to @iobmp_nr.

~Andrew