thr3ads.net - Xen devel - RE: [Xen-devel] RE: Resend: RE: enable_ats

If this information is useful, please help other people find it:
Share via:

Jan Beulich

2011-Sep-21 07:59 UTC

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site

>>> On 20.09.11 at 20:02, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
> I agree that the same problem exists for bus-to-bridge mapping function 
> pci_scan_device().  By the way, we probably should take the opportunity to 
> give it a proper name that reflects to the true purpose of this function.
How about the below (applying only on top of the multi-seg patches,
and not yet removing the scanning functions)?

Jan

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -129,11 +129,67 @@ static struct pci_dev *alloc_pdev(struct
     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
     spin_lock_init(&pdev->msix_table_lock);
 
+    /* update bus2bridge */
+    switch ( pdev_type(pseg->nr, bus, devfn) )
+    {
+        u8 sec_bus, sub_bus;
+
+        case DEV_TYPE_PCIe_BRIDGE:
+            break;
+
+        case DEV_TYPE_PCIe2PCI_BRIDGE:
+        case DEV_TYPE_LEGACY_PCI_BRIDGE:
+            sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
+                                     PCI_FUNC(devfn), PCI_SECONDARY_BUS);
+            sub_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
+                                     PCI_FUNC(devfn), PCI_SUBORDINATE_BUS);
+
+            spin_lock(&pseg->bus2bridge_lock);
+            for ( ; sec_bus <= sub_bus; sec_bus++ )
+            {
+                pseg->bus2bridge[sec_bus].map = 1;
+                pseg->bus2bridge[sec_bus].bus = bus;
+                pseg->bus2bridge[sec_bus].devfn = devfn;
+            }
+            spin_unlock(&pseg->bus2bridge_lock);
+            break;
+
+        case DEV_TYPE_PCIe_ENDPOINT:
+        case DEV_TYPE_PCI:
+            break;
+
+        default:
+            printk(XENLOG_WARNING "%s: unknown type:
%04x:%02x:%02x.%u\n",
+                   __func__, pseg->nr, bus, PCI_SLOT(devfn),
PCI_FUNC(devfn));
+            break;
+    }
+
     return pdev;
 }
 
-static void free_pdev(struct pci_dev *pdev)
+static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
 {
+    /* update bus2bridge */
+    switch ( pdev_type(pseg->nr, pdev->bus, pdev->devfn) )
+    {
+        u8 dev, func, sec_bus, sub_bus;
+
+        case DEV_TYPE_PCIe2PCI_BRIDGE:
+        case DEV_TYPE_LEGACY_PCI_BRIDGE:
+            dev = PCI_SLOT(pdev->devfn);
+            func = PCI_FUNC(pdev->devfn);
+            sec_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
+                                     PCI_SECONDARY_BUS);
+            sub_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
+                                     PCI_SUBORDINATE_BUS);
+
+            spin_lock(&pseg->bus2bridge_lock);
+            for ( ; sec_bus <= sub_bus; sec_bus++ )
+                pseg->bus2bridge[sec_bus].map = 0;
+            spin_unlock(&pseg->bus2bridge_lock);
+            break;
+    }
+
     list_del(&pdev->alldevs_list);
     xfree(pdev);
 }
@@ -378,7 +434,7 @@ int pci_remove_device(u16 seg, u8 bus, u
             if ( pdev->domain )
                 list_del(&pdev->domain_list);
             pci_cleanup_msi(pdev);
-            free_pdev(pdev);
+            free_pdev(pseg, pdev);
             printk(XENLOG_DEBUG "PCI remove device
%04x:%02x:%02x.%u\n",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
             break;
@@ -546,8 +602,6 @@ static int __init _scan_pci_devices(stru
 {
     struct pci_dev *pdev;
     int bus, dev, func;
-    u8 sec_bus, sub_bus;
-    int type;
 
     for ( bus = 0; bus < 256; bus++ )
     {
@@ -565,41 +619,6 @@ static int __init _scan_pci_devices(stru
                     return -ENOMEM;
                 }
 
-                /* build bus2bridge */
-                type = pdev_type(pseg->nr, bus, PCI_DEVFN(dev, func));
-                switch ( type )
-                {
-                    case DEV_TYPE_PCIe_BRIDGE:
-                        break;
-
-                    case DEV_TYPE_PCIe2PCI_BRIDGE:
-                    case DEV_TYPE_LEGACY_PCI_BRIDGE:
-                        sec_bus = pci_conf_read8(pseg->nr, bus, dev, func,
-                                                 PCI_SECONDARY_BUS);
-                        sub_bus = pci_conf_read8(pseg->nr, bus, dev, func,
-                                                 PCI_SUBORDINATE_BUS);
-
-                        spin_lock(&pseg->bus2bridge_lock);
-                        for ( sub_bus &= 0xff; sec_bus <= sub_bus;
sec_bus++ )
-                        {
-                            pseg->bus2bridge[sec_bus].map = 1;
-                            pseg->bus2bridge[sec_bus].bus = bus;
-                            pseg->bus2bridge[sec_bus].devfn -               
PCI_DEVFN(dev, func);
-                        }
-                        spin_unlock(&pseg->bus2bridge_lock);
-                        break;
-
-                    case DEV_TYPE_PCIe_ENDPOINT:
-                    case DEV_TYPE_PCI:
-                        break;
-
-                    default:
-                        printk("%s: unknown type:
%04x:%02x:%02x.%u\n",
-                               __func__, pseg->nr, bus, dev, func);
-                        return -EINVAL;
-                }
-
                 if ( !func && !(pci_conf_read8(pseg->nr, bus, dev,
func,
                                                PCI_HEADER_TYPE) & 0x80) )
                     break;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Kay, Allen M

2011-Sep-21 20:32 UTC

head link

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site

Looks good to me.

Allen

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Wednesday, September 21, 2011 12:59 AM
To: Kay, Allen M
Cc: Xen Devel
Subject: RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site
>>> On 20.09.11 at 20:02, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
> I agree that the same problem exists for bus-to-bridge mapping function 
> pci_scan_device().  By the way, we probably should take the opportunity to 
> give it a proper name that reflects to the true purpose of this function.
How about the below (applying only on top of the multi-seg patches,
and not yet removing the scanning functions)?

Jan

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -129,11 +129,67 @@ static struct pci_dev *alloc_pdev(struct
     list_add(&pdev->alldevs_list, &pseg->alldevs_list);
     spin_lock_init(&pdev->msix_table_lock);
 
+    /* update bus2bridge */
+    switch ( pdev_type(pseg->nr, bus, devfn) )
+    {
+        u8 sec_bus, sub_bus;
+
+        case DEV_TYPE_PCIe_BRIDGE:
+            break;
+
+        case DEV_TYPE_PCIe2PCI_BRIDGE:
+        case DEV_TYPE_LEGACY_PCI_BRIDGE:
+            sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
+                                     PCI_FUNC(devfn), PCI_SECONDARY_BUS);
+            sub_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
+                                     PCI_FUNC(devfn), PCI_SUBORDINATE_BUS);
+
+            spin_lock(&pseg->bus2bridge_lock);
+            for ( ; sec_bus <= sub_bus; sec_bus++ )
+            {
+                pseg->bus2bridge[sec_bus].map = 1;
+                pseg->bus2bridge[sec_bus].bus = bus;
+                pseg->bus2bridge[sec_bus].devfn = devfn;
+            }
+            spin_unlock(&pseg->bus2bridge_lock);
+            break;
+
+        case DEV_TYPE_PCIe_ENDPOINT:
+        case DEV_TYPE_PCI:
+            break;
+
+        default:
+            printk(XENLOG_WARNING "%s: unknown type:
%04x:%02x:%02x.%u\n",
+                   __func__, pseg->nr, bus, PCI_SLOT(devfn),
PCI_FUNC(devfn));
+            break;
+    }
+
     return pdev;
 }
 
-static void free_pdev(struct pci_dev *pdev)
+static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
 {
+    /* update bus2bridge */
+    switch ( pdev_type(pseg->nr, pdev->bus, pdev->devfn) )
+    {
+        u8 dev, func, sec_bus, sub_bus;
+
+        case DEV_TYPE_PCIe2PCI_BRIDGE:
+        case DEV_TYPE_LEGACY_PCI_BRIDGE:
+            dev = PCI_SLOT(pdev->devfn);
+            func = PCI_FUNC(pdev->devfn);
+            sec_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
+                                     PCI_SECONDARY_BUS);
+            sub_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
+                                     PCI_SUBORDINATE_BUS);
+
+            spin_lock(&pseg->bus2bridge_lock);
+            for ( ; sec_bus <= sub_bus; sec_bus++ )
+                pseg->bus2bridge[sec_bus].map = 0;
+            spin_unlock(&pseg->bus2bridge_lock);
+            break;
+    }
+
     list_del(&pdev->alldevs_list);
     xfree(pdev);
 }
@@ -378,7 +434,7 @@ int pci_remove_device(u16 seg, u8 bus, u
             if ( pdev->domain )
                 list_del(&pdev->domain_list);
             pci_cleanup_msi(pdev);
-            free_pdev(pdev);
+            free_pdev(pseg, pdev);
             printk(XENLOG_DEBUG "PCI remove device
%04x:%02x:%02x.%u\n",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
             break;
@@ -546,8 +602,6 @@ static int __init _scan_pci_devices(stru
 {
     struct pci_dev *pdev;
     int bus, dev, func;
-    u8 sec_bus, sub_bus;
-    int type;
 
     for ( bus = 0; bus < 256; bus++ )
     {
@@ -565,41 +619,6 @@ static int __init _scan_pci_devices(stru
                     return -ENOMEM;
                 }
 
-                /* build bus2bridge */
-                type = pdev_type(pseg->nr, bus, PCI_DEVFN(dev, func));
-                switch ( type )
-                {
-                    case DEV_TYPE_PCIe_BRIDGE:
-                        break;
-
-                    case DEV_TYPE_PCIe2PCI_BRIDGE:
-                    case DEV_TYPE_LEGACY_PCI_BRIDGE:
-                        sec_bus = pci_conf_read8(pseg->nr, bus, dev, func,
-                                                 PCI_SECONDARY_BUS);
-                        sub_bus = pci_conf_read8(pseg->nr, bus, dev, func,
-                                                 PCI_SUBORDINATE_BUS);
-
-                        spin_lock(&pseg->bus2bridge_lock);
-                        for ( sub_bus &= 0xff; sec_bus <= sub_bus;
sec_bus++ )
-                        {
-                            pseg->bus2bridge[sec_bus].map = 1;
-                            pseg->bus2bridge[sec_bus].bus = bus;
-                            pseg->bus2bridge[sec_bus].devfn -               
PCI_DEVFN(dev, func);
-                        }
-                        spin_unlock(&pseg->bus2bridge_lock);
-                        break;
-
-                    case DEV_TYPE_PCIe_ENDPOINT:
-                    case DEV_TYPE_PCI:
-                        break;
-
-                    default:
-                        printk("%s: unknown type:
%04x:%02x:%02x.%u\n",
-                               __func__, pseg->nr, bus, dev, func);
-                        return -EINVAL;
-                }
-
                 if ( !func && !(pci_conf_read8(pseg->nr, bus, dev,
func,
                                                PCI_HEADER_TYPE) & 0x80) )
                     break;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Sep-22 08:40 UTC

head link

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site

>>> On 21.09.11 at 22:32, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
> Looks good to me.
On a second thought I''m not sure about the free_pdev() part anymore:
When having a bridge behind a bridge, and the downstream one gets
removed, wouldn''t that mean the upstream one would now be covering
the bus range that got orphaned? Which means rather than flushing
.map we''d have to copy our parent''s entry.

Jan
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com] 
> Sent: Wednesday, September 21, 2011 12:59 AM
> To: Kay, Allen M
> Cc: Xen Devel
> Subject: RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site
> 
>>>> On 20.09.11 at 20:02, "Kay, Allen M"
<allen.m.kay@intel.com> wrote:
>> I agree that the same problem exists for bus-to-bridge mapping function
>> pci_scan_device().  By the way, we probably should take the opportunity
to
>> give it a proper name that reflects to the true purpose of this
function.
> 
> How about the below (applying only on top of the multi-seg patches,
> and not yet removing the scanning functions)?
> 
> Jan
> 
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -129,11 +129,67 @@ static struct pci_dev *alloc_pdev(struct
>      list_add(&pdev->alldevs_list, &pseg->alldevs_list);
>      spin_lock_init(&pdev->msix_table_lock);
>  
> +    /* update bus2bridge */
> +    switch ( pdev_type(pseg->nr, bus, devfn) )
> +    {
> +        u8 sec_bus, sub_bus;
> +
> +        case DEV_TYPE_PCIe_BRIDGE:
> +            break;
> +
> +        case DEV_TYPE_PCIe2PCI_BRIDGE:
> +        case DEV_TYPE_LEGACY_PCI_BRIDGE:
> +            sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
> +                                     PCI_FUNC(devfn), PCI_SECONDARY_BUS);
> +            sub_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
> +                                     PCI_FUNC(devfn),
PCI_SUBORDINATE_BUS);
> +
> +            spin_lock(&pseg->bus2bridge_lock);
> +            for ( ; sec_bus <= sub_bus; sec_bus++ )
> +            {
> +                pseg->bus2bridge[sec_bus].map = 1;
> +                pseg->bus2bridge[sec_bus].bus = bus;
> +                pseg->bus2bridge[sec_bus].devfn = devfn;
> +            }
> +            spin_unlock(&pseg->bus2bridge_lock);
> +            break;
> +
> +        case DEV_TYPE_PCIe_ENDPOINT:
> +        case DEV_TYPE_PCI:
> +            break;
> +
> +        default:
> +            printk(XENLOG_WARNING "%s: unknown type:
%04x:%02x:%02x.%u\n",
> +                   __func__, pseg->nr, bus, PCI_SLOT(devfn), 
> PCI_FUNC(devfn));
> +            break;
> +    }
> +
>      return pdev;
>  }
>  
> -static void free_pdev(struct pci_dev *pdev)
> +static void free_pdev(struct pci_seg *pseg, struct pci_dev *pdev)
>  {
> +    /* update bus2bridge */
> +    switch ( pdev_type(pseg->nr, pdev->bus, pdev->devfn) )
> +    {
> +        u8 dev, func, sec_bus, sub_bus;
> +
> +        case DEV_TYPE_PCIe2PCI_BRIDGE:
> +        case DEV_TYPE_LEGACY_PCI_BRIDGE:
> +            dev = PCI_SLOT(pdev->devfn);
> +            func = PCI_FUNC(pdev->devfn);
> +            sec_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
> +                                     PCI_SECONDARY_BUS);
> +            sub_bus = pci_conf_read8(pseg->nr, pdev->bus, dev, func,
> +                                     PCI_SUBORDINATE_BUS);
> +
> +            spin_lock(&pseg->bus2bridge_lock);
> +            for ( ; sec_bus <= sub_bus; sec_bus++ )
> +                pseg->bus2bridge[sec_bus].map = 0;
> +            spin_unlock(&pseg->bus2bridge_lock);
> +            break;
> +    }
> +
>      list_del(&pdev->alldevs_list);
>      xfree(pdev);
>  }
> @@ -378,7 +434,7 @@ int pci_remove_device(u16 seg, u8 bus, u
>              if ( pdev->domain )
>                  list_del(&pdev->domain_list);
>              pci_cleanup_msi(pdev);
> -            free_pdev(pdev);
> +            free_pdev(pseg, pdev);
>              printk(XENLOG_DEBUG "PCI remove device
%04x:%02x:%02x.%u\n",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
>              break;
> @@ -546,8 +602,6 @@ static int __init _scan_pci_devices(stru
>  {
>      struct pci_dev *pdev;
>      int bus, dev, func;
> -    u8 sec_bus, sub_bus;
> -    int type;
>  
>      for ( bus = 0; bus < 256; bus++ )
>      {
> @@ -565,41 +619,6 @@ static int __init _scan_pci_devices(stru
>                      return -ENOMEM;
>                  }
>  
> -                /* build bus2bridge */
> -                type = pdev_type(pseg->nr, bus, PCI_DEVFN(dev, func));
> -                switch ( type )
> -                {
> -                    case DEV_TYPE_PCIe_BRIDGE:
> -                        break;
> -
> -                    case DEV_TYPE_PCIe2PCI_BRIDGE:
> -                    case DEV_TYPE_LEGACY_PCI_BRIDGE:
> -                        sec_bus = pci_conf_read8(pseg->nr, bus, dev,
func,
> -                                                 PCI_SECONDARY_BUS);
> -                        sub_bus = pci_conf_read8(pseg->nr, bus, dev,
func,
> -                                                 PCI_SUBORDINATE_BUS);
> -
> -                        spin_lock(&pseg->bus2bridge_lock);
> -                        for ( sub_bus &= 0xff; sec_bus <= sub_bus;
sec_bus++ )
> -                        {
> -                            pseg->bus2bridge[sec_bus].map = 1;
> -                            pseg->bus2bridge[sec_bus].bus = bus;
> -                            pseg->bus2bridge[sec_bus].devfn > -     
PCI_DEVFN(dev, func);
> -                        }
> -                        spin_unlock(&pseg->bus2bridge_lock);
> -                        break;
> -
> -                    case DEV_TYPE_PCIe_ENDPOINT:
> -                    case DEV_TYPE_PCI:
> -                        break;
> -
> -                    default:
> -                        printk("%s: unknown type:
%04x:%02x:%02x.%u\n",
> -                               __func__, pseg->nr, bus, dev, func);
> -                        return -EINVAL;
> -                }
> -
>                  if ( !func && !(pci_conf_read8(pseg->nr, bus,
dev, func,
>                                                 PCI_HEADER_TYPE) &
0x80) )
>                      break;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Sep 2011 - RE: RE: Resend: RE: enable_ats_device() call site

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site

RE: [Xen-devel] RE: Resend: RE: enable_ats_device() call site