Re: [Xen-devel] [PATCH v3.1 15/15] xen/x86: setup PVHv2 Dom0 ACPI tables

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, Nov 30, 2016 at 07:09:47AM -0700, Jan Beulich wrote:
> >>> On 30.11.16 at 13:40, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Nov 14, 2016 at 09:15:37AM -0700, Jan Beulich wrote:
> >> >>> On 29.10.16 at 11:00, <roger.pau@xxxxxxxxxx> wrote:
> >> > Also, regions marked as E820_ACPI or E820_NVS are identity mapped into 
> >> > Dom0
> >> > p2m, plus any top-level ACPI tables that should be accessible to Dom0 and
> >> > that don't reside in RAM regions. This is needed because some memory maps
> >> > don't properly account for all the memory used by ACPI, so it's common to
> >> > find ACPI tables in holes.
> >> 
> >> I question whether this behavior should be enabled by default. Not
> >> having seen the code yet I also wonder whether these regions
> >> shouldn't simply be added to the guest's E820 as E820_ACPI, which
> >> should then result in them getting mapped without further special
> >> casing.
> >> 
> >> > +static int __init hvm_add_mem_range(struct domain *d, uint64_t s, 
> >> > uint64_t e,
> >> > +                                    uint32_t type)
> >> 
> >> I see s and e being uint64_t, but I don't see why type can't be plain
> >> unsigned int.
> > 
> > Well, that's the type for "type" as defined in e820.h. I'm just using 
> > uint32_t 
> > for consistency with that.
> 
> As said a number of times in various contexts: We should try to
> get away from using fixed width types where we don't really need
> them.

Done, I've changed it. Would you like me to also change the uint64_t's to 
paddr_t?

> >> > +        {
> >> > +            d->arch.e820[i].size += e - s;
> >> > +            return 0;
> >> > +        }
> >> > +
> >> > +        if ( rs >= e )
> >> > +            break;
> >> > +
> >> > +        if ( re > s )
> >> > +            return -ENOMEM;
> >> 
> >> I don't think ENOMEM is appropriate to signal an overlap. And don't
> >> you need to reverse these last two if()s?
> > 
> > I've changed ENOMEM to EEXIST. Hm, I don't think so, if I reversed those we 
> > will 
> > get error when trying to add a non-contiguous region to fill a hole between 
> > two 
> > existing regions right?
> 
> Looks like I've managed to write something else than I meant. I was
> really thinking of
> 
>         if ( re > s )
>         {
>             if ( rs >= e )
>                 break;
>             return -ENOMEM;
>         }
> 
> But then again I think with things being sorted it may not matter at all.

I slightly prefer the current one since it has less nested ifs, but if you have 
a strong preference for the later I don't really mind changing it.

> >> > +    if ( nr_ioapics > 1 )
> >> > +        printk("WARNING: found %d IO APICs, Dom0 will only have access 
> >> > to 1 emulated IO APIC\n",
> >> > +               nr_ioapics);
> >> 
> >> I've said elsewhere already that I think we should provide 1 vIO-APIC
> >> per physical one.
> > 
> > Agree, but the current vIO-APIC is not really up to it. I will work on 
> > getting 
> > it to support multiple instances.
> 
> Until then this should obtain a grep-able "fixme" annotation.

Oh, right (you said that several times, sorry).
 
Roger.

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