Re: [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -279,6 +279,18 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, 
> unsigned int addr,
>      return CF8_ADDR_LO(cf8) | (addr & 3);
>  }
>  
> +unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
> +                                   paddr_t addr, pci_sbdf_t *sbdf)
> +{
> +    addr -= mmcfg->addr;

As the function isn't even static, wouldn't it be helpful to at least
assert that addr >= mmcfg->addr ahead of this, and addr < mmcfg->size
afterwards?

> @@ -574,6 +550,17 @@ void destroy_vpci_mmcfg(struct domain *d)
>      write_unlock(&d->arch.hvm.mmcfg_lock);
>  }
>  
> +const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr)
> +{
> +    const struct hvm_mmcfg *mmcfg;
> +
> +    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
> +        if ( addr >= mmcfg->addr && addr < mmcfg->addr + mmcfg->size )

As a minor remark, this could be coded without even a theoretical risk
of overflow as

        if ( addr >= mmcfg->addr && addr - mmcfg->addr < mmcfg->size )

> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1326,27 +1326,34 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, 
> ioreq_t *p)
>      uint8_t type;
>      uint64_t addr;
>      unsigned int id;
> +    const struct hvm_mmcfg *mmcfg;
>  
>      if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
>          return XEN_INVALID_IOSERVID;
>  
>      cf8 = d->arch.hvm.pci_cf8;
>  
> -    if ( p->type == IOREQ_TYPE_PIO &&
> -         (p->addr & ~3) == 0xcfc &&
> -         CF8_ENABLED(cf8) )
> +    read_lock(&d->arch.hvm.mmcfg_lock);
> +    if ( (p->type == IOREQ_TYPE_PIO &&
> +          (p->addr & ~3) == 0xcfc &&
> +          CF8_ENABLED(cf8)) ||
> +         (p->type == IOREQ_TYPE_COPY &&
> +          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
>      {
>          uint32_t x86_fam;
>          pci_sbdf_t sbdf;
>          unsigned int reg;
>  
> -        reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> +        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
> +                                                              &sbdf)
> +                                        : hvm_mmcfg_decode_addr(mmcfg, 
> p->addr,
> +                                                                &sbdf);

I'm afraid old gcc will consider this use of mmcfg "potentially
uninitialized". I.e. I think the variable wants to gain a NULL
initializer, at which point you can slightly shorten the above
to

        reg = mmcfg ? hvm_mmcfg_decode_addr(mmcfg, p->addr, &sbdf)
                    : hvm_pci_decode_addr(cf8, p->addr, &sbdf);


also eliminating a pointer deref (i.e. likely producing minimally
more efficient code).

>          /* PCI config data cycle */
>          type = XEN_DMOP_IO_RANGE_PCI;
>          addr = ((uint64_t)sbdf.sbdf << 32) | reg;
>          /* AMD extended configuration space access? */
> -        if ( CF8_ADDR_HI(cf8) &&
> +        if ( p->type == IOREQ_TYPE_PIO && CF8_ADDR_HI(cf8) &&

Would be "!mmcfg && ..." then here.

> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -165,9 +165,19 @@ void stdvga_deinit(struct domain *d);
>  
>  extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
>  
> -/* Decode a PCI port IO access into a bus/slot/func/reg. */
> +struct hvm_mmcfg {
> +    struct list_head next;
> +    paddr_t addr;
> +    unsigned int size;
> +    uint16_t segment;
> +    uint8_t start_bus;
> +};
> +
> +/* Decode a PCI port IO or MMCFG access into a bus/slot/func/reg. */
>  unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
>                                   pci_sbdf_t *sbdf);
> +unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
> +                                   paddr_t addr, pci_sbdf_t *sbdf);

The latest now the comment also wants to include "seg/", I think.

> @@ -178,15 +188,18 @@ void register_g2m_portio_handler(struct domain *d);
>  /* HVM port IO handler for vPCI accesses. */
>  void register_vpci_portio_handler(struct domain *d);
>  
> -/* HVM MMIO handler for PCI MMCFG accesses. */
> -int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> -                                unsigned int start_bus, unsigned int end_bus,
> -                                unsigned int seg);
> -/* Destroy tracked MMCFG areas. */
> -void destroy_vpci_mmcfg(struct domain *d);
> +/* HVM PCI MMCFG regions registration. */
> +int hvm_register_mmcfg(struct domain *d, paddr_t addr,
> +                       unsigned int start_bus, unsigned int end_bus,
> +                       unsigned int seg);

As - from a hierarchy perspective - the segment is more significant, can
you put it ahead of the start/end bus numbers please?

> +void hvm_free_mmcfg(struct domain *d);
> +const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr);
>  
>  /* Check if an address is between a MMCFG region for a domain. */
> -bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr);
> +static inline bool hvm_is_mmcfg_address(const struct domain *d, paddr_t addr)
> +{
> +    return hvm_mmcfg_find(d, addr);
> +}

Hmm, yet another instance where the size of an access doesn't matter
in such a predicate. Is it a good idea (or is there a fair reason)
to extend this bad practice? But yes, I do see that vpci_mmcfg_accept()
has no way to pass a size here, and that you you effectively only
move the code here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel