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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.18 at 10:55, <roger.pau@xxxxxxxxxx> wrote:
> @@ -399,7 +372,8 @@ static int __init pvh_setup_p2m(struct domain *d)
>      } while ( preempted );
>  
>      /*
> -     * Memory below 1MB is identity mapped.
> +     * Memory below 1MB is identity mapped except RAM regions that are
> +     * populated and copied below.
>       * NB: this only makes sense when booted from legacy BIOS.
>       */
>      rc = modify_identity_mmio(d, 0, MB1_PAGES, true);

Mind making the comment even less ambiguous, e.g.

     * Memory below 1MB is identity mapped initially. RAM regions are
     * populated and copied below, replacing the respective mappings.

?

> @@ -420,16 +394,27 @@ 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 )
> +        {
> +            uint64_t end = min_t(uint64_t, MB(1),
> +                                 d->arch.e820[i].addr + 
> d->arch.e820[i].size);

I think min_t() and max_t() should only be used as a last resort, due
to their (hidden) casts. min(MB(1ULL), ...) ought to be fine here, I
would think.

> +            enum hvm_translation_result res =
> +                 hvm_copy_to_guest_phys(mfn_to_maddr(_mfn(addr)),
> +                                        mfn_to_virt(addr),
> +                                        d->arch.e820[i].addr - end,
> +                                        v);
> +
> +            if ( res != HVMTRANS_okay )
> +            {
> +                printk("Failed to copy [%#lx, %#lx): %d\n",
> +                       addr, addr + size, res);
> +                return -EFAULT;

I think this would better be logged only - there's no reason to believe
Dom0 wouldn't, in the common case, come up at least in a state
allowing investigation of a problem here. (Not that I think the copying
might fail in the first place, but anyway.)

Jan



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