Re: [Xen-devel] [PATCH v3 3/9] xen/mm: move modify_identity_mmio to global file and drop __init

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, May 19, 2017 at 07:35:39AM -0600, Jan Beulich wrote:
> >>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> wrote:
> > And also allow it to do non-identity mappings by adding a new parameter. 
> > This
> > function will be needed in other parts apart from PVH Dom0 build. While 
> > there
> > fix the function to use gfn_t and mfn_t instead of unsigned long for memory
> > addresses.
> 
> I'm afraid both title and description don't (or no longer) properly reflect
> what the patch does. I'm also afraid the reason the new parameter as
> well as the placement in common/memory.c aren't sufficiently explained.
> For example, what use is the function going to be without
> CONFIG_HAS_PCI?

It will still be needed in order to map the low 1MB for a PVH Dom0,
but anyway, see below.

> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -64,27 +64,7 @@ static struct acpi_madt_nmi_source __initdata *nmisrc;
> >  static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
> >                                         unsigned long nr_pages, const bool 
> > map)
> >  {
> > -    int rc;
> > -
> > -    for ( ; ; )
> > -    {
> > -        rc = (map ? map_mmio_regions : unmap_mmio_regions)
> > -             (d, _gfn(pfn), nr_pages, _mfn(pfn));
> > -        if ( rc == 0 )
> > -            break;
> > -        if ( rc < 0 )
> > -        {
> > -            printk(XENLOG_WARNING
> > -                   "Failed to identity %smap [%#lx,%#lx) for d%d: %d\n",
> > -                   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> > -            break;
> > -        }
> > -        nr_pages -= rc;
> > -        pfn += rc;
> > -        process_pending_softirqs();
> > -    }
> > -
> > -    return rc;
> > +    return modify_mmio(d, _gfn(pfn), _mfn(pfn), nr_pages, map);
> >  }
> 
> I don't see the value of retaining this wrapper.
> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1438,6 +1438,42 @@ int prepare_ring_for_helper(
> >      return 0;
> >  }
> >  
> > +int modify_mmio(struct domain *d, gfn_t gfn, mfn_t mfn, unsigned long 
> > nr_pages,
> > +                const bool map)
> > +{
> > +    int rc;
> > +
> > +    /*
> > +     * Make sure this function is only used by the hardware domain, 
> > because it
> > +     * can take an arbitrary long time, and could DoS the whole system.
> > +     */
> > +    ASSERT(is_hardware_domain(d));
> 
> If that can happen arbitrarily at run time (rather than just at boot,
> as suggested by the removal of __init), it definitely can't remain as
> is and will instead need to make use of continuations. I'm therefore
> unconvinced you really want to move this code instead of simply
> calling {,un}map_mmio_regions() while taking care of preemption
> needs.

I'm not sure I know how to use continuations with non-hypercall
vmexits. Do you have any recommendations about how to do this? pause
the domain and run the mmio changes inside of a tasklet?

Roger.

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