| Message ID | 20250415153246.81688-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: fix implementation of subpage r/o MMIO | expand |
On Tue, Apr 15, 2025 at 05:32:43PM +0200, Roger Pau Monne wrote: > Several handlers have the same necessity of reading or writing from or to > an MMIO region using 1, 2, 4 or 8 bytes accesses. So far this has been > open-coded in the function itself. Instead provide a new set of handlers > that encapsulate the accesses. > > Since the added helpers are not architecture specific, introduce a new > generic io.h header. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Denis Mukhin <dmukhin@ford.com> > --- > Changes since v1: > - Limit {read,write}q() to 64bit architectures. > - Always have a default case in switch statement. > --- > xen/arch/x86/hvm/vmsi.c | 47 ++-------------------------- > xen/arch/x86/mm.c | 32 +++++-------------- > xen/drivers/vpci/msix.c | 47 ++-------------------------- > xen/include/xen/io.h | 68 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 81 insertions(+), 113 deletions(-) > create mode 100644 xen/include/xen/io.h > > diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c > index fd83abb929ec..61b89834d97d 100644 > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -24,6 +24,7 @@ > * Will be merged it with virtual IOAPIC logic, since most is the same > */ > > +#include <xen/io.h> > #include <xen/types.h> > #include <xen/mm.h> > #include <xen/xmalloc.h> > @@ -304,28 +305,7 @@ static void adjacent_read( > > hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address); > > - switch ( len ) > - { > - case 1: > - *pval = readb(hwaddr); > - break; > - > - case 2: > - *pval = readw(hwaddr); > - break; > - > - case 4: > - *pval = readl(hwaddr); > - break; > - > - case 8: > - *pval = readq(hwaddr); > - break; > - > - default: > - ASSERT_UNREACHABLE(); > - break; > - } > + *pval = read_mmio(hwaddr, len); > } > > static void adjacent_write( > @@ -344,28 +324,7 @@ static void adjacent_write( > > hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address); > > - switch ( len ) > - { > - case 1: > - writeb(val, hwaddr); > - break; > - > - case 2: > - writew(val, hwaddr); > - break; > - > - case 4: > - writel(val, hwaddr); > - break; > - > - case 8: > - writeq(val, hwaddr); > - break; > - > - default: > - ASSERT_UNREACHABLE(); > - break; > - } > + write_mmio(hwaddr, val, len); > } > > static int cf_check msixtbl_read( > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 1cf236516789..989e62e7ce6f 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -95,6 +95,7 @@ > #include <xen/guest_access.h> > #include <xen/hypercall.h> > #include <xen/init.h> > +#include <xen/io.h> > #include <xen/iocap.h> > #include <xen/ioreq.h> > #include <xen/irq.h> > @@ -116,7 +117,6 @@ > #include <asm/flushtlb.h> > #include <asm/guest.h> > #include <asm/idt.h> > -#include <asm/io.h> > #include <asm/io_apic.h> > #include <asm/ldt.h> > #include <asm/mem_sharing.h> > @@ -5102,7 +5102,7 @@ static void __iomem *subpage_mmio_map_page( > static void subpage_mmio_write_emulate( > mfn_t mfn, > unsigned int offset, > - const void *data, > + unsigned long data, > unsigned int len) > { > struct subpage_ro_range *entry; > @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate( > > if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) ) > { > - write_ignored: > gprintk(XENLOG_WARNING, > "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n", > mfn_x(mfn), offset, len); > @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate( > return; > } > > - addr += offset; > - switch ( len ) > - { > - case 1: > - writeb(*(const uint8_t*)data, addr); > - break; > - case 2: > - writew(*(const uint16_t*)data, addr); > - break; > - case 4: > - writel(*(const uint32_t*)data, addr); > - break; > - case 8: > - writeq(*(const uint64_t*)data, addr); > - break; > - default: > - /* mmio_ro_emulated_write() already validated the size */ > - ASSERT_UNREACHABLE(); > - goto write_ignored; > - } > + write_mmio(addr + offset, data, len); > } > > #ifdef CONFIG_HVM > @@ -5185,18 +5165,20 @@ int cf_check mmio_ro_emulated_write( > struct x86_emulate_ctxt *ctxt) > { > struct mmio_ro_emulate_ctxt *mmio_ro_ctxt = ctxt->data; > + unsigned long data = 0; > > /* Only allow naturally-aligned stores at the original %cr2 address. */ > if ( ((bytes | offset) & (bytes - 1)) || !bytes || > - offset != mmio_ro_ctxt->cr2 ) > + offset != mmio_ro_ctxt->cr2 || bytes > sizeof(data) ) > { > gdprintk(XENLOG_WARNING, "bad access (cr2=%lx, addr=%lx, bytes=%u)\n", > mmio_ro_ctxt->cr2, offset, bytes); > return X86EMUL_UNHANDLEABLE; > } > > + memcpy(&data, p_data, bytes); > subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset), > - p_data, bytes); > + data, bytes); > > return X86EMUL_OKAY; > } > diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c > index 6bd8c55bb48e..6455f2a03a01 100644 > --- a/xen/drivers/vpci/msix.c > +++ b/xen/drivers/vpci/msix.c > @@ -17,6 +17,7 @@ > * License along with this program; If not, see <http://www.gnu.org/licenses/>. > */ > > +#include <xen/io.h> > #include <xen/sched.h> > #include <xen/vpci.h> > > @@ -344,28 +345,7 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix, > return X86EMUL_OKAY; > } > > - switch ( len ) > - { > - case 1: > - *data = readb(mem + PAGE_OFFSET(addr)); > - break; > - > - case 2: > - *data = readw(mem + PAGE_OFFSET(addr)); > - break; > - > - case 4: > - *data = readl(mem + PAGE_OFFSET(addr)); > - break; > - > - case 8: > - *data = readq(mem + PAGE_OFFSET(addr)); > - break; > - > - default: > - ASSERT_UNREACHABLE(); > - break; > - } > + *data = read_mmio(mem + PAGE_OFFSET(addr), len); > spin_unlock(&vpci->lock); > > return X86EMUL_OKAY; > @@ -493,28 +473,7 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix, > return X86EMUL_OKAY; > } > > - switch ( len ) > - { > - case 1: > - writeb(data, mem + PAGE_OFFSET(addr)); > - break; > - > - case 2: > - writew(data, mem + PAGE_OFFSET(addr)); > - break; > - > - case 4: > - writel(data, mem + PAGE_OFFSET(addr)); > - break; > - > - case 8: > - writeq(data, mem + PAGE_OFFSET(addr)); > - break; > - > - default: > - ASSERT_UNREACHABLE(); > - break; > - } > + write_mmio(mem + PAGE_OFFSET(addr), data, len); > spin_unlock(&vpci->lock); > > return X86EMUL_OKAY; > diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h > new file mode 100644 > index 000000000000..4495a04c403e > --- /dev/null > +++ b/xen/include/xen/io.h > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * IO related routines. > + * > + * Copyright (c) 2025 Cloud Software Group > + */ > +#ifndef XEN_IO_H > +#define XEN_IO_H > + > +#include <xen/bug.h> > + > +#include <asm/io.h> > + > +static inline unsigned long read_mmio(const volatile void __iomem *mem, > + unsigned int size) > +{ > + switch ( size ) > + { > + case 1: > + return readb(mem); > + > + case 2: > + return readw(mem); > + > + case 4: > + return readl(mem); > + > +#ifdef CONFIG_64BIT > + case 8: > + return readq(mem); > +#endif > + > + default: > + ASSERT_UNREACHABLE(); > + return ~0UL; > + } > +} > + > +static inline void write_mmio(volatile void __iomem *mem, unsigned long data, > + unsigned int size) > +{ > + switch ( size ) > + { > + case 1: > + writeb(data, mem); > + break; > + > + case 2: > + writew(data, mem); > + break; > + > + case 4: > + writel(data, mem); > + break; > + > +#ifdef CONFIG_64BIT > + case 8: > + writeq(data, mem); > + break; > +#endif > + > + default: > + ASSERT_UNREACHABLE(); > + break; > + } > +} > + > +#endif /* XEN_IO_H */ > -- > 2.48.1 > >
On 15/04/2025 4:32 pm, Roger Pau Monne wrote: > diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h > new file mode 100644 > index 000000000000..4495a04c403e > --- /dev/null > +++ b/xen/include/xen/io.h > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * IO related routines. > + * > + * Copyright (c) 2025 Cloud Software Group > + */ > +#ifndef XEN_IO_H > +#define XEN_IO_H > + > +#include <xen/bug.h> > + > +#include <asm/io.h> > + > +static inline unsigned long read_mmio(const volatile void __iomem *mem, void *__iomem. (i.e. without the __iomem in the middle of the type). It seems most examples of this in Xen are wrong. ~Andrew
On 16.04.2025 10:29, Andrew Cooper wrote: > On 15/04/2025 4:32 pm, Roger Pau Monne wrote: >> diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h >> new file mode 100644 >> index 000000000000..4495a04c403e >> --- /dev/null >> +++ b/xen/include/xen/io.h >> @@ -0,0 +1,68 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * IO related routines. >> + * >> + * Copyright (c) 2025 Cloud Software Group >> + */ >> +#ifndef XEN_IO_H >> +#define XEN_IO_H >> + >> +#include <xen/bug.h> >> + >> +#include <asm/io.h> >> + >> +static inline unsigned long read_mmio(const volatile void __iomem *mem, > > void *__iomem. (i.e. without the __iomem in the middle of the type). > It seems most examples of this in Xen are wrong. Hmm, perhaps in turn because I don't see why it would need to be that placement. The attribute describes what's pointed to, not the variable (or parameter here) itself. Jan
On 15.04.2025 17:32, Roger Pau Monne wrote: > @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate( > > if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) ) > { > - write_ignored: > gprintk(XENLOG_WARNING, > "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n", > mfn_x(mfn), offset, len); > @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate( > return; > } > > - addr += offset; > - switch ( len ) > - { > - case 1: > - writeb(*(const uint8_t*)data, addr); > - break; > - case 2: > - writew(*(const uint16_t*)data, addr); > - break; > - case 4: > - writel(*(const uint32_t*)data, addr); > - break; > - case 8: > - writeq(*(const uint64_t*)data, addr); > - break; > - default: > - /* mmio_ro_emulated_write() already validated the size */ > - ASSERT_UNREACHABLE(); > - goto write_ignored; > - } > + write_mmio(addr + offset, data, len); > } Should probably have noticed this on v1 already: The log message is now lost for the write-ignored case. It looks easy enough to have the function return a boolean indicating "done", to retain original behavior here. Jan
On Thu, Apr 17, 2025 at 09:43:09AM +0200, Jan Beulich wrote: > On 15.04.2025 17:32, Roger Pau Monne wrote: > > @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate( > > > > if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) ) > > { > > - write_ignored: > > gprintk(XENLOG_WARNING, > > "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n", > > mfn_x(mfn), offset, len); > > @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate( > > return; > > } > > > > - addr += offset; > > - switch ( len ) > > - { > > - case 1: > > - writeb(*(const uint8_t*)data, addr); > > - break; > > - case 2: > > - writew(*(const uint16_t*)data, addr); > > - break; > > - case 4: > > - writel(*(const uint32_t*)data, addr); > > - break; > > - case 8: > > - writeq(*(const uint64_t*)data, addr); > > - break; > > - default: > > - /* mmio_ro_emulated_write() already validated the size */ > > - ASSERT_UNREACHABLE(); > > - goto write_ignored; > > - } > > + write_mmio(addr + offset, data, len); > > } > > Should probably have noticed this on v1 already: The log message is now lost > for the write-ignored case. It looks easy enough to have the function return > a boolean indicating "done", to retain original behavior here. Hm, I didn't seem to me the message wants conserving, as it's unreachable code. I can try to add again, but we don't print such message in other cases. Thanks, Roger.
On 17.04.2025 16:16, Roger Pau Monné wrote: > On Thu, Apr 17, 2025 at 09:43:09AM +0200, Jan Beulich wrote: >> On 15.04.2025 17:32, Roger Pau Monne wrote: >>> @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate( >>> >>> if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) ) >>> { >>> - write_ignored: >>> gprintk(XENLOG_WARNING, >>> "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n", >>> mfn_x(mfn), offset, len); >>> @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate( >>> return; >>> } >>> >>> - addr += offset; >>> - switch ( len ) >>> - { >>> - case 1: >>> - writeb(*(const uint8_t*)data, addr); >>> - break; >>> - case 2: >>> - writew(*(const uint16_t*)data, addr); >>> - break; >>> - case 4: >>> - writel(*(const uint32_t*)data, addr); >>> - break; >>> - case 8: >>> - writeq(*(const uint64_t*)data, addr); >>> - break; >>> - default: >>> - /* mmio_ro_emulated_write() already validated the size */ >>> - ASSERT_UNREACHABLE(); >>> - goto write_ignored; >>> - } >>> + write_mmio(addr + offset, data, len); >>> } >> >> Should probably have noticed this on v1 already: The log message is now lost >> for the write-ignored case. It looks easy enough to have the function return >> a boolean indicating "done", to retain original behavior here. > > Hm, I didn't seem to me the message wants conserving, as it's > unreachable code. I can try to add again, but we don't print such > message in other cases. This sub-page stuff is special, but I wouldn't mind if we dropped the message altogether. The "unreachable code" argument is slightly weak, as it's in particular when the assumption is violated that we would want to know about it. Jan
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c index fd83abb929ec..61b89834d97d 100644 --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -24,6 +24,7 @@ * Will be merged it with virtual IOAPIC logic, since most is the same */ +#include <xen/io.h> #include <xen/types.h> #include <xen/mm.h> #include <xen/xmalloc.h> @@ -304,28 +305,7 @@ static void adjacent_read( hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address); - switch ( len ) - { - case 1: - *pval = readb(hwaddr); - break; - - case 2: - *pval = readw(hwaddr); - break; - - case 4: - *pval = readl(hwaddr); - break; - - case 8: - *pval = readq(hwaddr); - break; - - default: - ASSERT_UNREACHABLE(); - break; - } + *pval = read_mmio(hwaddr, len); } static void adjacent_write( @@ -344,28 +324,7 @@ static void adjacent_write( hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address); - switch ( len ) - { - case 1: - writeb(val, hwaddr); - break; - - case 2: - writew(val, hwaddr); - break; - - case 4: - writel(val, hwaddr); - break; - - case 8: - writeq(val, hwaddr); - break; - - default: - ASSERT_UNREACHABLE(); - break; - } + write_mmio(hwaddr, val, len); } static int cf_check msixtbl_read( diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 1cf236516789..989e62e7ce6f 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -95,6 +95,7 @@ #include <xen/guest_access.h> #include <xen/hypercall.h> #include <xen/init.h> +#include <xen/io.h> #include <xen/iocap.h> #include <xen/ioreq.h> #include <xen/irq.h> @@ -116,7 +117,6 @@ #include <asm/flushtlb.h> #include <asm/guest.h> #include <asm/idt.h> -#include <asm/io.h> #include <asm/io_apic.h> #include <asm/ldt.h> #include <asm/mem_sharing.h> @@ -5102,7 +5102,7 @@ static void __iomem *subpage_mmio_map_page( static void subpage_mmio_write_emulate( mfn_t mfn, unsigned int offset, - const void *data, + unsigned long data, unsigned int len) { struct subpage_ro_range *entry; @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate( if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) ) { - write_ignored: gprintk(XENLOG_WARNING, "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n", mfn_x(mfn), offset, len); @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate( return; } - addr += offset; - switch ( len ) - { - case 1: - writeb(*(const uint8_t*)data, addr); - break; - case 2: - writew(*(const uint16_t*)data, addr); - break; - case 4: - writel(*(const uint32_t*)data, addr); - break; - case 8: - writeq(*(const uint64_t*)data, addr); - break; - default: - /* mmio_ro_emulated_write() already validated the size */ - ASSERT_UNREACHABLE(); - goto write_ignored; - } + write_mmio(addr + offset, data, len); } #ifdef CONFIG_HVM @@ -5185,18 +5165,20 @@ int cf_check mmio_ro_emulated_write( struct x86_emulate_ctxt *ctxt) { struct mmio_ro_emulate_ctxt *mmio_ro_ctxt = ctxt->data; + unsigned long data = 0; /* Only allow naturally-aligned stores at the original %cr2 address. */ if ( ((bytes | offset) & (bytes - 1)) || !bytes || - offset != mmio_ro_ctxt->cr2 ) + offset != mmio_ro_ctxt->cr2 || bytes > sizeof(data) ) { gdprintk(XENLOG_WARNING, "bad access (cr2=%lx, addr=%lx, bytes=%u)\n", mmio_ro_ctxt->cr2, offset, bytes); return X86EMUL_UNHANDLEABLE; } + memcpy(&data, p_data, bytes); subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset), - p_data, bytes); + data, bytes); return X86EMUL_OKAY; } diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c index 6bd8c55bb48e..6455f2a03a01 100644 --- a/xen/drivers/vpci/msix.c +++ b/xen/drivers/vpci/msix.c @@ -17,6 +17,7 @@ * License along with this program; If not, see <http://www.gnu.org/licenses/>. */ +#include <xen/io.h> #include <xen/sched.h> #include <xen/vpci.h> @@ -344,28 +345,7 @@ static int adjacent_read(const struct domain *d, const struct vpci_msix *msix, return X86EMUL_OKAY; } - switch ( len ) - { - case 1: - *data = readb(mem + PAGE_OFFSET(addr)); - break; - - case 2: - *data = readw(mem + PAGE_OFFSET(addr)); - break; - - case 4: - *data = readl(mem + PAGE_OFFSET(addr)); - break; - - case 8: - *data = readq(mem + PAGE_OFFSET(addr)); - break; - - default: - ASSERT_UNREACHABLE(); - break; - } + *data = read_mmio(mem + PAGE_OFFSET(addr), len); spin_unlock(&vpci->lock); return X86EMUL_OKAY; @@ -493,28 +473,7 @@ static int adjacent_write(const struct domain *d, const struct vpci_msix *msix, return X86EMUL_OKAY; } - switch ( len ) - { - case 1: - writeb(data, mem + PAGE_OFFSET(addr)); - break; - - case 2: - writew(data, mem + PAGE_OFFSET(addr)); - break; - - case 4: - writel(data, mem + PAGE_OFFSET(addr)); - break; - - case 8: - writeq(data, mem + PAGE_OFFSET(addr)); - break; - - default: - ASSERT_UNREACHABLE(); - break; - } + write_mmio(mem + PAGE_OFFSET(addr), data, len); spin_unlock(&vpci->lock); return X86EMUL_OKAY; diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h new file mode 100644 index 000000000000..4495a04c403e --- /dev/null +++ b/xen/include/xen/io.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * IO related routines. + * + * Copyright (c) 2025 Cloud Software Group + */ +#ifndef XEN_IO_H +#define XEN_IO_H + +#include <xen/bug.h> + +#include <asm/io.h> + +static inline unsigned long read_mmio(const volatile void __iomem *mem, + unsigned int size) +{ + switch ( size ) + { + case 1: + return readb(mem); + + case 2: + return readw(mem); + + case 4: + return readl(mem); + +#ifdef CONFIG_64BIT + case 8: + return readq(mem); +#endif + + default: + ASSERT_UNREACHABLE(); + return ~0UL; + } +} + +static inline void write_mmio(volatile void __iomem *mem, unsigned long data, + unsigned int size) +{ + switch ( size ) + { + case 1: + writeb(data, mem); + break; + + case 2: + writew(data, mem); + break; + + case 4: + writel(data, mem); + break; + +#ifdef CONFIG_64BIT + case 8: + writeq(data, mem); + break; +#endif + + default: + ASSERT_UNREACHABLE(); + break; + } +} + +#endif /* XEN_IO_H */
Several handlers have the same necessity of reading or writing from or to an MMIO region using 1, 2, 4 or 8 bytes accesses. So far this has been open-coded in the function itself. Instead provide a new set of handlers that encapsulate the accesses. Since the added helpers are not architecture specific, introduce a new generic io.h header. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Limit {read,write}q() to 64bit architectures. - Always have a default case in switch statement. --- xen/arch/x86/hvm/vmsi.c | 47 ++-------------------------- xen/arch/x86/mm.c | 32 +++++-------------- xen/drivers/vpci/msix.c | 47 ++-------------------------- xen/include/xen/io.h | 68 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 113 deletions(-) create mode 100644 xen/include/xen/io.h