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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>> 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? 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 also wonder whether it wouldn't be worthwhile to avoid calling
>> modify_identity_mmio() for RAM ranges (which are now going to be
>> re-mapped anyway).
> 
> I think it's easier (code-wise) to identity map the whole area and
> then just populate the RAM regions as needed.

Well, okay then.

Jan



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