WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

Re: [Xen-ia64-devel] [Patch 4/4] Xwindow: Modify pci_acpi_scan_root()

To: Jun Kamada <kama@xxxxxxxxxxxxxx>
Subject: Re: [Xen-ia64-devel] [Patch 4/4] Xwindow: Modify pci_acpi_scan_root()
From: Alex Williamson <alex.williamson@xxxxxx>
Date: Thu, 14 Jun 2007 14:24:25 -0600
Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Thu, 14 Jun 2007 13:22:26 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20070613155519.F2AB.KAMA@xxxxxxxxxxxxxx>
List-help: <mailto:xen-ia64-devel-request@lists.xensource.com?subject=help>
List-id: Discussion of the ia64 port of Xen <xen-ia64-devel.lists.xensource.com>
List-post: <mailto:xen-ia64-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: HP OSLO R&D
References: <20070530140041.1D6E.KAMA@xxxxxxxxxxxxxx> <20070613153731.F29F.KAMA@xxxxxxxxxxxxxx> <20070613155519.F2AB.KAMA@xxxxxxxxxxxxxx>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
Hi,

   Some more comments below.  Thanks for switching to the standard list
implementation, it looks better.  How do you propose coordinating the
patches for xen-ia64-devel vs the one for xen-devel?  Should we get the
latter in first, then proceed w/ the xen-ia64-devel patch?  Thanks,

        Alex

On Wed, 2007-06-13 at 15:56 +0900, Jun Kamada wrote:
> # HG changeset patch
> # User Jun Kamada <kama@xxxxxxxxxxxxxx>
> # Date 1181706151 -32400
> # Node ID 76e7ecd69d2ce751bf33784f62762c2ba8ed7162
> # Parent  7286707307032b6b3117a4f48aa4f0fdce6e7587
> Issue ioremap hypercall in pci_acpi_scan_root() in order to
> map /dev/mem.
> 
> Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>
> 
> diff -r 728670730703 -r 76e7ecd69d2c
> linux-2.6-xen-sparse/arch/ia64/pci/pci.c
> --- a/linux-2.6-xen-sparse/arch/ia64/pci/pci.c  Wed Jun 13 12:38:18
> 2007 +0900
> +++ b/linux-2.6-xen-sparse/arch/ia64/pci/pci.c  Wed Jun 13 12:42:31
> 2007 +0900
> @@ -29,6 +29,15 @@
>  #include <asm/smp.h>
>  #include <asm/irq.h>
>  #include <asm/hw_irq.h>
> +
> +#ifdef CONFIG_XEN
> +struct ioremap_issue_list {
> +       struct list_head                listp;
> +       unsigned long                   start;
> +       unsigned long                   end;
> +};
> +typedef struct ioremap_issue_list      ioremap_issue_list_t;
> +#endif /* CONFIG_XEN */
>  
>  /*
>   * Low-level SAL-based PCI configuration access functions. Note that
> SAL
> @@ -332,6 +341,165 @@ pcibios_setup_root_windows(struct pci_bu
>         }
>  }
>  
> +#ifdef CONFIG_XEN
> +
> +#define MIN(x,y)       (((x) <= (y)) ? (x) : (y))
> +#define MAX(x,y)       (((x) >= (y)) ? (x) : (y))

min() and max() are already defined in kernel.h, can you make use of
those?

> +static int
> +__add_issue_list(unsigned long start, unsigned long end,
> +                ioremap_issue_list_t *top)
> +{
> +       ioremap_issue_list_t    *ptr, *new;
> +
> +       if (start > end) {
> +               printk(KERN_ERR "%s: Internal error (start addr > end
> addr)\n",
> +                          __FUNCTION__);
> +               return 0;
> +       }
> +
> +       start &= ~(PAGE_SIZE - 1);

Use PAGE_MASK

> +       end   |=  (PAGE_SIZE - 1);

And ~PAGE_MASK

> +
> +       list_for_each_entry(ptr, &(top->listp), listp) {
> +               if (((ptr->start >= start) && (ptr->start <= end + 1))
> ||
> +                   ((ptr->end >= start - 1) && (ptr->end <= end))
> ||
> +                   ((start >= ptr->start) && (start <= ptr->end) &&
> +                    (end >= ptr->start) && (end <= ptr->end))) {
> +                       ptr->start = MIN(start, ptr->start);
> +                       ptr->end   = MAX(end, ptr->end);
> +                       return 0;
> +               }

Can't this be greatly simplified?  Instead of checking all the degrees
of overlap, just look for the non-overlap cases.  Everything else is
overlap

                if (ptr->start > end + 1 || ptr->end + 1 < start)
                        continue;

                ptr->start = min(start, ptr->start);
                ptr->end = max(end, ptr->end);
                return 0;

However, what if this grows the range such that it bumps into the
entries around it.  Do you plan to merge them?  Maybe keeping the list
sorted would make this easier.

> +       }
> +
> +       new = kmalloc(sizeof(ioremap_issue_list_t), GFP_KERNEL);
> +       if (new == NULL) {
> +               printk(KERN_ERR "%s: Could not allocate memory. "
> +                      "HYPERVISOR_ioremap will not be issued\n",
> +                      __FUNCTION__);
> +               return -ENOMEM;
> +       }
> +
> +       new->start = start;
> +       new->end   = end;
> +
> +       list_add(&(new->listp), &(top->listp));
> +
> +       return 0;
> +}
> +
> +static int
> +__make_issue_list(struct resource *ptr, ioremap_issue_list_t *top) {
> +       unsigned int    curr_lvl = 1;
> +       unsigned int    prev_lvl = 0;
> +       int             ret;
> +
> +       for ( ;; ) {
> +               if (ptr->flags & IORESOURCE_MEM) {
> +                       ret = __add_issue_list(ptr->start, ptr->end,
> top);
> +                       if (ret)
> +                               return ret;
> +               }
> +

Thanks, the comments below are helpful, but wouldn't it make sense to do
this as a recursive algorithm?  Doesn't something like this do the same
thing (untested)?

int scan_foo(struct resource *res, ioremap_issue_list_t *list)
{
        int ret;

        if (res->child) {
                ret = scan_foo(res->child, list);
                if (ret)
                        return ret;
        }
        if (res->sibling) {
                ret = scan_foo(res->sibling, list);
                if (ret)
                        return ret;
        }

        if (ptr->flags & IORESOURCE_MEM) {
                ret = __add_issue_list(res->start, res->end, list);
                if (ret)
                        return ret;
        }
        return 0;
}

> +               /* means that the "ptr" came here from parent (upper
> level) */
> +               if (curr_lvl > prev_lvl) {
> +                       /* child exists, so go down to the child */
> +                       if (ptr->child != 0) {
> +                               ptr = ptr->child;
> +                               curr_lvl++;
> +                               prev_lvl++;
> +                       /* sibling exists, so go to the sibling */
> +                       } else if (ptr->sibling != 0) {
> +                               ptr = ptr->sibling;
> +                       /* no child and no sibling exist, so go up to
> parent */
> +                       } else {
> +                               ptr = ptr->parent;
> +                               curr_lvl--;
> +                               prev_lvl++;
> +                               if (curr_lvl == 0)
> +                                       return 0;
> +                       }
> +
> +               /* means that the "ptr" came here from child (lower
> level) */
> +               } else if (curr_lvl < prev_lvl) {
> +                       /* sibling exists, so go to the sibling */
> +                       if (ptr->sibling != 0) {
> +                               ptr = ptr->sibling;
> +                               prev_lvl = curr_lvl - 1;
> +                       /* no sibling exists, so go up to parent */
> +                       } else {
> +                               ptr = ptr->parent;
> +                               curr_lvl--;
> +                               prev_lvl--;
> +                               if (curr_lvl == 0)
> +                                       return 0;
> +                       }
> +               } else {
> +                       printk(KERN_ERR "%s: Internal error. "
> +                              "Must not reach here\n", __FUNCTION__);
> +                       return -EFAULT;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int
> +__issue_ioremap(struct ioremap_issue_list *top)
> +{
> +       ioremap_issue_list_t    *ptr, *tmp_ptr;
> +       unsigned int            offset;
> +
> +       list_for_each_entry(ptr, &(top->listp), listp) {
> +               offset = HYPERVISOR_ioremap(ptr->start,
> +                                           ptr->end - ptr->start +
> 1);
> +               if (offset == ~0) {
> +                       printk(KERN_ERR "%s: HYPERVISOR_ioremap()
> failed. "
> +                              "Address Range: 0x%016lx-0x%016lx\n",
> +                              __FUNCTION__, ptr->start, ptr->end);
> +               }
> +       }
> +
> +       list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
> +               list_del(&(ptr->listp));
> +               kfree(ptr);
> +       }

   These could be done in the same pass through the list.

> +       return 0;
> +}
> +
> +static int
> +do_ioremap_on_resource_list(struct resource *ptr)
> +{
> +       ioremap_issue_list_t    *ioremap_issue_list_top;
> +       int                     ret;
> +
> +       ioremap_issue_list_top = kmalloc(sizeof(ioremap_issue_list_t),
> +                                        GFP_KERNEL);
> +       if (ioremap_issue_list_top == NULL)
> +               return -ENOMEM;

  The top of the list doesn't need to be an ioremap_issue_list_t, it can
just be a struct list.  That could just be put on the stack rather than
dynamically allocated.

> +
> +       INIT_LIST_HEAD(&(ioremap_issue_list_top->listp));
> +

   If you took the recursive approach above, these could be eliminated,
just call scan_foo(ptr, &list).

> +       if (ptr->child != 0) {
> +               ret = __make_issue_list(ptr->child,
> ioremap_issue_list_top);
> +               if (ret) {

memory leak - be careful to do list_del & kfree should anything fail

> +                       return ret;
> +               }
> +       }
> +       if (ptr->sibling != 0) {
> +               ret = __make_issue_list(ptr->sibling,
> ioremap_issue_list_top);
> +               if (ret) {

memory leak - ditto

> +                       return ret;
> +               }
> +       }
> +
> +       (void)__issue_ioremap(ioremap_issue_list_top);
> +
> +       return 0;
> +}
> +#endif /* CONFIG_XEN */
> +
>  struct pci_bus * __devinit
>  pci_acpi_scan_root(struct acpi_device *device, int domain, int bus)
>  {
> @@ -374,6 +542,18 @@ pci_acpi_scan_root(struct acpi_device *d
>         pbus = pci_scan_bus_parented(NULL, bus, &pci_root_ops,
> controller);
>         if (pbus)
>                 pcibios_setup_root_windows(pbus, controller);
> +
> +#ifdef CONFIG_XEN
> +       if (is_initial_xendomain()) {
> +               if (do_ioremap_on_resource_list(&iomem_resource) != 0)
> {
> +                       printk(KERN_ERR
> +                              "%s: Counld not issue
> HYPERVISOR_ioremap "
> +                              "due to lack of memory or hypercall
> failure\n",
> +                              __FUNCTION__);
> +                       goto out3;
> +               }
> +       }
> +#endif /* CONFIG_XEN */
>  
>         return pbus;
>   
-- 
Alex Williamson                             HP Open Source & Linux Org.


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