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 06:49:42AM -0700, Jan Beulich wrote:
> >>> On 28.11.16 at 14:30, <roger.pau@xxxxxxxxxx> wrote:
> > 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:
> >> >> > +            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?).
> 
> Well, as implied before: If there's provably no harm to debuggability,
> then perhaps there's no real need for you to change your code. If,
> however, there remains any doubt, then I specifically suggested
> that override variant, knowing that handing a proper struct vcpu *
> or struct domain * to the function would likely end up touching a lot
> of code.

Hm, I don't really see any harm ATM, but I'm also wondering whether I should 
add 
a helper that does all this, there are multiple instances of the 
set_current/hvm_copy/set_current construct throughout the series, so adding a 
hvm_copy_to_phys seems quite sensible.

Roger.

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