Re: [RFC][PATCH 13/13] hvmloader/e820: construct guest e820 table

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

On 2015/5/15 16:12, Jan Beulich wrote:
>>>> On 15.05.15 at 10:00, <tiejun.chen@intel.com> wrote:
>> On 2015/5/15 15:34, Jan Beulich wrote:
>>>>>> On 15.05.15 at 09:11, <tiejun.chen@intel.com> wrote:
>>>> On 2015/5/15 14:56, Jan Beulich wrote:
>>>>>>>> On 15.05.15 at 08:39, <tiejun.chen@intel.com> wrote:
>>>>>> On 2015/5/15 14:25, Jan Beulich wrote:
>>>>>>>>>> On 15.05.15 at 08:11, <tiejun.chen@intel.com> wrote:
>>>>>>>> Even we may separate the
>>>>>>>> low memory to construct memory_map.map[]...
>>>>>>>
>>>>>>> ???
>>>>>>
>>>>>> Sorry I just mean that the low memory is not represented with only one
>>>>>> memory_map.map[] in some cases.
>>>>>
>>>>> That's correct.
>>>>>
>>>>
>>>> So just lets keep that original BUG_ON()?
>>>
>>> In your previous reply you seemed to agree that the BUG_ON() is
>>> becoming meaningless. Why do you now suggest to keep it then?
>>>
>>
>> Sorry just let me clear this.
>>
>> We still need to check this,
>>
>> (hvm_info->low_mem_pgend << PAGE_SHIFT) < (2u << 20)
>>
>> Right? I agree the original is really less relevant as you said.
>
> And I didn't ask you to drop it. All I asked it to amend it with another
> BUG_ON() checking what the one above won't cover anymore.
>

Another point hits me in this case while we're discussing MMIO in 
another email.

We may populate RAM to get enough MMIO in pci_setup() so this means 
hvm_info->low_mem_pgend would be changed. Furthermore, low_mem_pgend 
isn't going to keep recording our original lowmem while building domain. 
So I think this original BUG_ON() is still good to cover this case. But 
obviously, we need to adjust its associated memory_map.map[x] right now.

So what about this?

@@ -74,6 +74,7 @@ int build_e820_table(struct e820entry *e820,
                       unsigned int bios_image_base)
  {
      unsigned int nr = 0, i, j;
+    uint64_t low_mem_pgend = hvm_info->low_mem_pgend << PAGE_SHIFT;

      if ( !lowmem_reserved_base )
              lowmem_reserved_base = 0xA0000;
@@ -117,9 +118,6 @@ int build_e820_table(struct e820entry *e820,
      e820[nr].type = E820_RESERVED;
      nr++;

-    /* Low RAM goes here. Reserve space for special pages. */
-    BUG_ON((hvm_info->low_mem_pgend << PAGE_SHIFT) < (2u << 20));
-
      /*
       * Explicitly reserve space for special pages.
       * This space starts at RESERVED_MEMBASE an extends to cover various
@@ -162,6 +160,17 @@ int build_e820_table(struct e820entry *e820,
          nr++;
      }

+    /* Low RAM goes here. Reserve space for special pages. */
+    BUG_ON(low_mem_pgend < (2u << 20));
+    /* We may need to adjust lowmem end. */
+    for ( i = 0; i < memory_map.nr_map; i++ )
+    {
+        uint64_t end = e820[i].addr + e820[i].size;
+        if ( e820[i].type == E820_RAM &&
+             low_mem_pgend > e820[i].addr && low_mem_pgend < end )
+            e820[i].size = low_mem_pgend - e820[i].addr;
+    }
+
      /* May need to reorder all e820 entries. */
      for ( j = 0; j < nr-1; j++ )
      {


Thanks
Tiejun