Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps

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

On 2014/10/24 22:56, Jan Beulich wrote:
>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> wrote:
>> We need to check to reserve all reserved device memory maps in e820
>> to avoid any potential guest memory conflict.
>>
>> Currently, if we can't insert RDM entries directly, we may need to handle
>> several ranges as follows:
>> a. Fixed Ranges --> BUG()
>>   lowmem_reserved_base-0xA0000: reserved by BIOS implementation,
>>   BIOS region,
>>   RESERVED_MEMBASE ~ 0x100000000,
>
> This seems conceptually wrong to me, and I said so before:
> Depending on host characteristics this approach may mean you're
> going to be unable to build any HVM guests. Minimally there needs
> to be a way to avoid these checks (resulting in devices associated
> with RMRRs not being assignable to such a guest). I'm therefore

I just use 'err' to indicate if these fixed range overlaps RMRR,

+    /* These overlap may issue guest can't work well. */
+    if ( err )
+    {
+        printf("Guest can't work with some reserved device memory 
overlap!\n");
+        BUG();
+    }

As I understand, these fixed ranges don't like RAM that we can move 
safely out any RMRR overlap. And actually its rare to overlap with those 
fixed ranges.

But I can remove BUG if you insist on this point.

> only going to briefly look at the rest of this patch.
>
>> +static unsigned int construct_rdm_e820_maps(unsigned int next_e820_entry_index,
>> +                                            uint32_t nr_map,
>> +                                            struct
>> xen_mem_reserved_device_memory *map,
>> +                                            struct e820entry *e820,
>> +                                            unsigned int lowmem_reserved_base,
>> +                                            unsigned int bios_image_base)
>> +{
>> +    unsigned int i, j, sum_nr = next_e820_entry_index + nr_map;
>> +    uint64_t start, end, next_start, rdm_start, rdm_end;
>> +    uint32_t type;
>> +    unsigned int insert = 0, do_insert = 0;
>> +    int err = 0;
>> +
>> + do_real_construct:
>> +    for ( i = 0; i < nr_map; i++ )
>> +    {
>> +        rdm_start = map[i].start_pfn << PAGE_SHIFT;
>> +        rdm_end = rdm_start + (map[i].nr_pages << PAGE_SHIFT);
>> +
>> +        for ( j = 0; j < next_e820_entry_index - 1; j++ )
>> +        {
>> +            start = e820[j].addr;
>> +            end = e820[j].addr + e820[j].size;
>> +            type = e820[j].type;
>> +            next_start = e820[j+1].addr;
>> +
>> +            /* lowmem_reserved_base-0xA0000: reserved by BIOS implementation. */
>> +            if ( lowmem_reserved_base < 0xA0000 &&
>> +                 start == lowmem_reserved_base )
>> +            {
>> +                if ( rdm_start >= start && rdm_start <= end )
>> +                {
>> +                    err = -1;
>> +                    break;
>> +                }
>> +            }
>> +
>> +            /*
>> +             * BIOS region.
>> +             */
>> +            if ( start == bios_image_base )
>> +            {
>> +                if ( rdm_start >= start && rdm_start <= end )
>> +                {
>> +                    err = -1;
>> +                    break;
>> +                }
>> +            }
>> +
>> +            /* The default memory map always occupy one fixed reserved
>> +             * range: RESERVED_MEMBASE ~ 0x100000000
>> +             */
>> +            if ( rdm_start >= RESERVED_MEMBASE &&
>> +                      rdm_start <= ((uint64_t)1 << 32) )
>> +            {
>> +                err = -1;
>> +                break;
>> +            }
>> +
>> +            /* Just amid those remaining e820 entries. */
>> +            if ( (rdm_start > end) && (rdm_end < next_start) )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    memmove(&e820[j+2], &e820[j+1],
>> +                            (sum_nr - j - 1) * sizeof(struct e820entry));
>> +
>> +                    /* Then fill RMRR into that entry. */
>> +                    e820[j+1].addr = rdm_start;
>> +                    e820[j+1].size = rdm_end - rdm_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    next_e820_entry_index++;
>> +                }
>> +                insert++;
>> +            }
>> +            /* Already at the end. */
>> +            else if ( (rdm_start > end) && !next_start )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    e820[next_e820_entry_index].addr = rdm_start;
>> +                    e820[next_e820_entry_index].size = rdm_end - rdm_start;
>> +                    e820[next_e820_entry_index].type = E820_RESERVED;
>> +                    next_e820_entry_index++;
>> +                }
>> +                insert++;
>> +            }
>> +            /* If completely overlap with one RAM range. */
>> +            else if ( rdm_start == start && rdm_end == end && type == E820_RAM )
>
> Comment and expression disagree.

What about this?

/* If coincide with one RAM range. */

>
>> +            {
>> +                if ( do_insert )
>> +                    e820[j].type = E820_RESERVED;
>> +                insert++;
>> +            }
>> +            /* If we're just alligned with start of one RAM range. */
>> +            else if ( rdm_start == start && rdm_end < end && type == E820_RAM )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    memmove(&e820[j+1], &e820[j],
>> +                            (sum_nr - j) * sizeof(struct e820entry));
>> +
>> +                    e820[j+1].addr = rdm_end;
>> +                    e820[j+1].size = e820[j].addr + e820[j].size - rdm_end;
>> +                    e820[j+1].type = E820_RAM;
>> +                    next_e820_entry_index++;
>> +
>> +                    e820[j].addr = rdm_start;
>> +                    e820[j].size = rdm_end - rdm_start;
>> +                    e820[j].type = E820_RESERVED;
>> +                }
>> +                insert++;
>> +            }
>> +            /* If we're just alligned with end of one RAM range. */
>> +            else if ( rdm_start > start && rdm_end == end && type == E820_RAM )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    memmove(&e820[j+1], &e820[j],
>> +                            (sum_nr - j) * sizeof(struct e820entry));
>> +
>> +                    e820[j].size = rdm_start - e820[j].addr;
>> +                    e820[j].type = E820_RAM;
>> +
>> +                    e820[j+1].addr = rdm_start;
>> +                    e820[j+1].size = rdm_end - rdm_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    next_e820_entry_index++;
>> +                }
>> +                insert++;
>> +            }
>> +            /* If we're just in of one RAM range */
>> +            else if ( rdm_start > start && rdm_end < end && type == E820_RAM )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    memmove(&e820[j+2], &e820[j],
>> +                            (sum_nr - j) * sizeof(struct e820entry));
>> +
>> +                    e820[j+2].addr = rdm_end;
>> +                    e820[j+2].size = e820[j].addr + e820[j].size - rdm_end;
>> +                    e820[j+2].type = E820_RAM;
>> +                    next_e820_entry_index++;
>> +
>> +                    e820[j+1].addr = rdm_start;
>> +                    e820[j+1].size = rdm_end - rdm_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    next_e820_entry_index++;
>> +
>> +                    e820[j].size = rdm_start - e820[j].addr;
>> +                    e820[j].type = E820_RAM;
>> +                }
>> +                insert++;
>> +            }
>> +            /* If we're going last RAM:Hole range */
>> +            else if ( end < next_start &&
>> +                      rdm_start > start &&
>> +                      rdm_end < next_start &&
>> +                      type == E820_RAM )
>> +            {
>> +                if ( do_insert )
>> +                {
>> +                    memmove(&e820[j+1], &e820[j],
>> +                            (sum_nr - j) * sizeof(struct e820entry));
>> +
>> +                    e820[j].size = rdm_start - e820[j].addr;
>> +                    e820[j].type = E820_RAM;
>> +
>> +                    e820[j+1].addr = rdm_start;
>> +                    e820[j+1].size = rdm_end - rdm_start;
>> +                    e820[j+1].type = E820_RESERVED;
>> +                    next_e820_entry_index++;
>> +                }
>> +                insert++;
>> +            }
>
> This if-else-if series looks horrible - is there really no way to consolidate
> it? Also, other than punching holes in the E820 map you don't seem to

I know this is ugly but as you know there's no any rule we can make good 
use of this case. RMRR can start anywhere so We have to assume any 
scenarios,

1. Just amid those remaining e820 entries.
2. Already at the end.
3. If coincide with one RAM range.
4. If we're just aligned with start of one RAM range.
5. If we're just aligned with end of one RAM range.
6. If we're just in of one RAM range.
7. If we're going last RAM:Hole range.

So if you think we're handling correctly, maybe we can continue 
optimizing this way once we have a better idea.

> be doing anything here. And the earlier tools side patches didn't do
> anything about this either. Consequently, at the time where it may
> become necessary to establish the 1:1 mapping in the P2M, there'll
> be the RAM mapping still there, causing the device assignment to fail.

But I already set these range as p2m_access_n, and as you see I also 
reserved these range in e820 table. So although the RAM mapping still is 
still there but no any actual access.

Then when we assign device, we will override these p2m entry as 1:1 
mapping if p2m_access_n is set.

> Again - did you _test_ this scenario with a big enough guest and your
> gfx card passed through?

Yes. I validate these patches as following configuration:

memory = 2816
gfx_passthru=1
pci=["00:02.0"]

And I also perform 'xl dmesg' to check e820 table. Here I can provide an 
instance with overlap,

RMRR range:

root@tchen0-Shark-Bay-Client-platform:/home/tchen0/workspace# xl dmesg | 
grep RMRR
(XEN) [VT-D]dmar.c:834: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:679:   RMRR region: base_addr ab80a000 end_address 
ab81dfff
(XEN) [VT-D]dmar.c:834: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:679:   RMRR region: base_addr ad000000 end_address 
af7fffff
root@tchen0-Shark-Bay-Client-platform:/home/tchen0/workspace#

Without my patch:

(d4) E820 table:
(d4)  [00]: 00000000:00000000 - 00000000:0009e000: RAM
(d4)  [01]: 00000000:0009e000 - 00000000:000a0000: RESERVED
(d4)  HOLE: 00000000:000a0000 - 00000000:000e0000
(d4)  [02]: 00000000:000e0000 - 00000000:00100000: RESERVED
(d4)  [03]: 00000000:00100000 - 00000000:ab80a000: RAM
(d4)  [04]: 00000000:ab80a000 - 00000000:ab81e000: RESERVED
(d4)  [05]: 00000000:ab81e000 - 00000000:ad000000: RAM
(d4)  [06]: 00000000:ad000000 - 00000000:af800000: RESERVED
(d4)  HOLE: 00000000:af800000 - 00000000:fc000000
(d4)  [07]: 00000000:fc000000 - 00000001:00000000: RESERVED


With my patch:

(d2)  f0000-fffff: Main BIOS
(d2) E820 table:
(d2)  [00]: 00000000:00000000 - 00000000:0009e000: RAM
(d2)  [01]: 00000000:0009e000 - 00000000:000a0000: RESERVED
(d2)  HOLE: 00000000:000a0000 - 00000000:000e0000
(d2)  [02]: 00000000:000e0000 - 00000000:00100000: RESERVED
(d2)  [03]: 00000000:00100000 - 00000000:ab80a000: RAM
(d2)  [04]: 00000000:ab80a000 - 00000000:ab81e000: RESERVED
(d2)  [05]: 00000000:ab81e000 - 00000000:ad000000: RAM
(d2)  [06]: 00000000:ad000000 - 00000000:af800000: RESERVED
(d2)  HOLE: 00000000:af800000 - 00000000:fc000000
(d2)  [07]: 00000000:fc000000 - 00000000:fdffc000: RESERVED
(d2)  [08]: 00000000:fdffc000 - 00000000:fdfff000: NVS
(d2)  [09]: 00000000:fdfff000 - 00000001:00000000: RESERVED
(d2) Invoking ROMBIOS ...

Thanks
Tiejun