>>> On 18.10.17 at 13:40, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
