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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> Changes since v4:
>[...]
> * Hypervisor code:
>[...]
>  - Constify the data opaque parameter of read handlers.

Is that a good idea? Such callbacks should generally be allowed to
modify their state even if the operation is just a read - think of a
private lock or statistics/debugging data to be updated.

> --- /dev/null
> +++ b/tools/tests/vpci/main.c
> @@ -0,0 +1,314 @@
> +/*
> + * Unit tests for the generic vPCI handler code.
> + *
> + * Copyright (C) 2017 Citrix Systems R&D
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "emul.h"
> +
> +/* Single vcpu (current), and single domain with a single PCI device. */
> +static struct vpci vpci;
> +
> +static struct domain d = {
> +    .lock = false,

UNLOCKED ?

> +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] = { };
> +    uint32_t r24 = 0;
> +    uint8_t r28, r30;
> +    unsigned int i;
> +    int rc;
> +
> +    INIT_LIST_HEAD(&vpci.handlers);
> +
> +    VPCI_ADD_REG(vpci_read32, vpci_write32, 0, 4, r0);
> +    VPCI_READ_CHECK(0, 4, r0);
> +    VPCI_WRITE_CHECK(0, 4, 0xbcbcbcbc);
> +
> +    VPCI_ADD_REG(vpci_read8, vpci_write8, 5, 1, r5);
> +    VPCI_READ_CHECK(5, 1, r5);
> +    VPCI_WRITE_CHECK(5, 1, 0xba);
> +
> +    VPCI_ADD_REG(vpci_read8, vpci_write8, 6, 1, r6);
> +    VPCI_READ_CHECK(6, 1, r6);
> +    VPCI_WRITE_CHECK(6, 1, 0xba);
> +
> +    VPCI_ADD_REG(vpci_read8, vpci_write8, 7, 1, r7);
> +    VPCI_READ_CHECK(7, 1, r7);
> +    VPCI_WRITE_CHECK(7, 1, 0xbd);
> +
> +    VPCI_ADD_REG(vpci_read16, vpci_write16, 12, 2, r12);
> +    VPCI_READ_CHECK(12, 2, r12);
> +    VPCI_READ_CHECK(12, 4, 0xffff8696);
> +
> +    /*
> +     * At this point we have the following layout:
> +     *
> +     * Note that this refers to the position of the variables,
> +     * but the value has already changed from the one given at
> +     * initialization time because write tests have been performed.
> +     *
> +     * 32    24    16     8     0
> +     *  +-----+-----+-----+-----+
> +     *  |          r0           | 0
> +     *  +-----+-----+-----+-----+
> +     *  | r7  |  r6 |  r5 |/////| 32
> +     *  +-----+-----+-----+-----|
> +     *  |///////////////////////| 64
> +     *  +-----------+-----------+
> +     *  |///////////|    r12    | 96
> +     *  +-----------+-----------+
> +     *             ...
> +     *  / = empty.

Maybe better "unwritten"?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -76,6 +76,11 @@ SECTIONS
>  
>    __2M_rodata_start = .;       /* Start of 2M superpages, mapped RO. */
>    .rodata : {
> +#if defined(CONFIG_HAS_PCI) && defined(CONFIG_LATE_HWDOM)
> +       __start_vpci_array = .;
> +       *(.rodata.vpci)
> +       __end_vpci_array = .;
> +#endif
>         _srodata = .;

Please don't put anything ahead of this label. And I'd prefer if
ordinary .rodata contributions came first, i.e. please don't follow
the bad example ...

>         /* Bug frames table */
>         __start_bug_frames = .;

... this gives (there are plenty of good examples further down in
this section).

> @@ -167,6 +172,11 @@ SECTIONS
>         _einittext = .;
>    } :text
>    .init.data : {
> +#if defined(CONFIG_HAS_PCI) && !defined(CONFIG_LATE_HWDOM)
> +       __start_vpci_array = .;
> +       *(.init.rodata.vpci)
> +       __end_vpci_array = .;
> +#endif
>         *(.init.rodata)

Same here then.

> +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;
> +
> +    vpci_wlock(pdev->domain);
> +
> +    /* The list of handlers must be keep sorted at all times. */

kept

> +/*
> + * Merge new data into a partial result.
> + *
> + * Zero the bytes of 'data' from [offset, offset + size), and
> + * merge the value found in 'new' from [0, offset) left shifted

DYM [0, size) here? I also have to admit that I find it strange that
you talk of zeroing something here - the net effect of the function
is not producing any zeros anywhere afaict. Such a pre-function
comment is normally describing the effect of the function as seen
to the caller rather than its inner workings.

> --- /dev/null
> +++ b/xen/include/xen/vpci.h
> @@ -0,0 +1,80 @@
> +#ifndef _VPCI_
> +#define _VPCI_

This is a little short (and unusual) for an inclusion guard.

> +
> +#include <xen/pci.h>
> +#include <xen/types.h>
> +#include <xen/list.h>
> +
> +/*
> + * Helpers for locking/unlocking.
> + *
> + * NB: the recursive variants are used so that spin_is_locked
> + * returns whether the lock is hold by the current CPU (instead
> + * of just returning whether the lock is hold by any CPU).

Stale comment?

> + */
> +#define vpci_rlock(d) read_lock(&(d)->arch.hvm_domain.vpci_lock)
> +#define vpci_wlock(d) write_lock(&(d)->arch.hvm_domain.vpci_lock)
> +#define vpci_runlock(d) read_unlock(&(d)->arch.hvm_domain.vpci_lock)
> +#define vpci_wunlock(d) write_unlock(&(d)->arch.hvm_domain.vpci_lock)
> +#define vpci_rlocked(d) rw_is_locked(&(d)->arch.hvm_domain.vpci_lock)
> +#define vpci_wlocked(d) rw_is_write_locked(&(d)->arch.hvm_domain.vpci_lock)
> +
> +/*
> + * The vPCI handlers will never be called concurrently for the same domain, 
> it
> + * is guaranteed that the vpci domain lock will always be locked when calling
> + * any handler.

One more?

> + */
> +typedef uint32_t vpci_read_t(struct pci_dev *pdev, unsigned int reg,
> +                             const void *data);
> +
> +typedef void vpci_write_t(struct pci_dev *pdev, unsigned int reg,
> +                          uint32_t val, void *data);

Do these two really need access to the struct pci_dev, rather than
just struct vpci? And if they do, then are they really permitted to
alter that struct pci_dev?

Jan

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