Re: [RFC PATCH 3/8]: PVH: memory manager and paging related changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 17 Aug 2012 10:26:15 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Thu, 2012-08-16 at 02:02 +0100, Mukesh Rathor wrote:
> > +
> >         if (HYPERVISOR_update_va_mapping((unsigned long)addr, pte,
> > 0)) BUG();
> >  }
> > @@ -1745,6 +1785,7 @@ static void convert_pfn_mfn(void *v)
> >   * but that's enough to get __va working.  We need to fill in the
> > rest
> >   * of the physical mapping once some sort of allocator has been set
> >   * up.
> > + * NOTE: for PVH, the page tables are native with HAP required.
> 
> OOI does this mean shadow doesn't work?

Most likely now. Will need to explore that. Later.


> > +
> > +       if (xen_pvh_domain()) {
> > +               pv_mmu_ops.flush_tlb_others = xen_flush_tlb_others;
> > +
> > +               /* set_pte* for PCI devices to map iomem. */
> > +               if (xen_initial_domain()) {
> > +                       pv_mmu_ops.set_pte = xen_dom0pvh_set_pte;
> > +                       pv_mmu_ops.set_pte_at =
> > xen_dom0pvh_set_pte_at;
> 
> Is this wrong for domU or have you just not tried/implemented
> passthrough support yet?

No, passthrough is phase II/III. Too big a project for one person to do in
a single shot as it is ;)..


> > + * exported function, so no need to export this.
> > + */
> > +static int pvh_add_to_xen_p2m(unsigned long lpfn, unsigned long
> > fgmfn,
> > +                             unsigned int domid)
> > +{
> > +       int rc;
> > +       struct xen_add_to_physmap pmb = {.foreign_domid = domid};
> 
> I'm not sure but I think CodingStyle would want spaces inside the {}s.
> 
> What is the b in pmb? Phys Map B??? (every other user of this
> interface says xatp, FWIW)

because originally it was a batch function but got changed after the
code review earlier. I will rename it.


> > HYPERVISOR_memory_op(XENMEM_remove_from_physmap, &xrp);
> > +               if (rc) {
> > +                       pr_warn("Failed to unmap pfn:%lx rc:%d
> > done:%d\n",
> > +                               spfn+i, rc, i);
> > +                       return 1;
> > +               }
> > +       }
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pvh_rem_xen_p2m);
> 
> What is the external/modular user of this?

privcmd.

> I guess this is why you noted that pvh_add_to_xen_p2m didn't need
> exporting, which struck me as unnecessary at the time.
> 
> pvh_add_to_xen_p2m seems to have an exported a wrapper, why not rem
> too? e.g. xen_unmap_domain_mfn_range?

I believe unmapping happens thru native code. So, we just need to remove 
the entries from the ept. Ok, I mean, hap. 

> > +
> > +       native_set_pte(ptep, pteval);
> > +       if (pvh_add_to_xen_p2m(pfn, pvhp->fgmfn, pvhp->domid))
> > +               return -EFAULT;
> 
> Is there a little window here where we've setup the page table entry
> but the p2m entry behind it is uninitialised?
> 
> I suppose even if an interrupt occurred we can rely on the virtual
> address not having been "exposed" anywhere yet and therefore there is
> no chance of anyone dereferencing it. But is there any harm in just
> flipping the ordering here?

Should be ok flippiong the order.

> Why EFAULT? Seems a bit random, I think HYPERVISOR_memory_op returns a
> -ve errno which you could propagate.

Ok, sounds good.