[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3] vPCI: account for hidden devices



On Tue, 30 May 2023, Jan Beulich wrote:
> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> console) are associated with DomXEN, not Dom0. This means that while
> looking for overlapping BARs such devices cannot be found on Dom0's list
> of devices; DomXEN's list also needs to be scanned.
> 
> Suppress vPCI init altogether for r/o devices (which constitute a subset
> of hidden ones).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Tested-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> v3: Also consider pdev being DomXEN's in modify_bars(). Also consult
>     DomXEN in vpci_{read,write}(). Move vpci_write()'s check of the r/o
>     map out of mainline code. Re-base over the standalone addition of
>     the loop continuation in modify_bars(), and finally make the code
>     change there well-formed.
> v2: Extend existing comment. Relax assertion. Don't initialize vPCI for
>     r/o devices.
> 
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
>      struct vpci_header *header = &pdev->vpci->header;
>      struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>      struct pci_dev *tmp, *dev = NULL;
> +    const struct domain *d;
>      const struct vpci_msix *msix = pdev->vpci->msix;
>      unsigned int i;
>      int rc;
> @@ -285,58 +286,69 @@ static int modify_bars(const struct pci_
>  
>      /*
>       * Check for overlaps with other BARs. Note that only BARs that are
> -     * currently mapped (enabled) are checked for overlaps.
> +     * currently mapped (enabled) are checked for overlaps. Note also that
> +     * for hwdom we also need to include hidden, i.e. DomXEN's, devices.
>       */
> -    for_each_pdev ( pdev->domain, tmp )
> +    for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain; ; )
>      {
> -        if ( !tmp->vpci )
> -            /*
> -             * For the hardware domain it's possible to have devices assigned
> -             * to it that are not handled by vPCI, either because those are
> -             * read-only devices, or because vPCI setup has failed.
> -             */
> -            continue;
> -
> -        if ( tmp == pdev )
> +        for_each_pdev ( d, tmp )
>          {
> -            /*
> -             * Need to store the device so it's not constified and defer_map
> -             * can modify it in case of error.
> -             */
> -            dev = tmp;
> -            if ( !rom_only )
> +            if ( !tmp->vpci )
>                  /*
> -                 * If memory decoding is toggled avoid checking against the
> -                 * same device, or else all regions will be removed from the
> -                 * memory map in the unmap case.
> +                 * For the hardware domain it's possible to have devices
> +                 * assigned to it that are not handled by vPCI, either 
> because
> +                 * those are read-only devices, or because vPCI setup has
> +                 * failed.
>                   */
>                  continue;
> -        }
>  
> -        for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
> -        {
> -            const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> -            unsigned long start = PFN_DOWN(bar->addr);
> -            unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> -
> -            if ( !bar->enabled || !rangeset_overlaps_range(mem, start, end) 
> ||
> -                 /*
> -                  * If only the ROM enable bit is toggled check against other
> -                  * BARs in the same device for overlaps, but not against the
> -                  * same ROM BAR.
> -                  */
> -                 (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> -                continue;
> +            if ( tmp == pdev )
> +            {
> +                /*
> +                 * Need to store the device so it's not constified and 
> defer_map
> +                 * can modify it in case of error.
> +                 */
> +                dev = tmp;
> +                if ( !rom_only )
> +                    /*
> +                     * If memory decoding is toggled avoid checking against 
> the
> +                     * same device, or else all regions will be removed from 
> the
> +                     * memory map in the unmap case.
> +                     */
> +                    continue;
> +            }
>  
> -            rc = rangeset_remove_range(mem, start, end);
> -            if ( rc )
> +            for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>              {
> -                printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: %d\n",
> -                       start, end, rc);
> -                rangeset_destroy(mem);
> -                return rc;
> +                const struct vpci_bar *bar = &tmp->vpci->header.bars[i];
> +                unsigned long start = PFN_DOWN(bar->addr);
> +                unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
> +
> +                if ( !bar->enabled ||
> +                     !rangeset_overlaps_range(mem, start, end) ||
> +                     /*
> +                      * If only the ROM enable bit is toggled check against
> +                      * other BARs in the same device for overlaps, but not
> +                      * against the same ROM BAR.
> +                      */
> +                     (rom_only && tmp == pdev && bar->type == VPCI_BAR_ROM) )
> +                    continue;
> +
> +                rc = rangeset_remove_range(mem, start, end);
> +                if ( rc )
> +                {
> +                    printk(XENLOG_G_WARNING "Failed to remove [%lx, %lx]: 
> %d\n",
> +                           start, end, rc);
> +                    rangeset_destroy(mem);
> +                    return rc;
> +                }
>              }
>          }
> +
> +        if ( !is_hardware_domain(d) )
> +            break;
> +
> +        d = dom_xen;
>      }
>  
>      ASSERT(dev);
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -70,6 +70,7 @@ void vpci_remove_device(struct pci_dev *
>  int vpci_add_handlers(struct pci_dev *pdev)
>  {
>      unsigned int i;
> +    const unsigned long *ro_map;
>      int rc = 0;
>  
>      if ( !has_vpci(pdev->domain) )
> @@ -78,6 +79,11 @@ int vpci_add_handlers(struct pci_dev *pd
>      /* We should not get here twice for the same device. */
>      ASSERT(!pdev->vpci);
>  
> +    /* No vPCI for r/o devices. */
> +    ro_map = pci_get_ro_map(pdev->sbdf.seg);
> +    if ( ro_map && test_bit(pdev->sbdf.bdf, ro_map) )
> +        return 0;
> +
>      pdev->vpci = xzalloc(struct vpci);
>      if ( !pdev->vpci )
>          return -ENOMEM;
> @@ -332,8 +338,13 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsi
>          return data;
>      }
>  
> -    /* Find the PCI dev matching the address. */
> +    /*
> +     * Find the PCI dev matching the address, which for hwdom also requires
> +     * consulting DomXEN.  Passthrough everything that's not trapped.
> +     */
>      pdev = pci_get_pdev(d, sbdf);
> +    if ( !pdev && is_hardware_domain(d) )
> +        pdev = pci_get_pdev(dom_xen, sbdf);
>      if ( !pdev || !pdev->vpci )
>          return vpci_read_hw(sbdf, reg, size);
>  
> @@ -427,7 +438,6 @@ void vpci_write(pci_sbdf_t sbdf, unsigne
>      const struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
> -    const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
>  
>      if ( !size )
>      {
> @@ -435,18 +445,20 @@ void vpci_write(pci_sbdf_t sbdf, unsigne
>          return;
>      }
>  
> -    if ( ro_map && test_bit(sbdf.bdf, ro_map) )
> -        /* Ignore writes to read-only devices. */
> -        return;
> -
>      /*
> -     * Find the PCI dev matching the address.
> -     * Passthrough everything that's not trapped.
> +     * Find the PCI dev matching the address, which for hwdom also requires
> +     * consulting DomXEN.  Passthrough everything that's not trapped.
>       */
>      pdev = pci_get_pdev(d, sbdf);
> +    if ( !pdev && is_hardware_domain(d) )
> +        pdev = pci_get_pdev(dom_xen, sbdf);
>      if ( !pdev || !pdev->vpci )
>      {
> -        vpci_write_hw(sbdf, reg, size, data);
> +        /* Ignore writes to read-only devices, which have no ->vpci. */
> +        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
> +
> +        if ( !ro_map || !test_bit(sbdf.bdf, ro_map) )
> +            vpci_write_hw(sbdf, reg, size, data);
>          return;
>      }
>  
> 



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.