Re: [Xen-devel] [PATCH] x86/pvh: copy data from low 1MB to Dom0 physmap instead of mapping it

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 17, 2018 at 10:15:11AM -0600, Jan Beulich wrote:
> >>> On 17.09.18 at 15:37, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Sep 17, 2018 at 07:03:27AM -0600, Jan Beulich wrote:
> >> >>> On 14.09.18 at 13:16, <roger.pau@xxxxxxxxxx> wrote:
> >> > @@ -420,16 +393,24 @@ static int __init pvh_setup_p2m(struct domain *d)
> >> >          addr = PFN_DOWN(d->arch.e820[i].addr);
> >> >          size = PFN_DOWN(d->arch.e820[i].size);
> >> >  
> >> > -        if ( addr >= MB1_PAGES )
> >> > -            rc = pvh_populate_memory_range(d, addr, size);
> >> > -        else
> >> > -        {
> >> > -            ASSERT(addr + size < MB1_PAGES);
> >> > -            pvh_steal_low_ram(d, addr, size);
> >> > -        }
> >> > -
> >> > +        rc = pvh_populate_memory_range(d, addr, size);
> >> >          if ( rc )
> >> >              return rc;
> >> > +
> >> > +        if ( addr < MB1_PAGES )
> >> > +        {
> >> > +             enum hvm_translation_result res =
> >> > +                 hvm_copy_to_guest_phys(mfn_to_maddr(_mfn(addr)),
> >> > +                                        mfn_to_virt(addr), size << 
> > PAGE_SHIFT,
> >> > +                                        v);
> >> > +
> >> > +            if ( res != HVMTRANS_okay )
> >> > +            {
> >> > +                printk("Failed to copy [%#lx, %#lx): %d\n",
> >> > +                       addr, addr + size, res);
> >> > +                return -EFAULT;
> >> > +            }
> >> > +        }
> >> >      }
> >> 
> >> Is there any guarantee (in particular on, but not limited to EFI systems)
> >> for E820_RAM regions to never span the 1Mb boundary? If not, you
> >> may end up copying memory above 1Mb here.
> > 
> > Right, I guess I could do something like:
> > 
> > end = min(MB(1), d->arch.e820[i].addr + d->arch.e820[i].size);
> > 
> > And calculate the size based on the 'end' value.
> 
> Right.

Let me fix this and resend.

> 
> >> Furthermore, what about RAM / non-RAM boundaries in the middle of
> >> a page (which is quite common a situation for the first Mb)?
> > 
> > There are no such RAM ranges in the guest memory map because
> > pvh_setup_e820 aligns the RAM regions start/end to page boundaries.
> 
> Oh, right.
> 
> > This is not ideal, so if you want I can do the following:
> > 
> > hvm_copy_to_guest_phys(d->arch.e820[i].addr, d->arch.e820[i].size, v);
> > 
> > And if pvh_setup_e820 is improved so that RAM regions are no longer
> > aligned to page boundaries the copy will work without issues.
> 
> Hmm, I guess I'm having difficulty understanding what you think the
> goal ought to be: Would a page part of which is RAM be copied or
> mapped in your opinion?

IMO start of RAM ranges should be ceiled, and the end should be
floored so that the resulting range is the same size or smaller, but
never greater than the original RAM range.

For example a RAM range like [0x10, 0x2001) will end up as [0x1000,
0x2000), so a page that's only partially RAM loses the RAM part in
pvh_setup_e820.

> I think only mapping can possibly work
> reliably, and with your remark about pvh_setup_e820() I then think
> no change would be needed.

I agree, only identity mapping can work with such pages.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel