Xen project Mailing List

Re: [Xen-devel] [PATCH v6 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space

On Tue, Sep 19, 2017 at 04:29:27PM +0100, Roger Pau Monne wrote: > This functionality is going to reside in vpci.c (and the corresponding > vpci.h header), and should be arch-agnostic. The handlers introduced > in this patch setup the basic functionality required in order to trap > accesses to the PCI config space, and allow decoding the address and > finding the corresponding handler that should handle the access > (although no handlers are implemented). > > Note that the traps to the PCI IO ports registers (0xcf8/0xcfc) are > setup inside of a x86 HVM file, since that's not shared with other > arches. > > A new XEN_X86_EMU_VPCI x86 domain flag is added in order to signal Xen > whether a domain should use the newly introduced vPCI handlers, this > is only enabled for PVH Dom0 at the moment. > > A very simple user-space test is also provided, so that the basic > functionality of the vPCI traps can be asserted. This has been proven > quite helpful during development, since the logic to handle partial > accesses or accesses that expand across multiple registers is not > trivial. > > The handlers for the registers are added to a linked list that's keep > sorted at all times. Both the read and write handlers support accesses > that expand across multiple emulated registers and contain gaps not > emulated. > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> I am afraid I don't know much about PCI so I can't do meaningful review of this patch. The change to libxl looks good to me so for that bit: Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx> > --- > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx> > Cc: Jan Beulich <jbeulich@xxxxxxxx> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx> > --- > Changes since v5: > - Use a spinlock per pci device. > - Use the recently introduced pci_sbdf_t type. > - Fix test harness to use the right handler type and the newly > introduced lock. > - Move the position of the vpci sections in the linker scripts. > - Constify domain and pci_dev in vpci_{read/write}. > - Fix typos in comments. > - Use _XEN_VPCI_H_ as header guard. > > Changes since v4: > * User-space test harness: > - Do not redirect the output of the test. > - Add main.c and emul.h as dependencies of the Makefile target. > - Use the same rule to modify the vpci and list headers. > - Remove underscores from local macro variables. > - Add _check suffix to the test harness multiread function. > - Change the value written by every different size in the multiwrite > test. > - Use { } to initialize the r16 and r20 arrays (instead of { 0 }). > - Perform some of the read checks with the local variable directly. > - Expand some comments. > - Implement a dummy rwlock. > * Hypervisor code: > - Guard the linker script changes with CONFIG_HAS_PCI. > - Rename vpci_access_check to vpci_access_allowed and make it return > bool. > - Make hvm_pci_decode_addr return the register as return value. > - Use ~3 instead of 0xfffc to remove the register offset when > checking accesses to IO ports. > - s/head/prev in vpci_add_register. > - Add parentheses around & in vpci_add_register. > - Fix register removal. > - Change the BUGs in vpci_{read/write}_hw helpers to > ASSERT_UNREACHABLE. > - Make merge_result static and change the computation of the mask to > avoid using a uint64_t. > - Modify vpci_read to only read from hardware the not-emulated gaps. > - Remove the vpci_val union and use a uint32_t instead. > - Change handler read type to return a uint32_t instead of modifying > a variable passed by reference. > - Constify the data opaque parameter of read handlers. > - Change the size parameter of the vpci_{read/write} functions to > unsigned int. > - Place the array of initialization handlers in init.rodata or > .rodata depending on whether late-hwdom is enabled. > - Remove the pci_devs lock, assume the Dom0 is well behaved and won't > remove the device while trying to access it. > - Change the recursive spinlock into a rw lock for performance > reasons. > > Changes since v3: > * User-space test harness: > - Fix spaces in container_of macro. > - Implement a dummy locking functions. > - Remove 'current' macro make current a pointer to the statically > allocated vpcu. > - Remove unneeded parentheses in the pci_conf_readX macros. > - Fix the name of the write test macro. > - Remove the dummy EXPORT_SYMBOL macro (this was needed by the RB > code only). > - Import the max macro. > - Test all possible read/write size combinations with all possible > emulated register sizes. > - Introduce a test for register removal. > * Hypervisor code: > - Use a sorted list in order to store the config space handlers. > - Remove some unneeded 'else' branches. > - Make the IO port handlers always return X86EMUL_OKAY, and set the > data to all 1's in case of read failure (write are simply ignored). > - In hvm_select_ioreq_server reuse local variables when calling > XEN_DMOP_PCI_SBDF. > - Store the pointers to the initialization functions in the .rodata > section. > - Do not ignore the return value of xen_vpci_add_handlers in > setup_one_hwdom_device. > - Remove the vpci_init macro. > - Do not hide the pointers inside of the vpci_{read/write}_t > typedefs. > - Rename priv_data to private in vpci_register. > - Simplify checking for register overlap in vpci_register_cmp. > - Check that the offset and the length match before removing a > register in xen_vpci_remove_register. > - Make vpci_read_hw return a value rather than storing it in a > pointer passed by parameter. > - Handler dispatcher functions vpci_{read/write} no longer return an > error code, errors on reads/writes should be treated like hardware > (writes ignored, reads return all 1's or garbage). > - Make sure pcidevs is locked before calling pci_get_pdev_by_domain. > - Use a recursive spinlock for the vpci lock, so that spin_is_locked > checks that the current CPU is holding the lock. > - Make the code less error-chatty by removing some of the printk's. > - Pass the slot and the function as separate parameters to the > handler dispatchers (instead of passing devfn). > - Allow handlers to be registered with either a read or write > function only, the missing handler will be replaced by a dummy > handler (writes ignored, reads return 1's). > - Introduce PCI_CFG_SPACE_* defines from Linux. > - Simplify the handler dispatchers by removing the recursion, now the > dispatchers iterate over the list of sorted handlers and call them > in order. > - Remove the GENMASK_BYTES, SHIFT_RIGHT_BYTES and ADD_RESULT macros, > and instead provide a merge_result function in order to merge a > register output into a partial result. > - Rename the fields of the vpci_val union to u8/u16/u32. > - Remove the return values from the read/write handlers, errors > should be handled internally and signaled as would be done on > native hardware. > - Remove the usage of the GENMASK macro. > > Changes since v2: > - Generalize the PCI address decoding and use it for IOREQ code also. > > Changes since v1: > - Allow access to cross a word-boundary. > - Add locking. > - Add cleanup to xen_vpci_add_handlers in case of failure. > --- > .gitignore | 3 + > tools/libxl/libxl_x86.c | 2 +- > tools/tests/Makefile | 1 + > tools/tests/vpci/Makefile | 37 ++++ > tools/tests/vpci/emul.h | 133 +++++++++++ > tools/tests/vpci/main.c | 308 ++++++++++++++++++++++++++ > xen/arch/arm/xen.lds.S | 12 + > xen/arch/x86/domain.c | 18 +- > xen/arch/x86/hvm/hvm.c | 2 + > xen/arch/x86/hvm/io.c | 103 +++++++++ > xen/arch/x86/setup.c | 3 +- > xen/arch/x86/xen.lds.S | 12 + > xen/drivers/Makefile | 2 +- > xen/drivers/passthrough/pci.c | 9 +- > xen/drivers/vpci/Makefile | 1 + > xen/drivers/vpci/vpci.c | 450 > ++++++++++++++++++++++++++++++++++++++ > xen/include/asm-x86/domain.h | 1 + > xen/include/asm-x86/hvm/io.h | 3 + > xen/include/public/arch-x86/xen.h | 5 +- > xen/include/xen/pci.h | 3 + > xen/include/xen/pci_regs.h | 8 + > xen/include/xen/vpci.h | 54 +++++ > 22 files changed, 1161 insertions(+), 9 deletions(-) > create mode 100644 tools/tests/vpci/Makefile > create mode 100644 tools/tests/vpci/emul.h > create mode 100644 tools/tests/vpci/main.c > create mode 100644 xen/drivers/vpci/Makefile > create mode 100644 xen/drivers/vpci/vpci.c > create mode 100644 xen/include/xen/vpci.h > > diff --git a/.gitignore b/.gitignore > index cc16649457..1c670b27d7 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -240,6 +240,9 @@ tools/tests/regression/build/* > tools/tests/regression/downloads/* > tools/tests/mem-sharing/memshrtool > tools/tests/mce-test/tools/xen-mceinj > +tools/tests/vpci/list.h > +tools/tests/vpci/vpci.[hc] > +tools/tests/vpci/test_vpci > tools/xcutils/lsevtchn > tools/xcutils/readnotes > tools/xenbackendd/_paths.h > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c > index 455f6f0bed..dd7fc78a99 100644 > --- a/tools/libxl/libxl_x86.c > +++ b/tools/libxl/libxl_x86.c > @@ -11,7 +11,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) { > if (d_config->b_info.device_model_version != > LIBXL_DEVICE_MODEL_VERSION_NONE) { > - xc_config->emulation_flags = XEN_X86_EMU_ALL; > + xc_config->emulation_flags = (XEN_X86_EMU_ALL & > ~XEN_X86_EMU_VPCI); > } else if (libxl_defbool_val(d_config->b_info.u.hvm.apic)) { > /* > * HVM guests without device model may want > diff --git a/tools/tests/Makefile b/tools/tests/Makefile > index 7162945121..f6942a93fb 100644 > --- a/tools/tests/Makefile > +++ b/tools/tests/Makefile > @@ -13,6 +13,7 @@ endif > SUBDIRS-$(CONFIG_X86) += x86_emulator > SUBDIRS-y += xen-access > SUBDIRS-y += xenstore > +SUBDIRS-$(CONFIG_HAS_PCI) += vpci > > .PHONY: all clean install distclean uninstall > all clean distclean: %: subdirs-% > diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile > new file mode 100644 > index 0000000000..e45fcb5cd9 > --- /dev/null > +++ b/tools/tests/vpci/Makefile > @@ -0,0 +1,37 @@ > +XEN_ROOT=$(CURDIR)/../../.. > +include $(XEN_ROOT)/tools/Rules.mk > + > +TARGET := test_vpci > + > +.PHONY: all > +all: $(TARGET) > + > +.PHONY: run > +run: $(TARGET) > + ./$(TARGET) Is such target useful? It will possibly break if you invoke it outside of this directory. > + > +$(TARGET): vpci.c vpci.h list.h main.c emul.h > + $(HOSTCC) -g -o $@ vpci.c main.c > + > +.PHONY: clean > +clean: > + rm -rf $(TARGET) *.o *~ vpci.h vpci.c list.h > + > +.PHONY: distclean > +distclean: clean > + > +.PHONY: install > +install: > + > +vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c > + # Trick the compiler so it doesn't complain about missing symbols > + sed -e '/#include/d' \ > + -e '1s;^;#include "emul.h"\ > + vpci_register_init_t *const __start_vpci_array[1]\;\ > + vpci_register_init_t *const __end_vpci_array[1]\;\ > + ;' <$< >$@ This is prone to breakage. I think it would be better to just have a local stub file. Or maybe define it in your main.c? > + > +int > +main(int argc, char **argv) > +{ > + /* Index storage by offset. */ > + uint32_t r0 = 0xdeadbeef; > + uint8_t r5 = 0xef; > + uint8_t r6 = 0xbe; > + uint8_t r7 = 0xef; > + uint16_t r12 = 0x8696; > + uint8_t r16[4] = { }; > + uint16_t r20[2] = { }; Need to put 0 in the brackets. The code seems rather self-contained. Maybe we can write a fuzzer for it? _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.