Re: [Xen-devel] [PATCH v5 08/11] vpci/bars: add handlers to map the BARs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> +static int vpci_modify_bar(struct domain *d, const struct vpci_bar *bar,
> +                           bool map)
> +{
> +    struct rangeset *mem;
> +    struct map_data data = { .d = d, .map = map };
> +    int rc;
> +
> +    ASSERT(MAPPABLE_BAR(bar));
> +
> +    mem = vpci_get_bar_memory(d, bar);
> +    if ( IS_ERR(mem) )
> +        return PTR_ERR(mem);
> +
> +    rc = rangeset_report_ranges(mem, 0, ~0ul, vpci_map_range, &data);
> +    rangeset_destroy(mem);
> +    if ( rc )
> +        return rc;
> +
> +    return 0;
> +}

Please make this simply "return rc".

> +static int vpci_modify_bars(const struct pci_dev *pdev, const bool map)

We don't normally constify non-pointing-to types, and even less so
in function parameters.

> +static uint32_t vpci_cmd_read(struct pci_dev *pdev, unsigned int reg,
> +                              const void *data)
> +{
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +
> +    return pci_conf_read16(seg, bus, slot, func, reg);
> +}

I can see why you may want to have the slot and func local
variables, but at least seg and bus look pretty pointless to have
here.

> +static void vpci_cmd_write(struct pci_dev *pdev, unsigned int reg,
> +                           uint32_t cmd, void *data)
> +{
> +    uint16_t current_cmd;
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +
> +    current_cmd = pci_conf_read16(seg, bus, slot, func, reg);

Would you mind making this the initializer of the variable?

> +    /*
> +     * Let the guest play with all the bits directly except for the
> +     * memory decoding one.
> +     */
> +    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> +    {
> +        /* Memory space access change. */
> +        int rc = vpci_modify_bars(pdev, cmd & PCI_COMMAND_MEMORY);
> +
> +        if ( rc )
> +        {
> +            gdprintk(XENLOG_ERR,
> +                     "%04x:%02x:%02x.%u:unable to %smap BARs: %d\n",
> +                     seg, bus, slot, func,
> +                     cmd & PCI_COMMAND_MEMORY ? "" : "un", rc);

Perhaps gprintk(), and perhaps XENLOG_WARNING (depending on
the device it may not be that big of a problem)?

> +            return;

I think you should not bail here when it is an unmap that failed -
you'd better disable memory decode in that case.

> +static uint32_t vpci_bar_read(struct pci_dev *pdev, unsigned int reg,
> +                              const void *data)
> +{
> +    const struct vpci_bar *bar = data;
> +    uint32_t val;
> +    bool hi = false;
> +
> +    ASSERT(bar->type == VPCI_BAR_MEM32 || bar->type == VPCI_BAR_MEM64_LO ||
> +           bar->type == VPCI_BAR_MEM64_HI);
> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +    }
> +
> +    if ( bar->sizing )
> +        val = ~(bar->size - 1) >> (hi ? 32 : 0);

I'm not going to insist on it, but "-bar->size" is certainly easier to
read, and may also produce shorter code (unless the compiler
does the transformation itself anyway).

> +static void vpci_bar_write(struct pci_dev *pdev, unsigned int reg,
> +                           uint32_t val, void *data)
> +{
> +    struct vpci_bar *bar = data;
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    bool hi = false;
> +
> +    if ( pci_conf_read16(seg, bus, slot, func, PCI_COMMAND) &
> +         PCI_COMMAND_MEMORY )
> +    {
> +         gdprintk(XENLOG_WARNING,
> +                  "%04x:%02x:%02x.%u: ignored BAR write with memory decoding 
> enabled\n",
> +                  seg, bus, slot, func);

I'd again suggest gprintk().

> +        return;
> +    }
> +
> +    if ( bar->type == VPCI_BAR_MEM64_HI )
> +    {
> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
> +        bar--;
> +        hi = true;
> +    }
> +
> +    if ( !hi )
> +        val &= PCI_BASE_ADDRESS_MEM_MASK;

This could be the else to the earlier if().

> +
> +    /*
> +     * The PCI Local Bus Specification suggests writing ~0 to both the high
> +     * and the low part of the BAR registers before attempting to read back
> +     * the size.
> +     *
> +     * However real device BARs registers (at least the ones I've tried)
> +     * will return the size of the BAR just by having written ~0 to one half
> +     * of it, independently of the value of the other half of the register.
> +     * Hence here Xen will switch to returning the size as soon as one half
> +     * of the BAR register has been written with ~0.
> +     */

I don't believe this is correct behavior (but I'd have to play with
some hardware to see whether I can confirm the behavior you
describe): How would you place a BAR at, say, 0x1ffffff0?

> +    if ( val == (hi ? 0xffffffff : (uint32_t)PCI_BASE_ADDRESS_MEM_MASK) )
> +    {
> +        bar->sizing = true;
> +        return;
> +    }
> +    bar->sizing = false;
> +
> +    /* Update the relevant part of the BAR address. */
> +    bar->addr &= ~(0xffffffffull << (hi ? 32 : 0));
> +    bar->addr |= (uint64_t)val << (hi ? 32 : 0);
> +
> +    /* Make sure Xen writes back the same value for the BAR RO bits. */
> +    if ( !hi )
> +        val |= pci_conf_read32(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                               PCI_FUNC(pdev->devfn), reg) &
> +                               ~PCI_BASE_ADDRESS_MEM_MASK;

Why don't you break out the logic from vpci_bar_read() into a
helper and use it here too, saving the (slow) config space access.

> +static void vpci_rom_write(struct pci_dev *pdev, unsigned int reg,
> +                           uint32_t val, void *data)
> +{
> +    struct vpci_bar *rom = data;
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    uint16_t cmd = pci_conf_read16(seg, bus, slot, func, PCI_COMMAND);
> +    uint32_t addr = val & PCI_ROM_ADDRESS_MASK;
> +
> +    if ( addr == (uint32_t)PCI_ROM_ADDRESS_MASK )
> +    {
> +        rom->sizing = true;
> +        return;
> +    }
> +    rom->sizing = false;
> +
> +    rom->addr = addr;
> +
> +    /* Check if ROM BAR should be mapped. */
> +    if ( (cmd & PCI_COMMAND_MEMORY) &&

Why don't you disallow writes here when memory decoding is enabled
and the ROM BAR is enabled the same way (including a log message)
you do in vpci_bar_write()?

> +static int vpci_init_bars(struct pci_dev *pdev)
> +{
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    uint16_t cmd;
> +    uint64_t addr, size;
> +    unsigned int i, num_bars, rom_reg;
> +    struct vpci_header *header = &pdev->vpci->header;
> +    struct vpci_bar *bars = header->bars;
> +    int rc;
> +
> +    switch ( pci_conf_read8(seg, bus, slot, func, PCI_HEADER_TYPE) & 0x7f )
> +    {
> +    case PCI_HEADER_TYPE_NORMAL:
> +        num_bars = 6;
> +        rom_reg = PCI_ROM_ADDRESS;
> +        break;
> +    case PCI_HEADER_TYPE_BRIDGE:
> +        num_bars = 2;
> +        rom_reg = PCI_ROM_ADDRESS1;
> +        break;
> +    default:
> +        return -EOPNOTSUPP;
> +    }
> +
> +    /* Setup a handler for the command register. */
> +    rc = vpci_add_register(pdev, vpci_cmd_read, vpci_cmd_write, PCI_COMMAND,
> +                           2, header);
> +    if ( rc )
> +        return rc;
> +
> +    /* Disable memory decoding before sizing. */
> +    cmd = pci_conf_read16(seg, bus, slot, func, PCI_COMMAND);
> +    if ( cmd & PCI_COMMAND_MEMORY )
> +        pci_conf_write16(seg, bus, slot, func, PCI_COMMAND,
> +                         cmd & ~PCI_COMMAND_MEMORY);
> +
> +    for ( i = 0; i < num_bars; i++ )
> +    {
> +        uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
> +        uint32_t val = pci_conf_read32(seg, bus, slot, func, reg);
> +
> +        if ( i && bars[i - 1].type == VPCI_BAR_MEM64_LO )
> +        {
> +            bars[i].type = VPCI_BAR_MEM64_HI;
> +            rc = vpci_add_register(pdev, vpci_bar_read, vpci_bar_write, reg, 
> 4,
> +                                   &bars[i]);
> +            if ( rc )
> +            {
> +                pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +                return rc;
> +            }
> +
> +            continue;
> +        }
> +        if ( (val & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO )
> +        {
> +            bars[i].type = VPCI_BAR_IO;
> +            continue;
> +        }
> +        if ( (val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> +             PCI_BASE_ADDRESS_MEM_TYPE_64 )
> +            bars[i].type = VPCI_BAR_MEM64_LO;
> +        else
> +            bars[i].type = VPCI_BAR_MEM32;
> +
> +        /* Size the BAR and map it. */
> +        rc = pci_size_mem_bar(seg, bus, slot, func, reg, i == num_bars - 1,
> +                              &addr, &size, false, false);
> +        if ( rc < 0 )
> +        {
> +            pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +            return rc;
> +        }
> +
> +        if ( size == 0 )
> +        {
> +            bars[i].type = VPCI_BAR_EMPTY;
> +            continue;
> +        }
> +
> +        bars[i].addr = addr;
> +        bars[i].size = size;
> +        bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
> +
> +        rc = vpci_add_register(pdev, vpci_bar_read, vpci_bar_write, reg, 4,
> +                               &bars[i]);
> +        if ( rc )
> +        {
> +            pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +            return rc;
> +        }
> +    }
> +
> +    /* Check expansion ROM. */
> +    rc = pci_size_mem_bar(seg, bus, slot, func, rom_reg, true, &addr, &size,
> +                          false, true);
> +    if ( rc < 0 )
> +    {
> +        pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +        return rc;
> +    }
> +
> +    if ( size )
> +    {
> +        struct vpci_bar *rom = &header->bars[num_bars];
> +
> +        rom->type = VPCI_BAR_ROM;
> +        rom->size = size;
> +        rom->addr = addr;
> +
> +        rc = vpci_add_register(pdev, vpci_rom_read, vpci_rom_write, rom_reg, 
> 4,
> +                               rom);
> +        if ( rc )
> +        {
> +            pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +            return rc;
> +        }
> +    }
> +
> +    if ( cmd & PCI_COMMAND_MEMORY )
> +    {
> +        rc = vpci_modify_bars(pdev, true);
> +        if ( rc )
> +        {
> +            pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);
> +            return rc;
> +        }
> +
> +        /* Enable memory decoding. */
> +        pci_conf_write16(seg, bus, slot, func, PCI_COMMAND, cmd);

No need for two pci_conf_write16() invocations here - you can
simply arrange to ...

> +    }
> +
> +    return 0;
> +}

... "return rc" here.

> +REGISTER_VPCI_INIT(vpci_init_bars);

Considering that ultimately an error returned from this function will
lead to nothing but a log message, I wonder whether you wouldn't
better do best effort here, i.e. only record the first error, but
continue rather than bailing. Especially for the ROM it may well be
that it's not really needed for proper operation of the device.

Jan

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