| Message ID | 1474991845-27962-20-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > Sent: 27 September 2016 16:57 > To: xen-devel@lists.xenproject.org > Cc: konrad.wilk@oracle.com; boris.ostrovsky@oracle.com; Roger Pau Monne > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan > Beulich <jbeulich@suse.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com> > Subject: [PATCH v2 19/30] xen/dcpi: add a dpci passthrough handler for > hardware domain > > This is very similar to the PCI trap used for the traditional PV(H) Dom0. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Paul Durrant <paul.durrant@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/hvm/io.c | 72 > ++++++++++++++++++++++++++++++++++++++++++- > xen/arch/x86/traps.c | 39 ----------------------- > xen/drivers/passthrough/pci.c | 39 +++++++++++++++++++++++ > xen/include/xen/pci.h | 2 ++ > 4 files changed, 112 insertions(+), 40 deletions(-) > > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index > 1e7a5f9..31d54dc 100644 > --- a/xen/arch/x86/hvm/io.c > +++ b/xen/arch/x86/hvm/io.c > @@ -247,12 +247,79 @@ static int dpci_portio_write(const struct > hvm_io_handler *handler, > return X86EMUL_OKAY; > } > > +static bool_t hw_dpci_portio_accept(const struct hvm_io_handler > *handler, > + const ioreq_t *p) { > + if ( (p->addr == 0xcf8 && p->size == 4) || (p->addr & 0xfffc) == 0xcfc) > + { > + return 1; > + } > + > + return 0; Why not just return the value of the boolean expression? > +} > + > +static int hw_dpci_portio_read(const struct hvm_io_handler *handler, > + uint64_t addr, > + uint32_t size, > + uint64_t *data) { > + struct domain *currd = current->domain; > + > + if ( addr == 0xcf8 ) > + { > + ASSERT(size == 4); > + *data = currd->arch.pci_cf8; > + return X86EMUL_OKAY; > + } > + > + ASSERT((addr & 0xfffc) == 0xcfc); You could do 'addr &= 3' and simplify the expressions below. > + size = min(size, 4 - ((uint32_t)addr & 3)); > + if ( size == 3 ) > + size = 2; > + if ( pci_cfg_ok(currd, addr & 3, size, NULL) ) Is this the right place to do the check. Can this not be done in the accept op? > + *data = pci_conf_read(currd->arch.pci_cf8, addr & 3, size); > + > + return X86EMUL_OKAY; > +} > + > +static int hw_dpci_portio_write(const struct hvm_io_handler *handler, > + uint64_t addr, > + uint32_t size, > + uint64_t data) { > + struct domain *currd = current->domain; > + uint32_t data32; > + > + if ( addr == 0xcf8 ) > + { > + ASSERT(size == 4); > + currd->arch.pci_cf8 = data; > + return X86EMUL_OKAY; > + } > + > + ASSERT((addr & 0xfffc) == 0xcfc); 'addr &= 3' again here. Paul > + size = min(size, 4 - ((uint32_t)addr & 3)); > + if ( size == 3 ) > + size = 2; > + data32 = data; > + if ( pci_cfg_ok(currd, addr & 3, size, &data32) ) > + pci_conf_write(currd->arch.pci_cf8, addr & 3, size, data); > + > + return X86EMUL_OKAY; > +} > + > static const struct hvm_io_ops dpci_portio_ops = { > .accept = dpci_portio_accept, > .read = dpci_portio_read, > .write = dpci_portio_write > }; > > +static const struct hvm_io_ops hw_dpci_portio_ops = { > + .accept = hw_dpci_portio_accept, > + .read = hw_dpci_portio_read, > + .write = hw_dpci_portio_write > +}; > + > void register_dpci_portio_handler(struct domain *d) { > struct hvm_io_handler *handler = hvm_next_io_handler(d); @@ -261,7 > +328,10 @@ void register_dpci_portio_handler(struct domain *d) > return; > > handler->type = IOREQ_TYPE_PIO; > - handler->ops = &dpci_portio_ops; > + if ( is_hardware_domain(d) ) > + handler->ops = &hw_dpci_portio_ops; > + else > + handler->ops = &dpci_portio_ops; > } > > /* > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index > 24d173f..f3c5c9e 100644 > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2076,45 +2076,6 @@ static bool_t admin_io_okay(unsigned int port, > unsigned int bytes, > return ioports_access_permitted(d, port, port + bytes - 1); } > > -static bool_t pci_cfg_ok(struct domain *currd, unsigned int start, > - unsigned int size, uint32_t *write) > -{ > - uint32_t machine_bdf; > - > - if ( !is_hardware_domain(currd) ) > - return 0; > - > - if ( !CF8_ENABLED(currd->arch.pci_cf8) ) > - return 1; > - > - machine_bdf = CF8_BDF(currd->arch.pci_cf8); > - if ( write ) > - { > - const unsigned long *ro_map = pci_get_ro_map(0); > - > - if ( ro_map && test_bit(machine_bdf, ro_map) ) > - return 0; > - } > - start |= CF8_ADDR_LO(currd->arch.pci_cf8); > - /* AMD extended configuration space access? */ > - if ( CF8_ADDR_HI(currd->arch.pci_cf8) && > - boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > - boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 ) > - { > - uint64_t msr_val; > - > - if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) ) > - return 0; > - if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) ) > - start |= CF8_ADDR_HI(currd->arch.pci_cf8); > - } > - > - return !write ? > - xsm_pci_config_permission(XSM_HOOK, currd, machine_bdf, > - start, start + size - 1, 0) == 0 : > - pci_conf_write_intercept(0, machine_bdf, start, size, write) >= 0; > -} > - > uint32_t guest_io_read(unsigned int port, unsigned int bytes, > struct domain *currd) { diff --git a/xen/drivers/passthrough/pci.c > b/xen/drivers/passthrough/pci.c index dd291a2..a53b4c8 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -966,6 +966,45 @@ void pci_check_disable_device(u16 seg, u8 bus, u8 > devfn) > PCI_COMMAND, cword & ~PCI_COMMAND_MASTER); } > > +bool_t pci_cfg_ok(struct domain *currd, unsigned int start, > + unsigned int size, uint32_t *write) { > + uint32_t machine_bdf; > + > + if ( !is_hardware_domain(currd) ) > + return 0; > + > + if ( !CF8_ENABLED(currd->arch.pci_cf8) ) > + return 1; > + > + machine_bdf = CF8_BDF(currd->arch.pci_cf8); > + if ( write ) > + { > + const unsigned long *ro_map = pci_get_ro_map(0); > + > + if ( ro_map && test_bit(machine_bdf, ro_map) ) > + return 0; > + } > + start |= CF8_ADDR_LO(currd->arch.pci_cf8); > + /* AMD extended configuration space access? */ > + if ( CF8_ADDR_HI(currd->arch.pci_cf8) && > + boot_cpu_data.x86_vendor == X86_VENDOR_AMD && > + boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 ) > + { > + uint64_t msr_val; > + > + if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) ) > + return 0; > + if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) ) > + start |= CF8_ADDR_HI(currd->arch.pci_cf8); > + } > + > + return !write ? > + xsm_pci_config_permission(XSM_HOOK, currd, machine_bdf, > + start, start + size - 1, 0) == 0 : > + pci_conf_write_intercept(0, machine_bdf, start, size, write) > +>= 0; } > + > /* > * scan pci devices to add all existed PCI devices to alldevs_list, > * and setup pci hierarchy in array bus2bridge. > diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index > 0872401..f191773 100644 > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -162,6 +162,8 @@ const char *parse_pci(const char *, unsigned int *seg, > unsigned int *bus, > > bool_t pcie_aer_get_firmware_first(const struct pci_dev *); > > +bool_t pci_cfg_ok(struct domain *, unsigned int, unsigned int, uint32_t > +*); > + > struct pirq; > int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable); void > msixtbl_pt_unregister(struct domain *, struct pirq *); > -- > 2.7.4 (Apple Git-66)
On Mon, Oct 03, 2016 at 10:02:27AM +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Roger Pau Monne [mailto:roger.pau@citrix.com] > > Sent: 27 September 2016 16:57 > > To: xen-devel@lists.xenproject.org > > Cc: konrad.wilk@oracle.com; boris.ostrovsky@oracle.com; Roger Pau Monne > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Jan > > Beulich <jbeulich@suse.com>; Andrew Cooper > > <Andrew.Cooper3@citrix.com> > > Subject: [PATCH v2 19/30] xen/dcpi: add a dpci passthrough handler for > > hardware domain > > > > This is very similar to the PCI trap used for the traditional PV(H) Dom0. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Cc: Paul Durrant <paul.durrant@citrix.com> > > Cc: Jan Beulich <jbeulich@suse.com> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > xen/arch/x86/hvm/io.c | 72 > > ++++++++++++++++++++++++++++++++++++++++++- > > xen/arch/x86/traps.c | 39 ----------------------- > > xen/drivers/passthrough/pci.c | 39 +++++++++++++++++++++++ > > xen/include/xen/pci.h | 2 ++ > > 4 files changed, 112 insertions(+), 40 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index > > 1e7a5f9..31d54dc 100644 > > --- a/xen/arch/x86/hvm/io.c > > +++ b/xen/arch/x86/hvm/io.c > > @@ -247,12 +247,79 @@ static int dpci_portio_write(const struct > > hvm_io_handler *handler, > > return X86EMUL_OKAY; > > } > > > > +static bool_t hw_dpci_portio_accept(const struct hvm_io_handler > > *handler, > > + const ioreq_t *p) { > > + if ( (p->addr == 0xcf8 && p->size == 4) || (p->addr & 0xfffc) == 0xcfc) > > + { > > + return 1; > > + } > > + > > + return 0; > > Why not just return the value of the boolean expression? Thanks, fixed. > > +} > > + > > +static int hw_dpci_portio_read(const struct hvm_io_handler *handler, > > + uint64_t addr, > > + uint32_t size, > > + uint64_t *data) { > > + struct domain *currd = current->domain; > > + > > + if ( addr == 0xcf8 ) > > + { > > + ASSERT(size == 4); > > + *data = currd->arch.pci_cf8; > > + return X86EMUL_OKAY; > > + } > > + > > + ASSERT((addr & 0xfffc) == 0xcfc); > > You could do 'addr &= 3' and simplify the expressions below. Fixed. > > + size = min(size, 4 - ((uint32_t)addr & 3)); > > + if ( size == 3 ) > > + size = 2; > > + if ( pci_cfg_ok(currd, addr & 3, size, NULL) ) > > Is this the right place to do the check. Can this not be done in the accept op? In the intercept op we don't know if the operation is a read or a write, and pci_cfg_ok seems to do more than a check, it calls pci_conf_write_intercept. > > + *data = pci_conf_read(currd->arch.pci_cf8, addr & 3, size); > > + > > + return X86EMUL_OKAY; > > +} > > + > > +static int hw_dpci_portio_write(const struct hvm_io_handler *handler, > > + uint64_t addr, > > + uint32_t size, > > + uint64_t data) { > > + struct domain *currd = current->domain; > > + uint32_t data32; > > + > > + if ( addr == 0xcf8 ) > > + { > > + ASSERT(size == 4); > > + currd->arch.pci_cf8 = data; > > + return X86EMUL_OKAY; > > + } > > + > > + ASSERT((addr & 0xfffc) == 0xcfc); > > 'addr &= 3' again here. Thanks. Roger.
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2076,45 +2076,6 @@ static bool_t admin_io_okay(unsigned int port, unsigned int bytes, > return ioports_access_permitted(d, port, port + bytes - 1); > } > > -static bool_t pci_cfg_ok(struct domain *currd, unsigned int start, > - unsigned int size, uint32_t *write) I don't follow why you move this ... > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -966,6 +966,45 @@ void pci_check_disable_device(u16 seg, u8 bus, u8 devfn) > PCI_COMMAND, cword & ~PCI_COMMAND_MASTER); > } > > +bool_t pci_cfg_ok(struct domain *currd, unsigned int start, > + unsigned int size, uint32_t *write) here. After all ... > +{ > + uint32_t machine_bdf; > + > + if ( !is_hardware_domain(currd) ) > + return 0; > + > + if ( !CF8_ENABLED(currd->arch.pci_cf8) ) > + return 1; > + > + machine_bdf = CF8_BDF(currd->arch.pci_cf8); ... concepts like I/O ports in general and ports CF8 / CFC in particular are x86-specific. If this really needs moving at all, it should stay in an x86-specific file. Jan
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index 1e7a5f9..31d54dc 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -247,12 +247,79 @@ static int dpci_portio_write(const struct hvm_io_handler *handler, return X86EMUL_OKAY; } +static bool_t hw_dpci_portio_accept(const struct hvm_io_handler *handler, + const ioreq_t *p) +{ + if ( (p->addr == 0xcf8 && p->size == 4) || (p->addr & 0xfffc) == 0xcfc) + { + return 1; + } + + return 0; +} + +static int hw_dpci_portio_read(const struct hvm_io_handler *handler, + uint64_t addr, + uint32_t size, + uint64_t *data) +{ + struct domain *currd = current->domain; + + if ( addr == 0xcf8 ) + { + ASSERT(size == 4); + *data = currd->arch.pci_cf8; + return X86EMUL_OKAY; + } + + ASSERT((addr & 0xfffc) == 0xcfc); + size = min(size, 4 - ((uint32_t)addr & 3)); + if ( size == 3 ) + size = 2; + if ( pci_cfg_ok(currd, addr & 3, size, NULL) ) + *data = pci_conf_read(currd->arch.pci_cf8, addr & 3, size); + + return X86EMUL_OKAY; +} + +static int hw_dpci_portio_write(const struct hvm_io_handler *handler, + uint64_t addr, + uint32_t size, + uint64_t data) +{ + struct domain *currd = current->domain; + uint32_t data32; + + if ( addr == 0xcf8 ) + { + ASSERT(size == 4); + currd->arch.pci_cf8 = data; + return X86EMUL_OKAY; + } + + ASSERT((addr & 0xfffc) == 0xcfc); + size = min(size, 4 - ((uint32_t)addr & 3)); + if ( size == 3 ) + size = 2; + data32 = data; + if ( pci_cfg_ok(currd, addr & 3, size, &data32) ) + pci_conf_write(currd->arch.pci_cf8, addr & 3, size, data); + + return X86EMUL_OKAY; +} + static const struct hvm_io_ops dpci_portio_ops = { .accept = dpci_portio_accept, .read = dpci_portio_read, .write = dpci_portio_write }; +static const struct hvm_io_ops hw_dpci_portio_ops = { + .accept = hw_dpci_portio_accept, + .read = hw_dpci_portio_read, + .write = hw_dpci_portio_write +}; + void register_dpci_portio_handler(struct domain *d) { struct hvm_io_handler *handler = hvm_next_io_handler(d); @@ -261,7 +328,10 @@ void register_dpci_portio_handler(struct domain *d) return; handler->type = IOREQ_TYPE_PIO; - handler->ops = &dpci_portio_ops; + if ( is_hardware_domain(d) ) + handler->ops = &hw_dpci_portio_ops; + else + handler->ops = &dpci_portio_ops; } /* diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 24d173f..f3c5c9e 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -2076,45 +2076,6 @@ static bool_t admin_io_okay(unsigned int port, unsigned int bytes, return ioports_access_permitted(d, port, port + bytes - 1); } -static bool_t pci_cfg_ok(struct domain *currd, unsigned int start, - unsigned int size, uint32_t *write) -{ - uint32_t machine_bdf; - - if ( !is_hardware_domain(currd) ) - return 0; - - if ( !CF8_ENABLED(currd->arch.pci_cf8) ) - return 1; - - machine_bdf = CF8_BDF(currd->arch.pci_cf8); - if ( write ) - { - const unsigned long *ro_map = pci_get_ro_map(0); - - if ( ro_map && test_bit(machine_bdf, ro_map) ) - return 0; - } - start |= CF8_ADDR_LO(currd->arch.pci_cf8); - /* AMD extended configuration space access? */ - if ( CF8_ADDR_HI(currd->arch.pci_cf8) && - boot_cpu_data.x86_vendor == X86_VENDOR_AMD && - boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 ) - { - uint64_t msr_val; - - if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) ) - return 0; - if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) ) - start |= CF8_ADDR_HI(currd->arch.pci_cf8); - } - - return !write ? - xsm_pci_config_permission(XSM_HOOK, currd, machine_bdf, - start, start + size - 1, 0) == 0 : - pci_conf_write_intercept(0, machine_bdf, start, size, write) >= 0; -} - uint32_t guest_io_read(unsigned int port, unsigned int bytes, struct domain *currd) { diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index dd291a2..a53b4c8 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -966,6 +966,45 @@ void pci_check_disable_device(u16 seg, u8 bus, u8 devfn) PCI_COMMAND, cword & ~PCI_COMMAND_MASTER); } +bool_t pci_cfg_ok(struct domain *currd, unsigned int start, + unsigned int size, uint32_t *write) +{ + uint32_t machine_bdf; + + if ( !is_hardware_domain(currd) ) + return 0; + + if ( !CF8_ENABLED(currd->arch.pci_cf8) ) + return 1; + + machine_bdf = CF8_BDF(currd->arch.pci_cf8); + if ( write ) + { + const unsigned long *ro_map = pci_get_ro_map(0); + + if ( ro_map && test_bit(machine_bdf, ro_map) ) + return 0; + } + start |= CF8_ADDR_LO(currd->arch.pci_cf8); + /* AMD extended configuration space access? */ + if ( CF8_ADDR_HI(currd->arch.pci_cf8) && + boot_cpu_data.x86_vendor == X86_VENDOR_AMD && + boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 ) + { + uint64_t msr_val; + + if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) ) + return 0; + if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) ) + start |= CF8_ADDR_HI(currd->arch.pci_cf8); + } + + return !write ? + xsm_pci_config_permission(XSM_HOOK, currd, machine_bdf, + start, start + size - 1, 0) == 0 : + pci_conf_write_intercept(0, machine_bdf, start, size, write) >= 0; +} + /* * scan pci devices to add all existed PCI devices to alldevs_list, * and setup pci hierarchy in array bus2bridge. diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 0872401..f191773 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -162,6 +162,8 @@ const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, bool_t pcie_aer_get_firmware_first(const struct pci_dev *); +bool_t pci_cfg_ok(struct domain *, unsigned int, unsigned int, uint32_t *); + struct pirq; int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable); void msixtbl_pt_unregister(struct domain *, struct pirq *);
This is very similar to the PCI trap used for the traditional PV(H) Dom0. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Paul Durrant <paul.durrant@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/hvm/io.c | 72 ++++++++++++++++++++++++++++++++++++++++++- xen/arch/x86/traps.c | 39 ----------------------- xen/drivers/passthrough/pci.c | 39 +++++++++++++++++++++++ xen/include/xen/pci.h | 2 ++ 4 files changed, 112 insertions(+), 40 deletions(-)