RE: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Penny Zheng <Penny.Zheng@xxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, August 17, 2021 1:55 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory
> 
> 
> 
> On 16/08/2021 08:51, Penny Zheng wrote:
> >>> +               d, bank, pbase, pbase + psize);
> >>> +
> >>> +        /*
> >>> +         * It shall be mapped to the fixed guest RAM address rambase[i],
> >>> +         * And until it exhausts the ramsize[i], it will seek to the next
> >>> +         * rambase[i+1].
> >>> +         */
> >>> +        while ( 1 )
> >>> +        {
> >>> +            /*
> >>> +             * The current physical bank is fully mapped.
> >>> +             * Handle the next physical bank.
> >>> +             */
> >>> +            if ( gsize >= psize )
> >>> +            {
> >>> +                if ( !append_static_memory_to_bank(d, &kinfo-
> >mem.bank[gbank],
> >>> +                                                   smfn, psize) )
> >>> +                    goto fail;
> >>> +
> >>> +                gsize = gsize - psize;
> >>> +                bank++;
> >>> +                break;
> >>> +            }
> >>> +            /*
> >>> +             * Current guest bank memory is not enough to map.
> >>> +             * Check if we have another guest bank available.
> >>> +             * gbank refers guest memory bank index.
> >>> +             */
> >>> +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {
> >>
> >> I don't understand the +2. Can you clarify it?
> >>
> >
> > gbank refers to the index of the guest bank, and here since current guest
> bank(gbank)
> >   memory is not enough to map, users seeks to the next one(gbank + 1),
> >
> > gbank + 2 is the number of requested guest memory banks right now, and
> > shall not be larger than GUEST_RAM_BANKS.
> 
> Thanks for the clarification. When I read "2" I tend to think we are checking
> the bank after the next. How about writing:
> 
> (gbank + 1) >= GUEST_RAM_BANKS
> 
> or
> 
> gbank >= (GUEST_RAM_BANKS - 1)
> 
> This as the same end results, but we check the index rather than the number of
> banks.
> 
> Anyway, I can settle with 2 if you really prefer it.
> 

Nah, I'll take your suggestion. Yours is more generic.
 
> Cheers,
> 
> --
> Julien Grall