[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 2015/6/18 16:05, Jan Beulich wrote:
On 18.06.15 at 09:01, <tiejun.chen@xxxxxxxxx> wrote:
On 2015/6/18 14:29, Jan Beulich wrote:
On 18.06.15 at 08:17, <tiejun.chen@xxxxxxxxx> wrote:
On 2015/6/17 17:24, Jan Beulich wrote:
On 17.06.15 at 11:18, <tiejun.chen@xxxxxxxxx> wrote:
On 2015/6/17 17:02, Jan Beulich wrote:
On 17.06.15 at 10:26, <tiejun.chen@xxxxxxxxx> wrote:
Something hits me to generate another idea,

#1. Still allocate all devices as before.
#2. Lookup all actual bars to check if they're conflicting RMRR

We can skip these bars to keep zero. Then later it would make lookup easily.

#3. Need to reallocate these conflicting bars.
#3.1 Trying to reallocate them with the remaining resources
#3.2 If the remaining resources aren't enough, we need to allocate them
from high_mem_resource.

That's possible onyl for 64-bit BARs.

You're right so this means its not proper to adjust mmio_total to
include conflicting reserved ranges and finally moved all conflicting
bars to high_mem_resource as Kevin suggested previously, so i high
level, we still need to decrease pci_mem_start to populate more RAM to
compensate them as I did, right?

You probably should do both: Prefer moving things beyond 4Gb,
but if not possible increase the MMIO hole.


I'm trying to figure out a better solution. Perhaps we can allocate
32-bit bars and 64-bit bars orderly. This may help us bypass those
complicated corner cases.

Dealing with 32- and 64-bit BARs separately won't help at all, as

More precisely I'm saying to deal with them orderly.

there may only be 32-bit ones, or the set of 32-bit ones may
already require you to do re-arrangements. Plus, for compatibility

Yes but I don't understand they are specific cases to my idea.

Perhaps the problem is that you don't say what "orderly" is supposed
to mean here?

You're right. Here when "separately" vs "orderly", I should definitely use "orderly" to make more understandable.


reasons (just like physical machines' BIOSes do) avoiding to place
MMIO above 4Gb where possible is still a goal.

So are you sure you see my idea completely? I don't intend to expand pci
memory above 4GB.

Let me clear this simply,

#1. I'm still trying to allocate all 32bit bars from
[pci_mem_start,pci_mem_end] as before.

#2. But [pci_mem_start,pci_mem_end] mightn't enough cover all 32bit-bars
again because of RMRR, right? So I will populate RAM to push downward at
cur_pci_mem_start ( = pci_mem_start - reserved device memory), then
allocate the remaining 32bit-bars from [cur_pci_mem_start , pci_mem_start]

#3. Then I'm still trying to allocate 64bit-bars from
[pci_mem_start,pci_mem_end], unless its not enough. This is just going
to follow the original.

So anything is breaking that goal?

Maybe not, from the above.

And overall, its same as the original.

If the model follows the original, what's the point of outlining
supposed changes to the model? All I'm trying to understand is how

Its not same completely, or let me change this statement, "same" -> "similar".

you want to change the current code to accommodate the not
aligned reserved memory regions. If everything is the same as
before, this can't have been taken care of. If something is different
from the original, that's what needs spelling out (and nothing else,
as that would only clutter the picture).

Doesn't look like the right approach to me. As said before, I think

Could you see what I'm saying again? I just feel you don't understand
what you mean. If you still think I'm wrong let me know.

I think I understand what _I_ mean, but I'm indeed unsure I see
what _you_ mean. Part of the problem is that you toggle between
sending (incomplete) patches, code fragments, and discussing the
approach verbally. I'd much prefer if either you started with a clear
picture of what you intend to implement, or with an implementation
that at least attempts to take care of all the corner cases (showing
that you understand what the corner cases are, which so far I'm
getting the - perhaps false - impression that you don't).


Based on my previous recognition and our recent discussion, my current understanding can be summarized as follows;

#1. Goal

MMIO region should exclude all reserved device memory

#2. Requirements

#2.1 Still need to make sure MMIO region is fit all pci devices as before

#2.2 Accommodate the not aligned reserved memory regions

If I'm missing something let me know.

#3. How to

#3.1 Address #2.1

We need to either of populating more RAM, or of expanding more highmem. But we should know just 64bit-bar can work with highmem, and as you mentioned we also should avoid expanding highmem as possible.

So my implementation is to allocate 32bit-bar and 64bit-bar orderly.

1>. The first allocation round just to 32bit-bar

If we can finish allocating all 32bit-bar, we just go to allocate 64bit-bar with all remaining resources including low pci memory.

If not, we need to calculate how much RAM should be populated to allocate the remaining 32bit-bars, then populate sufficient RAM as
exp_mem_resource to go to the second allocation round 2>.

2>. The second allocation round to the remaining 32bit-bar

We should can finish allocating all 32bit-bar in theory, then go to the third allocation round 3>.

3>. The third allocation round to 64bit-bar

We'll try to first allocate from the remaining low memory resource. If that isn't enough, we try to expand highmem to allocate for 64bit-bar. This process should be same as the original.

I think my pasted patch should represent this framework above.

#3.2 Address #2.2

As you said, we need to accommodate the not aligned reserved memory regions. I didn't consider this more previously.

Now I'm trying to rewrite that chunk of code fragment to address this point like this,

-        base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
+ reallocate_bar:
+        base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
         bar_data |= (uint32_t)base;
         bar_data_upper = (uint32_t)(base >> 32);
+        /*
+         * We should skip all reserved device memory, but we also need
+         * to check if other smaller bars can be allocated if a mmio hole
+         * exists between resource->base and reserved device memory.
+         */
+        for ( j = 0; j < memory_map.nr_map ; j++ )
+        {
+            if ( memory_map.map[j].type != E820_RAM )
+            {
+                reserved_start = memory_map.map[i].addr;
+                reserved_size = memory_map.map[i].size;
+                reserved_end = reserved_start + reserved_size;
+                if ( check_overlap(base, bar_sz,
+                                   reserved_start, reserved_size) )
+                {
+                    /*
+                     * If a hole exists between base and reserved device
+                     * memory, lets go out simply to try allocate for next
+                     * bar since all bars are in descending order of size.
+                     */
+                    if ( resource->base < reserved_start )
+                        continue;
+                    /*
+                     * If not, we need to move resource->base to
+                     * reserved_end just to reallocate this bar.
+                     */
+                    else
+                    {
+                        resource->base = reserved_end;
+                        goto reallocate_bar;
+                    }
+                }
+            }
+        }
         base += bar_sz;


Thanks
Tiejun

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.