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

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 10 Jul 2018, Julien Grall wrote:
> 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...

Sure, I can do


> >       */
> >      smfn = page_to_mfn(pg);
> >      start = mfn_to_maddr(smfn);
> 
> ... but this specific line. This should have been in a else.

This goes away with having separate functions for DomUs


> >      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).

Yeah, I should have used is_domain_direct_mapped. This also goes away
with having separate functions.


> >      {
> >          /*
> >           * 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.

OK


> >           *   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.

I have introduced two separate functions now, I am not so sure it's an
actual improvement.


> 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.

I fixed this.


> 	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.

You'll see the new functions in the next series. I think there is more
than 20% in common with the older functions. Anyhow, you'll have a
chance to comment on them on the next series.

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