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

[Julien Grall <julien.grall@xxxxxxx>]

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.

>           *   GUEST_RAM0_BASE - GUEST_RAM0_BASE + GUEST_RAM0_SIZE
>           *   GUEST_RAM1_BASE - GUEST_RAM1_BASE + GUEST_RAM1_SIZE
>           *
>           * 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.
> OK
>
>
> > > +                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
> > > amount\n");
> > This looks quite strange to use D11PRINT here as this related to direct-domain
> > mapped.
>   
> 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.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel