Re: [Xen-devel] [PATCH v6 3/7] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.02.17 at 11:10, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, Feb 13, 2017 at 06:53:49AM -0700, Jan Beulich wrote:
>> >>> On 10.02.17 at 13:33, <roger.pau@xxxxxxxxxx> wrote:
>> > +static int __init pvh_steal_ram(struct domain *d, unsigned long size,
>> > +                                unsigned long align, paddr_t limit,
>> > +                                paddr_t *addr)
>> > +{
>> > +    unsigned int i = d->arch.nr_e820;
>> > +
>> > +    /*
>> > +     * Alignment 0 should be set to 1, so it doesn't wrap around in the
>> > +     * calculations below.
>> > +     */
>> > +    align = align ? : 1;
>> > +    while ( i-- )
>> > +    {
>> > +        struct e820entry *entry = &d->arch.e820[i];
>> > +
>> > +        if ( entry->type != E820_RAM || entry->addr + entry->size > limit 
>> > ||
>> > +             entry->addr < MB(1) )
>> > +            continue;
>> > +
>> > +        *addr = (entry->addr + entry->size - size) & ~(align - 1);
>> 
>> Without taking the present callers into account (who don't request
>> huge alignment) this (theoretically) risks running into the low 1Mb.
>> I see two options - either add a comment clarifying that an entry
>> will never cross the 1Mb boundary (and hence the issue can't
>> occur), or limit the alignment (by way of ASSERT()) to PAGE_SIZE
>> (in which case no significant harm would result from crossing the
>> boundary).
> 
> I don't mind adding the ASSERT, but I don't see how this can risk running into
> the low 1MB. If entry->addr < MB(1) the entry is skipped. If an entry crosses
> the 1Mb boundary it will certainly have entry->addr < 1Mb.

Oh, of course. I'm sorry for the noise (and the code here is fine
then as is).

>> > --- a/xen/include/asm-x86/page.h
>> > +++ b/xen/include/asm-x86/page.h
>> > @@ -374,6 +374,18 @@ perms_strictly_increased(uint32_t old_flags, uint32_t 
>> > new_flags)
>> >      return ((of | (of ^ nf)) == nf);
>> >  }
>> >  
>> > +/* Build a 32bit PSE page table using 4MB pages. */
>> > +static inline void
>> > +write_32bit_pse_identmap(uint32_t *l2)
>> 
>> Why does this need to be an inline function?
> 
> Given it's size and the low number of callers I though it would be better to
> make it inline, but since this is not in any performance critical path I'm
> going to remove the inlining, although I think the compiler is probably going
> to do it anyway.

I don't think so, unless you use LTO, as the function body can be
visible in at most one of the two CUs containing a call to it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel