| Message ID | 20241001122438.1454218-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pv: Fixes to guest_io_okay() | expand |
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
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 {
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(-)