Re: [Xen-devel] [RFC][PATCH 12/13] hvmloader/pci: skip reserved ranges

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/5/15 14:19, Jan Beulich wrote:
> > > > On 15.05.15 at 05:18, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2015/4/20 22:21, Jan Beulich wrote:
> > > > > > On 10.04.15 at 11:22, <tiejun.chen@xxxxxxxxx> wrote:
> > > > --- a/tools/firmware/hvmloader/pci.c
> > > > +++ b/tools/firmware/hvmloader/pci.c
> > > > @@ -59,8 +59,8 @@ void pci_setup(void)
> > > >            uint32_t bar_reg;
> > > >            uint64_t bar_sz;
> > > >        } *bars = (struct bars *)scratch_start;
> > > > -    unsigned int i, nr_bars = 0;
> > > > -    uint64_t mmio_hole_size = 0;
> > > > +    unsigned int i, j, nr_bars = 0;
> > > > +    uint64_t mmio_hole_size = 0, reserved_end;
> > > >
> > > >        const char *s;
> > > >        /*
> > > > @@ -393,8 +393,23 @@ void pci_setup(void)
> > > >            }
> > > >
> > > >            base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
> > > > + reallocate_mmio:
> > > >            bar_data |= (uint32_t)base;
> > > >            bar_data_upper = (uint32_t)(base >> 32);
> > > > +        for ( j = 0; j < memory_map.nr_map ; j++ )
> > > > +        {
> > > > +            if ( memory_map.map[j].type != E820_RAM )
> > > > +            {
> > > > +                reserved_end = memory_map.map[j].addr +
> > memory_map.map[j].size;
> > > > +                if ( check_hole_conflict(base, bar_sz,
> > > > +                                         memory_map.map[j].addr,
> > > > +                                         memory_map.map[j].size) )
> > > > +                {
> > > > +                    base = (reserved_end  + bar_sz - 1) & ~(uint64_t)(bar_sz -
> > 1);
> > > > +                    goto reallocate_mmio;
> > > > +                }
> > > > +            }
> > > > +        }
> > > >            base += bar_sz;
> > > >
> > > >            if ( (base < resource->base) || (base > resource->max) )
> > Actually some original codes are missing here,
> >
> >           if ( (base < resource->base) || (base > resource->max) )
> >           {
> >               printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
> >                      "resource!\n", devfn>>3, devfn&7, bar_reg,
> >                      PRIllx_arg(bar_sz));
> >               continue;
> >           }
> >
> > I think this can guarantee the MMIO regions just fit in the available RAM.
> >
> > Or am I wrong?
> The code you cite guarantees almost nothing, it simply skips assigning
> resources. Your changes potentially growing the space needed to fit
> all MMIO BARs therefore also needs to adjust the up front calculation,
> such that if necessary more RAM can be relocated to make the hole
> large enough.
You're right.

Just think about we're always trying to check pci_mem_start to populate 
more RAM to obtain enough PCI mempry,
    /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
    while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
    {
        struct xen_add_to_physmap xatp;
        unsigned int nr_pages = min_t(
            unsigned int,
            hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
            (1u << 16) - 1);
        if ( hvm_info->high_mem_pgend == 0 )
            hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
        hvm_info->low_mem_pgend -= nr_pages;
        printf("Relocating 0x%x pages from "PRIllx" to "PRIllx\
               " for lowmem MMIO hole\n",
               nr_pages,
               PRIllx_arg(((uint64_t)hvm_info->low_mem_pgend)<<PAGE_SHIFT),

PRIllx_arg(((uint64_t)hvm_info->high_mem_pgend)<<PAGE_SHIFT));
        xatp.domid = DOMID_SELF;
        xatp.space = XENMAPSPACE_gmfn_range;
        xatp.idx   = hvm_info->low_mem_pgend;
        xatp.gpfn  = hvm_info->high_mem_pgend;
        xatp.size  = nr_pages;
        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
            BUG();
        hvm_info->high_mem_pgend += nr_pages;
    }


So I think we may need to adjust pci_mem_start like this,

@@ -301,6 +301,19 @@ void pci_setup(void)
             pci_mem_start <<= 1;
     }

+    /* Relocate PCI memory that overlaps reserved space, like RDM. */
+    for ( j = 0; j < memory_map.nr_map ; j++ )
+    {
+        if ( memory_map.map[j].type != E820_RAM )
+        {
+            reserved_end = memory_map.map[j].addr + memory_map.map[j].size;
+            if ( check_overlap(pci_mem_start, pci_mem_end,
+                               memory_map.map[j].addr,
+                               memory_map.map[j].size) )
+                pci_mem_start -= memory_map.map[j].size >> PAGE_SHIFT;
+        }
+    }
+
     if ( mmio_total > (pci_mem_end - pci_mem_start) )
     {
         printf("Low MMIO hole not large enough for all devices,"

Right?

Thanks
Tiejun


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