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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.17 at 17:29, <roger.pau@xxxxxxxxxx> wrote:
> +static int vpci_portio_read(const struct hvm_io_handler *handler,
> +                            uint64_t addr, uint32_t size, uint64_t *data)
> +{
> +    struct domain *d = current->domain;
> +    unsigned int reg;
> +    pci_sbdf_t sbdf;
> +    uint32_t cf8;
> +
> +    *data = ~(uint64_t)0;
> +
> +    if ( addr == 0xcf8 )
> +    {
> +        ASSERT(size == 4);
> +        *data = d->arch.hvm_domain.pci_cf8;
> +        return X86EMUL_OKAY;
> +    }
> +
> +    cf8 = ACCESS_ONCE(d->arch.hvm_domain.pci_cf8);
> +    if ( !CF8_ENABLED(cf8) )
> +        return X86EMUL_OKAY;

Why is this OKAY instead of UNHANDLEABLE? The access is supposed to be
forwarded to qemu if it's not a config space one. Same in the write path
then.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -124,6 +124,12 @@ SECTIONS
>         __param_start = .;
>         *(.data.param)
>         __param_end = .;
> +
> +#if defined(CONFIG_HAS_PCI) && defined(CONFIG_LATE_HWDOM)
> +       __start_vpci_array = .;
> +       *(.data.vpci)
> +       __end_vpci_array = .;
> +#endif
>    } :text
>  
>  #if defined(BUILD_ID)
> @@ -213,6 +219,12 @@ SECTIONS
>         *(.init_array)
>         *(SORT(.init_array.*))
>         __ctors_end = .;
> +
> +#if defined(CONFIG_HAS_PCI) && !defined(CONFIG_LATE_HWDOM)
> +       __start_vpci_array = .;
> +       *(.data.vpci)
> +       __end_vpci_array = .;
> +#endif
>    } :text

Suitable alignment needs to be enforced in both cases, or else we risk
someone adding something immediately ahead of one of your insertions,
making __start_vpci_array no longer point to the first entry.

> @@ -1052,9 +1053,10 @@ static void __hwdom_init setup_one_hwdom_device(const 
> struct setup_hwdom *ctxt,
>                                                  struct pci_dev *pdev)
>  {
>      u8 devfn = pdev->devfn;
> +    int err;
>  
>      do {
> -        int err = ctxt->handler(devfn, pdev);
> +        err = ctxt->handler(devfn, pdev);
>  
>          if ( err )

Please also remove the now stray blank line.

> +int vpci_add_register(const struct pci_dev *pdev, vpci_read_t *read_handler,
> +                      vpci_write_t *write_handler, unsigned int offset,
> +                      unsigned int size, void *data)
> +{
> +    struct list_head *prev;
> +    struct vpci_register *r;
> +
> +    /* Some sanity checks. */
> +    if ( (size != 1 && size != 2 && size != 4) ||
> +         offset >= PCI_CFG_SPACE_EXP_SIZE || (offset & (size - 1)) ||
> +         (!read_handler && !write_handler) )
> +        return -EINVAL;
> +
> +    r = xmalloc(struct vpci_register);
> +    if ( !r )
> +        return -ENOMEM;
> +
> +    r->read = read_handler ?: vpci_ignored_read;
> +    r->write = write_handler ?: vpci_ignored_write;
> +    r->size = size;
> +    r->offset = offset;
> +    r->private = data;
> +
> +    spin_lock(&pdev->vpci->lock);
> +
> +    /* The list of handlers must be kept sorted at all times. */
> +    list_for_each ( prev, &pdev->vpci->handlers )
> +    {
> +        const struct vpci_register *this =
> +            list_entry(prev, const struct vpci_register, node);
> +        int cmp = vpci_register_cmp(r, this);
> +
> +        if ( cmp < 0 )
> +            break;
> +        if ( cmp == 0 )
> +        {
> +            spin_unlock(&pdev->vpci->lock);
> +            xfree(r);
> +            return -EEXIST;
> +        }
> +    }
> +
> +    list_add_tail(&r->node, prev);
> +    spin_unlock(&pdev->vpci->lock);
> +
> +    return 0;
> +}

Looking at this and its remove counterpart it is not (no longer?) clear
why they both take a struct pci_dev * as parameter - struct vpci * would
fully suffice, and would eliminate the question on whether functions
like these should have the respective parameters const-qualified.

> +/*
> + * Merge new data into a partial result.
> + *
> + * Copy the value found in 'new' from [0, size) left shifted by
> + * 'offset' into 'data'.
> + */
> +static uint32_t merge_result(uint32_t data, uint32_t new, unsigned int size,
> +                             unsigned int offset)
> +{
> +    uint32_t mask = 0xffffffff >> (32 - 8 * size);
> +
> +    return (data & ~(mask << (offset * 8))) | ((new & mask) << (offset * 8));
> +}

If a function like this one has a relatively long comment, I think that
comment should clarify that both size and offset are byte-granular.
Especially for offset (used for shifting) bit otherwise would seem more
natural to me.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel