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

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

On Mon, Apr 24, 2017 at 03:42:08PM +0100, Julien Grall wrote:
> On 20/04/17 16:17, Roger Pau Monne wrote:
> >  /* Populate a HVM memory range using the biggest possible order. */
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 52879e7438..0d970482cb 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -1438,6 +1438,40 @@ int prepare_ring_for_helper(
> >      return 0;
> >  }
> > 
> > +int modify_mmio(struct domain *d, unsigned long gfn, unsigned long pfn,
> 
> Whilst you introduce this new function, please use mfn_t and gfn_t.
> 
> Also s/pfn/mfn/

Done.

> > +                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));
> 
> What would be the plan for guest if we decide to use vpci?

One option would be to not allow the DomU to relocate it's BARs and ignore
writes to the 2nd bit of the command register (PCI_COMMAND_MEMORY), thus always
having the BARs mapped. The other is to somehow allow VMExit (and the ARM
equivalent) continuation (something similar to what we do with hypercalls).

> > +
> > +    for ( ; ; )
> > +    {
> > +        rc = (map ? map_mmio_regions : unmap_mmio_regions)
> 
> On ARM, map_mmio_regions and unmap_mmio_regions will map the MMIO with very
> strict attribute. I think we would need an extra argument to know the wanted
> memory attribute (maybe p2m_type_t?).

I'm not sure I can do anything regarding this ATM. Sorry for my ignorance, but
map_mmio_regions on ARM maps the region as p2m_mmio_direct_dev, and according
to the comment that's "Read/write mapping of genuine Device MMIO area", which
fits exactly into my usage (I'm using it to map BARs).

> > +             (d, _gfn(gfn), nr_pages, _mfn(pfn));
> > +        if ( rc == 0 )
> > +            break;
> > +        if ( rc < 0 )
> > +        {
> > +            printk(XENLOG_WARNING
> 
> I would probably use XENLOG_G_WARNING.

Done.

> > +                   "Failed to %smap [%#lx, %#lx) -> [%#lx,%#lx) for d%d: 
> > %d\n",
> > +                   map ? "" : "un", gfn, gfn + nr_pages, pfn, pfn + 
> > nr_pages,
> > +                   d->domain_id, rc);
> > +            break;
> > +        }
> > +        nr_pages -= rc;
> > +        pfn += rc;
> > +        gfn += rc;
> > +        process_pending_softirqs();
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> > index 8cd5a6b503..1308da44e7 100644
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -13,4 +13,8 @@ int unmap_mmio_regions(struct domain *d,
> >                         unsigned long nr,
> >                         mfn_t mfn);
> > 
> > +
> 
> Spurious newline.

Done.

Thanks for the comments.

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