Re: [Xen-devel] [v3][PATCH 07/16] hvmloader/pci: skip reserved ranges

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

On 2015/6/17 15:19, Jan Beulich wrote:
> > > > On 17.06.15 at 09:10, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2015/6/16 17:40, Jan Beulich wrote:
> > > > > > On 16.06.15 at 11:29, <tiejun.chen@xxxxxxxxx> wrote:
> > > > I'm trying to walk into this direction:
> > > >
> > > >        /*
> > > >         * We'll skip all space overlapping with reserved memory later,
> > > >         * so we need to increase mmio_total to compensate them.
> > > >         */
> > > >        for ( j = 0; j < memory_map.nr_map ; j++ )
> > > >        {
> > > >            uint64_t conflict_size = 0;
> > > >            if ( memory_map.map[j].type != E820_RAM )
> > > >            {
> > > >                reserved_start = memory_map.map[j].addr;
> > > >                reserved_size = memory_map.map[j].size;
> > > >                reserved_end = reserved_start + reserved_size;
> > > >                if ( check_overlap(pci_mem_start, pci_mem_end - pci_mem_start,
> > > >                                   reserved_start, reserved_size) )
> > > >                {
> > > >                    /*
> > > >                     * Calculate how much mmio range conflict with
> > > >                     * reserved device memory.
> > > >                     */
> > > >                    conflict_size += reserved_size;
> > > >
> > > >                    /*
> > > >                     * But we may need to subtract those sizes beyond the
> > > >                     * pci memory, [pci_mem_start, pci_mem_end].
> > > >                     */
> > > >                    if ( reserved_start < pci_mem_start )
> > > >                        conflict_size -= (pci_mem_start - reserved_start);
> > > >                    if ( reserved_end > pci_mem_end )
> > > >                        conflict_size -= (reserved_end - pci_mem_end);
> > > >                }
> > > >            }
> > > >
> > > >            if ( conflict_size )
> > > >            {
> > > >                uint64_t conflict_size = max_t(
> > > >                        uint64_t, conflict_size, max_bar_sz);
> > > >                conflict_size &= ~(conflict_size - 1);
> > > >                mmio_total += conflict_size;
> > > >            }
> > > >        }
> > > This last thing goes in the right direction, but is complete overkill
> > > when you have a small reserved region and a huge BAR. You
> > Yeah, this may waste some spaces in this worst case but I this think
> > this can guarantee our change don't impact on the original expectation,
> > right?
> "Some space" may be multiple Gb (e.g. the frame buffer of a graphics
Sure.

> card), which is totally unacceptable.
But then I don't understand what's your way. How can we fit all pci 
devices just with "the smallest power-of-2 region enclosing the reserved 
device memory"?
For example, the whole pci memory is sitting at
[0xa0000000, 0xa2000000]. And there are two PCI devices, A and B. Note 
each device needs to be allocated with 0x1000000. So if without 
concerning RMRR,
A. [0xa0000000,0xa1000000]
B. [0xa1000000,0xa2000000]

But if one RMRR resides at [0xa0f00000, 0xa1f00000] which obviously 
generate its own alignment with 0x1000000. So the pci memory is expended 
as [0xa0000000, 0xa3000000], right?
Then actually the whole pci memory can be separated three segments like,

#1. [0xa0000000, 0xa0f00000]
#2. [0xa0f00000, 0xa1f00000] -> RMRR would occupy
#3. [0xa1f00000, 0xa3000000]

So just #3 can suffice to allocate but just for one device, right?

If I'm wrong please correct me.

> > > ought to work out the smallest power-of-2 region enclosing the
> > Okay. I remember the smallest size of a given PCI I/O space is 8 bytes,
> > and the smallest size of a PCI memory space is 16 bytes. So
> >
> > /* At least 16 bytes to align a PCI BAR size. */
> > uint64_t align = 16;
> >
> > reserved_start = memory_map.map[j].addr;
> > reserved_size = memory_map.map[j].size;
> >
> > reserved_start = (reserved_star + align) & ~(align - 1);
> > reserved_size = (reserved_size + align) & ~(align - 1);
> >
> > Is this correct?
> Simply aligning the region doesn't help afaict. You need to fit it
> with the other MMIO allocations.
I guess you're saying just those mmio allocations conflicting with RMRR? 
But we don't know these exact addresses until we finalize to allocate 
them, right?
Thanks
Tiejun

> > > reserved range (albeit there are tricky corner cases to consider).
> > Yeah, its a little tricky since RMRR always owns a fixed start address,
> > so we can't reorder them with all pci bars. I just think at least we
> > should provide a correct solution now, then further look into what can
> > be optimized. So I think we'd better get conflict_size with
> > max(conflict_size, max_bar_sz), right?
> As per above - no, this is not an option.
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel