Re: [Xen-devel] [PATCH v4 1/9] xen/vpci: introduce basic handlers to trap accesses to the PCI config space

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jul 13, 2017 at 08:36:18AM -0600, Jan Beulich wrote:
> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:01 PM >>>
> > --- /dev/null
> > +++ b/tools/tests/vpci/Makefile
> > @@ -0,0 +1,40 @@
> > +
> > +XEN_ROOT=$(CURDIR)/../../..
> > +include $(XEN_ROOT)/tools/Rules.mk
> > +
> > +TARGET := test_vpci
> > +
> > +.PHONY: all
> > +all: $(TARGET)
> > +
> > +.PHONY: run
> > +run: $(TARGET)
> > +    ./$(TARGET) > $(TARGET).out
> 
> Is this a good way to run a test? Aiui it'll result in there not being
> anything visible immediately; one has to go look at the produced file.
> I'd suggest to leave it to the person invoking "make run" whether to
> redirect output.

OK, this is based in the hpet testing code, that does this. I'm fine
with not redirecting the output.

> > +$(TARGET): vpci.c vpci.h list.h
> > +    $(HOSTCC) -g -o $@ vpci.c main.c
> 
> If you compile main.c, why is there no dependency on it? And how about
> emul.h?

I didn't add such dependencies because those files are not the result
of any other targets, but I agree it's better to explicitly list them.

> > +.PHONY: clean
> > +clean:
> > +    rm -rf $(TARGET) $(TARGET).out *.o *~ vpci.h vpci.c list.h
> > +
> > +.PHONY: distclean
> > +distclean: clean
> > +
> > +.PHONY: install
> > +install:
> > +
> > +vpci.h: $(XEN_ROOT)/xen/include/xen/vpci.h
> > +    sed -e '/#include/d' <$< >$@
> 
> Couldn't you combine this and list.h's rule into a pattern one?

Yes, I think so, let me try.

> > --- /dev/null
> > +++ b/tools/tests/vpci/emul.h
> > @@ -0,0 +1,117 @@
> > +/*
> > + * 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/>.
> > + */
> > +
> > +#ifndef _TEST_VPCI_
> > +#define _TEST_VPCI_
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <stddef.h>
> > +#include <stdint.h>
> > +#include <stdbool.h>
> > +#include <errno.h>
> > +#include <assert.h>
> > +
> > +#define container_of(ptr, type, member) ({                      \
> > +        typeof(((type *)0)->member) *__mptr = (ptr);            \
> > +        (type *)((char *)__mptr - offsetof(type, member));      \
> 
> I don't know what tools maintainers think about such name space
> violations; in hypervisor code I'd ask you to avoid leading underscores
> in macro local variables (same in min()/max() and elsewhere then).

OK. container_of, max and min and verbatim copies of the macros in
xen/include/xen/kernel.h, with the style adjusted in the container_of
case IIRC (as requested in the previous review).

> > +/* Read a 32b register using all possible sizes. */
> > +void multiread4(unsigned int reg, uint32_t val)
> > +{
> > +    unsigned int i;
> > +
> > +    /* Read using bytes. */
> > +    for ( i = 0; i < 4; i++ )
> > +        VPCI_READ_CHECK(reg + i, 1, (val >> (i * 8)) & UINT8_MAX);
> > +
> > +    /* Read using 2bytes. */
> > +    for ( i = 0; i < 2; i++ )
> > +        VPCI_READ_CHECK(reg + i * 2, 2, (val >> (i * 2 * 8)) & UINT16_MAX);
> > +
> > +    VPCI_READ_CHECK(reg, 4, val);
> > +}
> > +
> > +void multiwrite4_check(unsigned int reg, uint32_t val)
> 
> Naming question again: Why the _check suffix here, but not on the read
> function above?

Right, I guess it's clearer to add the _check prefix to both. I didn't
add it to the read one because I felt it was already implicit, while
on the write one not so much.

> > +{
> > +    unsigned int i;
> > +
> > +    /* Write using bytes. */
> > +    for ( i = 0; i < 4; i++ )
> > +        VPCI_WRITE_CHECK(reg + i, 1, (val >> (i * 8)) & UINT8_MAX);
> > +    multiread4(reg, val);
> > +
> > +    /* Write using 2bytes. */
> > +    for ( i = 0; i < 2; i++ )
> > +        VPCI_WRITE_CHECK(reg + i * 2, 2, (val >> (i * 2 * 8)) & 
> > UINT16_MAX);
> > +    multiread4(reg, val);
> > +
> > +    VPCI_WRITE_CHECK(reg, 4, val);
> > +    multiread4(reg, val);
> > +}
> 
> Wouldn't it be better to vary the value written between the individual
> sizes? Perhaps move the 32-bit write between the two loops, using ~val?
> Otherwise you won't know whether what you read back is a result of the
> writes you actually mean to test or earlier ones?

So storing a new value in val between each size test? I could even use
something randomly generated.

> > +int
> > +main(int argc, char **argv)
> > +{
> > +    /* Index storage by offset. */
> > +    uint32_t r0 = 0xdeadbeef;
> > +    uint8> +    uint16_t r20[2] = { 0 };
> 
> Just { } will suffice.
> 
> > +    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, 0xdeadbeef);
> 
> Why aren't you using r0 here?

Yes, that would be better (and safer in case this is changed).

> > +    VPCI_WRITE_CHECK(0, 4, 0xbcbcbcbc);
> > +
> > +    VPCI_ADD_REG(vpci_read8, vpci_write8, 5, 1, r5);
> > +    VPCI_READ_CHECK(5, 1, 0xef);
> > +    VPCI_WRITE_CHECK(5, 1, 0xba);
> > +
> > +    VPCI_ADD_REG(vpci_read8, vpci_write8, 6, 1, r6);
> > +    VPCI_READ_CHECK(6, 1, 0xbe);
> > +    VPCI_WRITE_CHECK(6, 1, 0xba);
> > +
> > +    VPCI_ADD_REG(vpci_read8, vpci_write8, 7, 1, r7);
> > +    VPCI_READ_CHECK(7, 1, 0xef);
> > +    VPCI_WRITE_CHECK(7, 1, 0xbd);
> > +
> > +    VPCI_ADD_REG(vpci_read16, vpci_write16, 12, 2, r12);
> > +    VPCI_READ_CHECK(12, 2, 0x8696);
> > +    VPCI_READ_CHECK(12, 4, 0xffff8696);
> > +
> > +    /*
> > +     * At this point we have the following layout:
> > +     *
> > +     * 32    24    16     8     0
> > +     *  +-----+-----+-----+-----+
> > +     *  |          r0           | 0
> > +     *  +-----+-----+-----+-----+
> > +     *  | r7  |  r6 |  r5 |/////| 32
> > +     *  +-----+-----+-----+-----|
> 
> This is misleading (especially for readers of the code following this
> comment), as you've written different values by now.

Well, the position of the variables that hold the values of each
register are correct, it's just the value they store that has changed.

> > +     *  |///////////////////////| 64
> > +     *  +-----------+-----------+
> > +     *  |///////////|    r12    | 96
> > +     *  +-----------+-----------+
> > +     *             ...
> > +     *  / = empty.
> > +     */
> > +
> > +    /* Try to add an overlapping register handler. */
> > +    VPCI_ADD_INVALID_REG(vpci_read32, vpci_write32, 4, 4);
> > +
> > +    /* Try to add a non-aligned register. */
> > +    VPCI_ADD_INVALID_REG(vpci_read16, vpci_write16, 15, 2);
> > +
> > +    /* Try to add a register with wrong size. */
> > +    VPCI_ADD_INVALID_REG(vpci_read16, vpci_write16, 8, 3);
> > +
> > +    /* Try to add a register with missing handlers. */
> > +    VPCI_ADD_INVALID_REG(NULL, NULL, 8, 2);
> > +
> > +    /* Read/write of unset register. */
> > +    VPCI_READ_CHECK(8, 4, 0xffffffff);
> > +    VPCI_READ_CHECK(8, 2, 0xffff);
> > +    VPCI_READ_CHECK(8, 1, 0xff);
> > +    VPCI_WRITE(10, 2, 0xbeef);
> > +    VPCI_READ_CHECK(10, 2, 0xffff);
> > +
> > +    /* Read of multiple registers */
> > +    VPCI_WRITE_CHECK(7, 1, 0xbd);
> > +    VPCI_READ_CHECK(4, 4, 0xbdbabaff);
> > +
> > +    /* Partial read of a register. */
> > +    VPCI_WRITE_CHECK(0, 4, 0x1a1b1c1d);
> > +    VPCI_READ_CHECK(2, 1, 0x1b);
> > +    VPCI_READ_CHECK(6, 2, 0xbdba);
> > +
> > +    /* Write of multiple registers. */
> > +    VPCI_WRITE_CHECK(4, 4, 0xaabbccff);
> > +
> > +    /* Partial write of a register. */
> > +    VPCI_WRITE_CHECK(2, 1, 0xfe);
> > +    VPCI_WRITE_CHECK(6, 2, 0xfebc);
> > +
> > +    /*
> > +     * Test all possible read/write size combinations.
> > +     *
> > +     * Populate 128bits (16B) with 1B registers, 160bits (20B) with 2B
> > +     * registers, and finally 192bits (24B) with 4B registers.
> 
> I can't see how the numbers here are in line with the code this is
> meant to describe. Perhaps this is a leftover from an earlier variant
> of the code?

I'm not sure I understand this, the registers (or layout) described in
this comment are just added below the comment. Would you like me to
first add the registers and place the comment afterwards?

> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -41,6 +41,9 @@ SECTIONS
> >  
> >    . = ALIGN(PAGE_SIZE);
> >    .rodata : {
> > +       __start_vpci_array = .;
> > +       *(.rodata.vpci)
> > +       __end_vpci_array = .;
> 
> Do you really need this (unconditionally)?

Right, this should have a ifdef CONFIG_PCI.

> > +static int vpci_access_check(unsigned int reg, unsigned int len)
> 
> The way you use it, this function want to return bool.
> 
> > +void hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
> > +                         unsigned int *bus, unsigned int *slot,
> > +                         unsigned int *func, unsigned int *reg)
> 
> Since you return nothing right now, how about avoid one of the
> indirections? Best candidate would probably be the register value.

I don't really like functions that return some data in the return
value (if it's not an error code) and some other data in parameters.
But yes, if it has to return something I guess the register value is
the one that makes more sense.

> > +{
> > +    unsigned long bdf;
> 
> Why long instead of int?
> 
> > +static bool vpci_portio_accept(const struct hv> +    return (p->addr == 
> > 0xcf8 && p->size == 4) || (p->addr & 0xfffc) == 0xcfc;
> 
> Maybe better ~3 instead of 0xfffc (also likely to produce slightly
> better code)?

Yes, it's certainly not any worse than using 0xfffc. Maybe a define
would be helpful.

> > +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 bus, slot, func, reg;
> > +
> > +    *data = ~(uint64_t)0;
> > +
> > +    vpci_lock(d);
> > +    if ( addr == 0xcf8 )
> > +    {
> > +        ASSERT(size == 4);
> > +        *data = d->arch.hvm_domain.pci_cf8;
> > +        vpci_unlock(d);
> > +        return X86EMUL_OKAY;
> > +    }
> > +    if ( !CF8_ENABLED(d->arch.hvm_domain.pci_cf8) )
> > +    {
> > +        vpci_unlock(d);
> > +        return X86EMUL_OKAY;
> > +    }
> > +
> > +    /* Decode the PCI address. */
> > +    hvm_pci_decode_addr(d->arch.hvm_domain.pci_cf8, addr, &bus, &slot, 
> > &func,
> > +                        ®);
> 
> With the function name I don't view a comment like the one here as very
> useful.
> 
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -1178,18 +1178,16 @@ struct hvm_ioreq_server 
> > *hvm_select_ioreq_server(struct domain *d,
> >           CF8_ENABLED(cf8) )
> >      {
> >          uint32_t sbdf, x86_fam;
> > +        unsigned int bus, slot, func, reg;
> > +
> > +        hvm_pci_decode_addr(cf8, p->addr, &bus, &slot, &func, ®);
> >  
> >          /* PCI config data cycle */
> >  
> > -        sbdf = XEN_DMOP_PCI_SBDF(0,
> > -                                 PCI_BUS(CF8_BDF(cf8)),
> > -                                 PCI_SLOT(CF8_BDF(cf8)),
> > -                                 PCI_FUNC(CF8_BDF(cf8)));
> > +        sbdf = XEN_DMOP_PCI_SBDF(0, bus, slot, func);
> >  
> >          type = XEN_DMOP_IO_RANGE_PCI;
> > -        addr = ((uint64_t)sbdf << 32) |
> > -               CF8_ADDR_LO(cf8) |
> > -               (p->addr & 3);
> > +        addr = ((uint64_t)sbdf << 32) | reg;
> >          /* AMD extended configuration space access? */
> >          if ( CF8_ADDR_HI(cf8) &&
> >               d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
> 
> This and the introduction of hvm_pci_decode_addr() would likely better
> be broken out into a prereq patch, as this one is quite large even
> without this effectively unrelated change.

OK.

> > --- /dev/null
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -0,0 +1,405 @@
> > +/*
> > + * Generic functionality for handling accesses to the PCI configuration 
> > space
> > + * from guests.
> > + *
> > + * 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 <xen/sched.h>
> > +#include <xen/vpci.h>
> > +
> > +extern const vpci_register_init_t __start_vpci_array[], __end_vpci_array[];
> > +#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
> > +
> > +/* Internal struct to store the emulated PCI registers. */
> > +struct vpci_register {
> > +    vpci_read_t *read;
> > +    vpci_write_t *write;
> > +    unsigned int size;
> > +    unsigned int offset;
> > +    void *private;
> > +    struct list_head node;
> > +};
> > +
> > +int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> 
> As pointed out in reply to an earlier version, this lacks a prereq
> change: setup_one_hwdom_device() needs to be marked __hwdom_init. And
> then, now that you have the annotation here, the placement of the
> array in the linker script should depend on whether __hwdom_init is an
> alias of __init.

The __hwdom_init prefix is dropped shortly from this function (patch
#3), but I agree on sending a pre-patch to address
setup_one_hwdom_device.

The linker script I'm not sure it's worth modifying, by the end of the
series the list of handlers must reside in .rodata.

> > +int vpci_add_register(const struct pci_dev *pdev, vpci_read_t r> +         
> >              unsigned int size, void *data)
> > +{
> > +    struct list_head *head;
> > +    struct vpci_register *r;
> > +
> > +    /* Some sanity checks. */
> > +    if ( (size != 1 && size != 2 && size != 4) ||
> > +         offset >= PCI_CFG_SPACE_EXP_SIZE || offset & (size - 1) ||
> 
> Please add parens around the operands of &.
> 
> > +         (read_handler == NULL && write_handler == NULL) )
> 
> Please be consistent with NULL checks - as they're shorter, I'd suggest
> to always use ...
> 
> > +        return -EINVAL;
> > +
> > +    r = xmalloc(struct vpci_register);
> > +    if ( !r )
> 
> ... this style.

I'm trying, but this is different from BSD coding style that only
allows explicitly checking against NULL, so sometimes I fail, sorry.

> > +        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_lock(pdev->domain);
> > +
> > +    /* The list of handlers must be keep sorted at all times. */
> 
> kept
> 
> > +    list_for_each ( head, &pdev->vpci->handlers )
> 
> "head" is not a good name for something that doesn't always point at
> the head of whatever list. How about "prev"?
> 
> > +int vpci_remove_register(const struct pci_dev *pdev, unsigned int offset,
> > +                         unsigned int size)
> > +{
> > +    const struct vpci_register r = { .offset = offset, .size = size };
> > +    struct vpci_register *rm = NULL;
> 
> Pointless initializer afaict (there's none on the equivalent variable
> in the add function).
>
> > +    vpci_lock(pdev->domain);
> > +
> > +    list_for_each_entry ( rm, &pdev->vpci->handlers, node )
> > +        if ( vpci_register_cmp(&r, rm) <= 0 )
> > +            break;
> > +
> > +    if ( !rm || rm->offset != offset || rm->size != size )
> 
> Obviously the !rm check here isn't needed then either, which points out
> that you have a problem here: You don't properly handle the case of not
> coming through the "break" path above, i.e. when rm points at the list
> head (which isn't a full struct vpci_register).

Right (also applies to the comment above), IMHO the interface of the
Linux lists seems terrible. Will fix it.

> > +static uint32_t vpci_read_hw(unsigned int seg, unsigned int bus,
> > +                             unsigned int slot, unsigned int func,
> > +                             unsigned int reg, uint32_t size)
> > +{
> > +    uint32_t data;
> > +
> > +    switch ( size )
> > +    {
> > +    case 4:
> > +        data = pci_conf_read32(seg, bus, slot, func, reg);
> > +        break;
> > +    case 2:
> > +        data = pci_conf_read16(seg, bus, slot, func, reg);
> > +        break;
> > +    case 1:
> > +        data = pci_conf_read8(seg, bus, slot, func, reg);
> > +        break;
> > +    default:
> > +        BUG();
> 
> As long as this is Dom0-only, BUG()s like this are probably fine, but
> if this ever gets extended to DomU-s, will we really remember to
> convert them?

ASSERT_UNREACHABLE() and set data to ~0 to be safe?

> > +/*
> > + * 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
> > + * by 'offset'.
> > + */
> > +uint32_t merge_result(uint32_t data, uint32_t new, unsigned int size,
> 
> static?

Ups.

> > +                      unsigned int offset)
> > +{
> > +    uint32_t mask = ((uint64_t)1 << (size * 8)) - 1;
> 
> No need to use 64-bit arithmetic here: 0xffffffff >> (32 - 8 * size).

Shame, will fix.

> > +uint32_t vpci_read(unsigned int seg, unsigned int bus, unsigned int slot,
> > +                   unsigned int func, unsigned int reg, uint32_t size)
> > +{
> > +    struct domain *d = current->domain;
> > +    struct pci_dev *pdev;
> > +    const struct vpci_register *r;
> > +    unsigned int data_offset = 0;
> > +    uint32_t data;
> > +
> > +    ASSERT(pcidevs_locked());
> > +    ASSERT(vpci_locked(d));
> > +
> > +    /*
> > +     * Read the hardware value.
> > +     * NB: at the moment vPCI passthroughs everything (ie: permissive).
> 
> passes through
> 
> > +     */
> > +    data = vpci_read_hw(seg, bus, slot, func, reg, size);
> 
> I continue to be worried of reads that have side effects here. Granted
> we currently don't emulate any, but it would feel better if we didn't
> do the read for no reason. I.e. do hw reads only to fill gaps between
> emulated fields.

Heh, right. I got this "idea" from pciback, but I will change it so
the logic is similar to the write one (which obviously doesn't write
everything and then checks for emulated registers).

As a side question, which kind of registers have read side effects on
PCI? Reading the spec (PCIe 3.1A) there's no type of register listed
in section 7.4 (ro, rw, rw1c and the sticky versions) that mentions
read side effects. Is that described somewhere for specific
registers?

> > +    /* Find the PCI dev matching the address. *> +    /* Replace any 
> > values reported by the emulated registers. */
> > +    list_for_each_entry ( r, &pdev->vpci->handlers, node )
> > +    {
> > +        const struct vpci_register emu = {
> > +            .offset = reg + data_offset,
> > +            .size = size - data_offset
> > +        };
> > +        int cmp = vpci_register_cmp(&emu, r);
> > +        union vpci_val val = { .u32 = ~0 };
> > +        unsigned int merge_size;
> > +
> > +        if ( cmp < 0 )
> > +            break;
> > +        if ( cmp > 0 )
> > +            continue;
> > +
> > +        r->read(pdev, r->offset, &val, r->private);
> > +
> > +        /* Check if the read is in the middle of a register. */
> > +        if ( r->offset < emu.offset )
> > +            val.u32 >>= (emu.offset - r->offset) * 8;
> > +
> > +        data_offset = max(emu.offset, r->offset) - reg;
> > +        /* Find the intersection size between the two sets. */
> > +        merge_size = min(emu.offset + emu.size, r->offset + r->size) -
> > +                     max(emu.offset, r->offset);
> > +        /* Merge the emulated data into the native read value. */
> > +        data = merge_result(data, val.u32, merge_size, data_offset);
> > +        data_offset += merge_size;
> > +        if ( data_offset == size )
> > +            break;
> 
> ASSERT(data_offset < size) ?
> 
> > --- /dev/null
> > +++ b/xen/include/xen/vpci.h
> > @@ -0,0 +1,79 @@
> > +#ifndef _VPCI_
> > +#define _VPCI_
> > +
> > +#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).
> > + */
> > +#define vpci_lock(d) spin_lock_recursive(&(d)->arch.hvm_domain.vpci_lock)
> > +#define vpci_unlock(d) 
> > spin_unlock_recursive(&(d)->arch.hvm_domain.vpci_lock)
> > +#define vpci_locked(d) spin_is_locked(&(d)->arch.hvm_domain.vpci_lock)
> > +
> > +/* Value read or written by the handlers. */
> > +union vpci_val {
> > +    uint8_t u8;
> > +    uint16_t u16;
> > +    uint32_t u32;
> > +};
> 
> I continue to be unconvinced that this union is a good way to handle
> different sizes. Afaict Coverity (or similar tools) may recognize quite
> a few possible uses of uninitialized data. Quite likely all of them
> would be false positives, but anyway. Would it really be a big problem
> to uniformly pass uint32_t values around?

Hm, no I don't think so. I would then add explicit truncation of the
values in the read/write handlers.

> > +/*
> > + * The vPCI handlers will never be called concurrently for the same 
> > domain, ii
> > + * is guaranteed that the vpci domain lock will always be locked when 
> > calling
> > + * any handler.
> > + */
> > +typedef void (vpci_read_t)(struct pci_dev *pdev, unsigned int reg,
> > +                           union vpci_val *val, void *data);
> > +
> > +typedef void (vpci_write_t)(struct pci_dev *pdev, unsigned int reg,
> > +                            union vpci_val val, void *data);
> 
> Stray parentheses around the type name being defined.
> 
> > +typedef int (*vpci_register_init_t)(struct pci_dev *dev);
> 
> This one is inconsistent with the other two in that it defines a
> pointer type.
> 
> > +#define REGISTER_VPCI_INIT(x)                   \
> > +  static const vpci_register_init_t x##_entry   \
> > +               __used_section(".rodata.vpci") = x
> > +
> > +/* Add vPCI handlers to device. */
> > +int __must_check vpci_add_handlers(struct pci_dev *dev);
> > +
> > +/* Add/remove a register handler. */
> > +int __must_check vpci_add_register(const struct pci_dev *pdev,
> > +                                   vpci_read_t read_handler,
> > +                                   vpci_write_t write_handler,
> 
> I'm surprised this compiles without (at least) warnings - you appear to
> be lacking *s here.

I think in the previous version the type itself had a pointer, and
then I removed it and haven't updated it here. But yes, none of the
compilers seems to complain:

https://travis-ci.org/royger/xen/builds/248811315

Is it maybe implicit that function types are pointers?

> > +                                   unsigned int offset,
> > +                                   unsigned int size, void *data);
> > +int __must_check vpci_remove_register(const struct pci_dev *pdev,
> > +                                      unsigned int offset,
> > +                                      unsigned int size);
> > > +uint32_t vpci_read(unsigned int seg, unsigned int bus, unsigned int slot,
> > +                   unsigned int func, unsigned int reg, uint32_t size);
> > +void vpci_write(unsigned int seg, unsigned int bus, unsigned int slot,
> > +                unsigned int func, unsigned int reg, uint32_t size,
> > +                uint32_t data);
> 
> I don't see why size needs to be of a fixed width type in both of these.

unsigned int it is then.

> > +struct vpci {
> > +    /* Root pointer for the tree of vPCI handlers. */
> > +    struct list_head handlers;
> 
> The comment says "tree", but right now this really is just a list.

Ups, leftover from the previous RB version.

Thanks, Roger.

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