Re: [Xen-devel] [PATCH v2 01/10] xen: add a p2mt parameter to map_mmio_regions

[Stefano Stabellini <sstabellini@kernel.org>]

On Thu, 2 May 2019, Jan Beulich wrote:
> >>> On 30.04.19 at 23:02, <sstabellini@kernel.org> wrote:
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -79,8 +79,11 @@ static int __init modify_identity_mmio(struct domain *d, unsigned long pfn,
> >  
> >      for ( ; ; )
> >      {
> > -        rc = map ?   map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn))
> > -                 : unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
> > +        if ( map )
> > +            rc = map_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn),
> > +                                  p2m_mmio_direct);
> > +        else
> > +            rc = unmap_mmio_regions(d, _gfn(pfn), nr_pages, _mfn(pfn));
> 
> May I ask that you leave alone the use of the conditional
> operator here, and _just_ add the new argument?

Yes, I can do that. This change is due to the way it was done in the
previous version of the series, it doesn't make sense anymore.


> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -2264,12 +2264,16 @@ static unsigned int mmio_order(const struct domain *d,
> >  int map_mmio_regions(struct domain *d,
> >                       gfn_t start_gfn,
> >                       unsigned long nr,
> > -                     mfn_t mfn)
> > +                     mfn_t mfn,
> > +                     p2m_type_t p2mt)
> >  {
> >      int ret = 0;
> >      unsigned long i;
> >      unsigned int iter, order;
> >  
> > +    if ( p2mt != p2m_mmio_direct )
> > +        return -EOPNOTSUPP;
> 
> Considering this and ...
> 
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -927,6 +927,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >          unsigned long nr_mfns = op->u.memory_mapping.nr_mfns;
> >          unsigned long mfn_end = mfn + nr_mfns - 1;
> >          int add = op->u.memory_mapping.add_mapping;
> > +        p2m_type_t p2mt;
> >  
> >          ret = -EINVAL;
> >          if ( mfn_end < mfn || /* wrap? */
> > @@ -939,6 +940,10 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >          /* Must break hypercall up as this could take a while. */
> >          if ( nr_mfns > 64 )
> >              break;
> > +
> > +        p2mt = p2m_mmio_direct_dev;
> > +#else
> > +        p2mt = p2m_mmio_direct;
> >  #endif
> 
> ... this, is there really value in adding the new parameter for
> x86? A wrapper macro of the same name could be used to
> strip the new last argument at all call sites (current and future
> ones).
 
Sure, no problem.

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