[Xen-ia64-devel] Re: [4/4] Issue ioremap hypercall in pci_acpi_scan_root() in order to map /dev/mem

[Jun Kamada <kama@xxxxxxxxxxxxxx>]

Hi, Alex-san,

Thank you for your helpful comments.
I will attach a modified patch.

Thanks


On Thu, 19 Jul 2007 13:04:44 -0600
Alex Williamson <alex.williamson@xxxxxx> wrote:

> 
>    While we're waiting on staging to get pushed out, here are a few more
> comments.  Casting ioremap_issue_list_top as a struct ioremap_issue_list
> is awkward, and I think unnecessary.  The gotos are easy to remove too.
> You might also want to consider a few simple variable renames, for
> instance "top" is used as both a struct resource and a struct
> ioremap_issue_list.  A few additional cleanups below.  Thanks,
> 
>       Alex
> 
> On Fri, 2007-07-13 at 18:40 +0900, Jun Kamada wrote:
> > # HG changeset patch
> > # User Jun Kamada <kama@xxxxxxxxxxxxxx>
> > # Date 1184348594 -32400
> > # Node ID b82a7428f53a5d03c964fd30d21100429376e64f
> > # Parent  031ea456e2756b3f7c10c00f7fcdccb4fc01baa2
> > Issue ioremap hypercall in pci_acpi_scan_root() in order to map /dev/mem.
> > 
> > Signed-off-by: Jun Kamada <kama@xxxxxxxxxxxxxx>
> > 
> > diff -r 031ea456e275 -r b82a7428f53a arch/ia64/pci/pci.c
> > --- a/arch/ia64/pci/pci.c       Sat Jul 14 02:41:26 2007 +0900
> > +++ b/arch/ia64/pci/pci.c       Sat Jul 14 02:43:14 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
> > @@ -337,6 +346,182 @@ pcibios_setup_root_windows(struct pci_bu
> >         }
> >  }
> >  
> > +#ifdef CONFIG_XEN
> > +static void
> > +__cleanup_issue_list(ioremap_issue_list_t *top)
> 
> s/ioremap_issue_list_t/struct list/
> 
> 
> > +{
> > +       ioremap_issue_list_t    *ptr, *tmp_ptr;
> > +
> > +       list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
> 
> s/&(top->listp)/top/
> 
> > +               list_del(&(ptr->listp));
> > +               kfree(ptr);
> > +       }
> > +}
> > +
> > +static int
> > +__add_issue_list(unsigned long start, unsigned long end,
> > +                ioremap_issue_list_t *top)
> 
> s/ioremap_issue_list_t/struct list/
> 
> > +{
> > +       ioremap_issue_list_t    *ptr, *new;
> > +
> > +       if (start > end) {
> > +               printk(KERN_ERR "%s: Internal error (start addr > end 
> > addr)\n",
> > +                      __FUNCTION__);
> > +               return 0;
> > +       }
> > +
> > +       /* Head of the resource structure list contains */
> > +       /* dummy val.(start=0, end=~0), so skip it      */
> > +       if ((start == 0) && (end == ~0)) {
> 
> > +               return 0;
> > +       }
> > +
> > +       start &=  PAGE_MASK;
> > +       end   |= ~PAGE_MASK;
> > +
> > +       /* We can merge specified address range into existing entry */
> > +       list_for_each_entry(ptr, &(top->listp), listp) {
> 
> s/&(top->listp)/top
> 
> > +               if ((ptr->start > end + 1) || (ptr->end + 1 < start))
> > +                       continue;
> > +               ptr->start = min(start, ptr->start);
> > +               ptr->end   = max(end, ptr->end);
> > +               goto out;
> > +       }
> > +
> > +       /* We could not merge, so create new entry */
> > +       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;
> > +
> > +       /* Insert the new entry to the list by ascending order */
> > +       if (list_empty(&(top->listp))) {
> 
> s/&(top->listp)/top
> 
> > +               list_add_tail(&(new->listp), &(top->listp));
> 
> s/&(top->listp)/top
> 
> > +               goto out;
> 
> s/goto out/return 0/
> 
> > +       }
> > +       list_for_each_entry(ptr, &(top->listp), listp) {
> 
> s/&(top->listp)/top
> 
> > +               if (new->start > ptr->start)
> > +                       continue;
> > +               list_add(&(new->listp), ((struct list_head *)ptr)->prev);
> > +               goto out;
> 
> s/goto out/return 0/
> 
> > +       }
> > +       list_add_tail(&(new->listp), &(top->listp));
> 
> s/&(top->listp)/top
> 
> 
> > +
> > +out:
> 
> s/out://
> 
> > +       return 0;
> > +}
> > +
> > +static int
> > +__make_issue_list(struct resource *ptr, ioremap_issue_list_t *top)
> 
> s/ioremap_issue_list_t/struct list/
> 
> > +{
> > +       int     ret;
> > +
> > +       if (ptr->child) {
> > +               ret = __make_issue_list(ptr->child, top);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +       if (ptr->sibling) {
> > +               ret = __make_issue_list(ptr->sibling, top);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       if (ptr->flags & IORESOURCE_MEM) {
> > +               ret = __add_issue_list(ptr->start, ptr->end, top);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void
> > +__compress_issue_list(ioremap_issue_list_t *top)
> > +{
> > +       ioremap_issue_list_t    *ptr, *tmp_ptr, *next;
> > +       int                     compressed;
> > +
> > +       /* merge adjacent entries, if overlapped                */
> > +       /* (assumes entries are sorted by ascending order)      */
> 
> s/assumes//  - At least I hope we're sure they're sorted ;)
> 
> > +       do {
> > +               compressed = 0;
> > +               
> > +               list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), 
> > listp) {
> 
> s/&(top->listp)/top
> 
> > +                       if (list_is_last((struct list_head *)ptr,
> > +                                        &(top->listp)) == 0) {
> 
> s/&(top->listp)/top
> 
> Rather than adding another level of nesting here, how about:
> 
>       if (list_is_last((struct list_head *)ptr, top))
>               continue; /* or maybe break */
> 
> > +                               next = (ioremap_issue_list_t *)
> > +                                       (((struct list_head *)ptr)->next);
> > +                               if (next->start <= (ptr->end) + 1) {
> > +                                       next->start = min(ptr->start,
> > +                                                         next->start);
> > +                                       next->end   = max(ptr->end,
> > +                                                         next->end);
> > +
> > +                                       list_del(&(ptr->listp));
> > +                                       kfree(ptr);
> > +                                       compressed = 1;
> > +                               }
> > +                       }
> > +               }
> > +       } while (compressed == 1);
> 
> 
> Does this really need the do/while?  We're expanding the next list entry
> and removing the current, so it seems like we complete the merging in
> one pass.
> 
> > +}
> > +
> > +static int
> > +__issue_ioremap(ioremap_issue_list_t *top)
> 
> s/ioremap_issue_list_t/struct list/
> 
> > +{
> > +       ioremap_issue_list_t    *ptr, *tmp_ptr;
> > +       unsigned int            offset;
> > +
> > +       list_for_each_entry_safe(ptr, tmp_ptr, &(top->listp), listp) {
> 
> 
> s/&(top->listp)/top
> 
> > +               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_del(&(ptr->listp));
> > +               kfree(ptr);
> > +       }
> > +       
> > +       return 0;
> > +}
> > +
> > +static int
> > +do_ioremap_on_resource_list(struct resource *top)
> > +{
> > +       struct list_head        ioremap_issue_list_top = {
> > +               &ioremap_issue_list_top,
> > +               &ioremap_issue_list_top
> > +       };
> 
> Use:
> 
>       LIST_HEAD(ioremap_issue_list_top);
> 
> > +       int                     ret;
> > +
> > +       ret = __make_issue_list(top,
> > +                       (ioremap_issue_list_t *)(&ioremap_issue_list_top));
> 
> s/(ioremap_issue_list_t *)//
> 
> > +       if (ret) {
> > +               __cleanup_issue_list((ioremap_issue_list_t *)
> 
> s/(ioremap_issue_list_t *)//
> 
> > +                                    (&ioremap_issue_list_top));
> > +               return ret;
> > +       }
> > +
> > +       __compress_issue_list((ioremap_issue_list_t *)
> 
> s/(ioremap_issue_list_t *)//
> 
> > +                             (&ioremap_issue_list_top));
> > +
> > +       (void)__issue_ioremap((ioremap_issue_list_t *)
> 
> s/(ioremap_issue_list_t *)//
> 
> > +                             (&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)
> >  {
> > @@ -379,6 +564,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.
> 

Jun Kamada
Linux Technology Development Div.
Server Systems Unit
Fujitsu Ltd.
kama@xxxxxxxxxxxxxx

Attachment: new_paravirt_pci.patch
Description: Binary data

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