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

Re: [Xen-devel] [PATCH v2 02/21] xen/arm: make allocate_memory work for non 1:1 mapped guests

Hi Stefano,

On 10/07/18 00:02, Stefano Stabellini wrote:
On Mon, 9 Jul 2018, Julien Grall wrote:
On 07/07/18 00:11, Stefano Stabellini wrote:
       mfn_t smfn;
       paddr_t start, size;
+    struct membank *bank;
         smfn = page_to_mfn(pg);
       start = mfn_to_maddr(smfn);

The new code is pretty horrible to read. Can you please add some comments to
help understanding it?

Here is already an example where you set start to MFN. But then override after
with none comment to understand why.

Also, this code as always assumed MFN == GFN so start was making sense. Now,
you re-purpose it to just the guest-physical address. So more likely you want
to rework the code a bit.

I'll add more comments in the code. Next time the patch will be clearer.
This is a snippet to give you an idea, but it might be best for you to
just wait for the next version before reading this again.

      * smfn: the address of the set of pages to map
      * start: the address in guest pseudo-physical memory where to map
      *        the pages

The best way is to rename start to gaddr or better provide a frame. So there are no need for such self-explanatory comment. However, my main issue was not the name itself...

     smfn = page_to_mfn(pg);
     start = mfn_to_maddr(smfn);

... but this specific line. This should have been in a else.

     size = pfn_to_paddr(1UL << order);
     if ( !is_hardware_domain(d) )

Why is_hardware_domain(d)? None of the code below is assuming it is an hardware domain and we should not assume the 1:1 mapping. That was the exact reason of the BUG_ON(!is_domain_direct_mapped(d)) in the caller and not !is_hardware_domain(d).

          * Dom0 is 1:1 mapped, so start is the same as (smfn <<
          * PAGE_SHIFT).

This comment is misplaced.

          * Instead, DomU memory is provided in two banks:

Why instead? The comment should be split.

          * Find the right start address for DomUs accordingly.

       size = pfn_to_paddr(1UL << order);
+    if ( !map_11 )

I am not sure why map_11 would mean DomU? I don't see any reason to not allow
that for Dom0. Note that I am not asking to do it, just having clearer name.

Good point. I think I should just drop the option, which is just
confusing, and keep using is_hardware_domain(d) checks like in
allocate_memory. Otherwise let me know if you have a better idea.

TBH, I dislike the way you re-purpose the 2 functions. 80% of this code is not necessary because you will never merge the range before the bank or deal with 1:1 mappings.

Furthermore, I just spotted a few issues with that code:
1) If you request 4GB for a guest and the memory has been allocated in one chunk, all the RAM will be starting at GUEST_RAM1_SIZE. While we officially don't support guest with hardcoded memory layout, there are some existing. Such change will break them depending on your memory layout at boot. 2) If in the future we decide to add more banks (this may happen with PCI passthrough), then you have to add yet another if.

What is the problem to provide a separate function to allocate memory for non-direct domain? You could just pass the base and the size of the region to populate.

+    {
+        start = GUEST_RAM0_BASE;
+        if ( kinfo->mem.nr_banks > 0 )
+        {
+            for( i = 0; i < kinfo->mem.nr_banks; i++ )
+            {
+                bank = &kinfo->mem.bank[i];
+                start = bank->start + bank->size;
+            }
+            if ( bank->start == GUEST_RAM0_BASE &&
+                    start + size > (GUEST_RAM0_BASE + GUEST_RAM0_SIZE) )

The indentation looks wrong.


+                start = GUEST_RAM1_BASE;
+            if ( bank->start == GUEST_RAM1_BASE &&
+                    start + size > (GUEST_RAM1_BASE + GUEST_RAM1_SIZE) )
+            {
+                D11PRINT("Allocation of domain memory exceeds max

This looks quite strange to use D11PRINT here as this related to direct-domain
You are right, but I didn't feel like replacing all D11PRINT
occurrences. Should I do that? Or should I change just this instance? I
could also just drop this D11PRINT, given that the printk is not even
enabled by default. Let me know.

But then, the D11PRINT below does not make any sense after your change. We should really aim to keep that code fairly sane. So I would rename D11PRINT unless you are doing what I suggested above.


Julien Grall

Xen-devel mailing list



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