Re: [Xen-devel] [PATCH v2 15/30] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.16 at 16:06, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Sep 30, 2016 at 09:52:56AM -0600, Jan Beulich wrote:
>> >>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
>> > +     * VM86 TSS. Note that after this not all e820 regions will be aligned
>> > +     * to PAGE_SIZE.
>> > +     */
>> > +    for ( i = 1; i <= d->arch.nr_e820; i++ )
>> > +    {
>> > +        entry = &d->arch.e820[d->arch.nr_e820 - i];
>> > +        if ( entry->type != E820_RAM ||
>> > +             entry->size < PAGE_SIZE + HVM_VM86_TSS_SIZE )
>> > +            continue;
>> > +
>> > +        entry->size -= PAGE_SIZE + HVM_VM86_TSS_SIZE;
>> > +        gaddr = entry->addr + entry->size;
>> > +        break;
>> > +    }
>> > +
>> > +    if ( gaddr == 0 || gaddr < MB(1) )
>> > +    {
>> > +        printk("Unable to find memory to stash the identity map and 
>> > TSS\n");
>> > +        return -ENOMEM;
>> 
>> One function up you panic() on error - please be consistent. Also for
>> one of the other patches I think we figured that the TSS isn't really
>> required, so please only warn in that case.
> 
> The allocation is done together for the ident PT and the TSS, and while 
> the TSS is not mandatory the identity page-table is (or else Dom0 kernel 
> won't boot at all on this kind of systems). In any case, it's almost 
> impossible for this allocation to fail (because Xen is not actually 
> allocating memory, just stealing a part of a RAM region that has already 
> been populated).

All fine, but I continue to think errors should be dealt with in a
consistent manner (no matter how [un]likely they are), and
warnings better get issued when there's any meaningful impact
due to whatever triggers the warning. Albeit - having looked at
the full patch again - it looks like I was wrong with naming this
"warning": Both here and further down you return an error if any
of the steps failed. The allocation being done in one go is fine to
be an error; failure of the mapping of the non-required TSS, otoh,
shouldn't cause the loading of Dom0 to fail (and I think that is
where I've been unsure the printk() is warranted).

Jan


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