> -----Original Message-----
> From: Roger Pau Monne [mailto:[email protected]]
> Sent: 27 September 2016 16:57
> To: [email protected]
> Cc: [email protected]; [email protected]; Roger Pau Monne
> <[email protected]>; Paul Durrant <[email protected]>; Jan
> Beulich <[email protected]>; Andrew Cooper
> <[email protected]>
> 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é <[email protected]>
> ---
> Cc: Paul Durrant <[email protected]>
> Cc: Jan Beulich <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> ---
> 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)
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
