Re: [Xen-devel] [PATCH v3 6/9] xen/vpci: trap access to the list of PCI capabilities

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> wrote:
> Add traps to each capability PCI_CAP_LIST_NEXT field in order to mask them on
> request.
> 
> All capabilities from the device are fetched and stored in an internal list,
> that's later used in order to return the next capability to the guest. Note
> that this only removes the capability from the linked list as seen by the
> guest, but the actual capability structure could still be accessed by the
> guest, provided that it's position can be found using another mechanism.

Which is a problem. Drivers tied to a single device or a narrow set
aren't unknown to do such. In fact in the past Intel has given us
workaround outlines for some of their chipset issues which directed
us to fixed offsets instead of using the capability chains.

> Finally the MSI and MSI-X capabilities are masked until Xen knows how to
> properly handle accesses to them.
> 
> This should allow a PVH Dom0 to boot on some hardware, provided that the
> hardware doesn't require MSI/MSI-X and that there are no SR-IOV devices in the
> system, so the panic at the end of the PVH Dom0 build is replaced by a
> warning.

While this is certainly nice for development / debugging purposes,
what's the longer term intention with the functionality being added
here? We had no need to mask capabilities for PV Dom0, so I would
have hoped to get away without for PVH too.

Assuming there is a reason other than to temporarily hide MSI/MSI-X,
I'll give some comments on the patch itself anyway.

> --- /dev/null
> +++ b/xen/drivers/vpci/capabilities.c
> @@ -0,0 +1,159 @@
> +/*
> + * Generic functionality for handling accesses to the PCI capabilities 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>
> +
> +struct vpci_capability {
> +    struct list_head next;
> +    uint8_t offset;
> +    bool masked;

I think I'd prefer "hidden" or "suppressed".

> +};
> +
> +static int vpci_cap_read(struct pci_dev *pdev, unsigned int reg,
> +                         union vpci_val *val, void *data)
> +{
> +    struct vpci_capability *cap = data;
> +
> +    val->half_word = 0;

Instead of doing such (and, like here, leaving part of what val
points to uninitialized), wouldn't is be better to do this in the code
calling these helpers?

> +static int vpci_cap_write(struct pci_dev *pdev, unsigned int reg,
> +                          union vpci_val val, void *data)
> +{
> +    /* Ignored. */
> +    return 0;
> +}

One possible example of what a NULL write handler might mean.

> +static int vpci_index_capabilities(struct pci_dev *pdev)
> +{
> +    uint8_t seg = pdev->seg, bus = pdev->bus;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    uint8_t pos = PCI_CAPABILITY_LIST;
> +    uint16_t status;
> +    unsigned int max_cap = 48;

I think it's high time to introduce a #define for this, which is now
at least the 3rd instance.

> +    struct vpci_capability *cap;
> +    int rc;
> +
> +    INIT_LIST_HEAD(&pdev->vpci->cap_list);
> +
> +    /* Check if device has capabilities. */
> +    status = pci_conf_read16(seg, bus, slot, func, PCI_STATUS);
> +    if ( !(status & PCI_STATUS_CAP_LIST) )
> +        return 0;
> +
> +    /* Add the root capability pointer. */
> +    cap = xzalloc(struct vpci_capability);
> +    if ( !cap )
> +        return -ENOMEM;
> +
> +    cap->offset = pos;

Please be consistent with the naming of field and variable.

> +    list_add_tail(&cap->next, &pdev->vpci->cap_list);
> +    rc = xen_vpci_add_register(pdev, vpci_cap_read, vpci_cap_write, pos,
> +                               1, cap);
> +    if ( rc )
> +        return rc;
> +
> +    /*
> +     * Iterate over the list of capabilities present in the device, and
> +     * add a handler for each register pointer to the next item
> +     * (PCI_CAP_LIST_NEXT).
> +     */
> +    while ( max_cap-- )
> +    {
> +        pos = pci_conf_read8(seg, bus, slot, func, pos);
> +        if ( pos < 0x40 )
> +            break;
> +
> +        cap = xzalloc(struct vpci_capability);
> +        if ( !cap )
> +            return -ENOMEM;
> +
> +        cap->offset = pos;

Pre-existing code clears the low two bits from the value read and
also checks whether the capability ID is 0xff.

> +static int vpci_capabilities_init(struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    rc = vpci_index_capabilities(pdev);
> +    if ( rc )
> +        return rc;
> +
> +    /* Mask MSI and MSI-X capabilities until Xen handles them. */
> +    vpci_mask_capability(pdev, PCI_CAP_ID_MSI);
> +    vpci_mask_capability(pdev, PCI_CAP_ID_MSIX);
> +
> +    return 0;
> +}

What about extended capabilities?

Jan

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