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

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

On Mon, Nov 28, 2016 at 04:41:22AM -0700, Jan Beulich wrote:
> >>> 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)?

Nothing, I guess it's Monday...

> >> > +            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()).

This could be solved by making __hvm_copy take a struct domain param, but this 
is a very big change. I could also try to fix __hvm_copy so that we can set an 
override vcpu, much like mapcache_override_current (hvm_override_current?).

Roger.

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