Re: [Xen-devel] [PATCH v7 for-next 09/12] vpci/bars: add handlers to map the BARs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.10.17 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/xen/drivers/vpci/header.c
> @@ -0,0 +1,518 @@
> +/*
> + * Generic functionality for handling accesses to the PCI header from the
> + * configuration space.
> + *
> + * 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>
> +#include <xen/p2m-common.h>
> +#include <xen/softirq.h>

Unless there's a reason for this ordering, please sort #include-s in
new files.

> +static void modify_decoding(const struct pci_dev *pdev, bool map, bool rom)
> +{
> +    struct vpci_header *header = &pdev->vpci->header;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    unsigned int i;
> +
> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +    {
> +        if ( rom && header->bars[i].type == VPCI_BAR_ROM )
> +        {
> +            unsigned int rom_pos = (i == 6) ? PCI_ROM_ADDRESS
> +                                            : PCI_ROM_ADDRESS1;
> +            uint32_t val = pci_conf_read32(pdev->seg, pdev->bus, slot, func,
> +                                           rom_pos);
> +
> +            header->bars[i].enabled = header->bars[i].rom_enabled = map;
> +
> +            val &= ~PCI_ROM_ADDRESS_ENABLE;
> +            val |= map ? PCI_ROM_ADDRESS_ENABLE : 0;
> +            pci_conf_write32(pdev->seg, pdev->bus, slot, func, rom_pos, val);
> +            break;
> +        }
> +        if ( !rom && (header->bars[i].type != VPCI_BAR_ROM ||
> +                      header->bars[i].rom_enabled) )
> +            header->bars[i].enabled = map;
> +    }

Looking at all of this, it would clearly be more logical for
rom_enabled to be a per-header instead of a per-BAR flag.

> +    if ( !rom )
> +    {
> +        uint16_t cmd = pci_conf_read16(pdev->seg, pdev->bus, slot,
> +                                       func, PCI_COMMAND);
> +
> +        cmd &= ~PCI_COMMAND_MEMORY;
> +        cmd |= map ? PCI_COMMAND_MEMORY : 0;
> +        pci_conf_write16(pdev->seg, pdev->bus, slot, func, PCI_COMMAND,
> +                         cmd);
> +    }

For both writes, wouldn't it be worthwhile to avoid them when the
flag is already in the intended state?

> +bool vpci_process_pending(struct vcpu *v)
> +{
> +    while ( v->vpci.mem )
> +    {
> +        struct map_data data = {
> +            .d = v->domain,
> +            .map = v->vpci.map,
> +        };
> +
> +        switch ( rangeset_consume_ranges(v->vpci.mem, map_range, &data) )
> +        {
> +        case -ERESTART:
> +            return true;
> +        case 0:

Blank line between non-fall-through case blocks please.

> +            if ( v->vpci.map )
> +            {
> +                spin_lock(&v->vpci.pdev->vpci->lock);
> +                modify_decoding(v->vpci.pdev, v->vpci.map, v->vpci.rom);
> +                spin_unlock(&v->vpci.pdev->vpci->lock);
> +            }
> +            /* fallthrough. */
> +        case -ENOMEM:
> +            /*
> +             * Other errors are ignored, hopping that at least some regions

hoping

I also don't really understand your intentions here: If other errors
are being ignored, wouldn't that lead to the rangeset being leaked?
Or is "other" here meant to include -ENOMEM, in which case you
really mean "default:" above?

> +static void modify_bars(const struct pci_dev *pdev, bool map, bool rom)
> +{
> +    struct vpci_header *header = &pdev->vpci->header;
> +    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> +    const struct pci_dev *tmp;
> +    unsigned int i;
> +    int rc;
> +
> +    if ( !map )
> +        modify_decoding(pdev, false, rom);
> +
> +    if ( !mem )
> +        return;
> +
> +    /*
> +     * Create a rangeset that represents the current device BARs memory 
> region
> +     * and compare it against all the currently active BAR memory regions. If
> +     * an overlap is found, subtract it from the region to be
> +     * mapped/unmapped.
> +     *
> +     * NB: the rangeset uses inclusive frame numbers.
> +     */
> +
> +    /*
> +     * First fill the rangeset with all the BARs of this device or with the 
> ROM
> +     * BAR only, depending on whether the guest is toggling the memory decode
> +     * bit of the command register, or the enable bit of the ROM BAR 
> register.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +    {
> +        const struct vpci_bar *bar = &header->bars[i];
> +
> +        if ( !MAPPABLE_BAR(bar) ||
> +             rom ? bar->type != VPCI_BAR_ROM
> +                 : (bar->type == VPCI_BAR_ROM && !bar->rom_enabled) )
> +            continue;

Logically as well as from the indentation used I think this is again
lacking parentheses around the conditional expression.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -102,6 +102,21 @@ static void vpci_ignored_write(const struct pci_dev 
> *pdev, unsigned int reg,
>  {
>  }
>  
> +uint32_t vpci_hw_read16(const struct pci_dev *pdev, unsigned int reg,
> +                        void *data)
> +{
> +    return pci_conf_read16(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                           PCI_FUNC(pdev->devfn), reg);
> +}
> +
> +uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
> +                        void *data)
> +{
> +    return pci_conf_read32(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> +                           PCI_FUNC(pdev->devfn), reg);
> +}
> +
> +
>  int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,

No double blank lines please.

> @@ -264,6 +265,11 @@ struct vcpu
>  
>      struct evtchn_fifo_vcpu *evtchn_fifo;
>  
> +#ifdef CONFIG_HAS_PCI
> +    /* vPCI per-vCPU area, used to store data for long running operations. */
> +    struct vpci_vcpu vpci;
> +#endif

Wasn't the #ifdef here supposed to be dropped with the dummy
structure you now have?

>  struct vpci {
>      /* List of vPCI handlers for a device. */
>      struct list_head handlers;
>      spinlock_t lock;
> +
> +#ifdef __XEN__
> +    /* Hide the rest of the vpci struct from the user-space test harness. */
> +    struct vpci_header {
> +        /* Information about the PCI BARs of this device. */
> +        struct vpci_bar {
> +            uint64_t addr;
> +            uint64_t size;
> +            enum {
> +                VPCI_BAR_EMPTY,
> +                VPCI_BAR_IO,
> +                VPCI_BAR_MEM32,
> +                VPCI_BAR_MEM64_LO,
> +                VPCI_BAR_MEM64_HI,
> +                VPCI_BAR_ROM,
> +            } type;
> +            bool prefetchable : 1;
> +            /* Store whether the BAR is mapped into guest p2m. */
> +            bool enabled      : 1;
> +            /*
> +             * Store whether the ROM enable bit is set (doesn't imply ROM BAR
> +             * is mapped into guest p2m). Only used for type VPCI_BAR_ROM.
> +             */
> +            bool rom_enabled  : 1;
> +        } bars[7]; /* At most 6 BARS + 1 expansion ROM BAR. */
> +        /* FIXME: currently there's no support for SR-IOV. */
> +    } header;
> +#endif
>  };
>  
> +#ifdef __XEN__
> +struct vpci_vcpu {
> +    struct rangeset *mem;
> +    const struct pci_dev *pdev;
> +    bool map : 1;
> +    bool rom : 1;
> +};
> +#endif

This structure could do with a comment briefly noting it purpose.
Also - if the #ifdef really needed here?

Jan

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