Re: [Xen-devel] [PATCH v3.1 12/15] xen/x86: populate PVHv2 Dom0 physical memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.11.16 at 12:26, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Nov 11, 2016 at 10:16:43AM -0700, Jan Beulich wrote:
>> >>> On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> wrote:
>> > +static int __init hvm_steal_ram(struct domain *d, unsigned long size,
>> > +                                paddr_t limit, paddr_t *addr)
>> > +{
>> > +    unsigned int i;
>> > +
>> > +    for ( i = 1; i <= d->arch.nr_e820; i++ )
>> > +    {
>> > +        struct e820entry *entry = &d->arch.e820[d->arch.nr_e820 - i];
>> 
>> Why don't you simply make the loop count downwards?
> 
> With i being an unsigned int, this would make the condition look quite 
> awkward, 
> because i >= 0 cannot be used. I would have to use i <= d->arch.nr_e820, so I 
> think it's best to leave it as-is for readability.

What's wrong with

    i = d->arch.nr_e820;
    while ( i-- )
    {
        ...

(or its for() equivalent)?

>> > +            saved_current = current;
>> > +            set_current(v);
>> > +            rc = hvm_copy_to_guest_phys(d->arch.e820[i].addr,
>> > +                                        
>> > maddr_to_virt(d->arch.e820[i].addr),
>> > +                                        end - d->arch.e820[i].addr);
>> > +            set_current(saved_current);
>> 
>> If anything goes wrong here, how much confusion will result from
>> current being wrong? In particular, will this complicate debugging
>> of possible issues?
> 
> TBH, I'm not sure, current in this case is the idle domain, so trying to 
> execute 
> a hvm_copy_to_guest_phys with current being the idle domain, which from a Xen 
> PoV is a PV vCPU, would probably result in some assert triggering in the 
> hvm_copy_to_guest_phys path (or at least I would expect so). Note that this 
> chunk of code is removed, since RAM regions below 1MB are now mapped as 
> p2m_ram_rw.

Even if this chunk of code no longer exists, iirc there  were a few
more instances of this current overriding, so unless they're all gone
now I still think this need considering (and ideally finding a better
solution, maybe along the lines of mapcache_override_current()).

Jan


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