Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi,
> 
> On 6/19/19 12:20 AM, Stefano Stabellini wrote:
> > Reuse the existing padding field to pass memory policy information. On
> > Arm, the caller can specify whether the memory should be mapped as
> > Device-nGnRE (Device Memory on Armv7) at stage-2, which is the default
> > and the only possibility today, or cacheable memory write-back. The
> > resulting memory attributes will be a combination of stage-2 and stage-1
> > memory attributes: it will actually be the strongest between the 2
> > stages attributes.
> > 
> > On x86, the only option is uncachable. The current behavior becomes the
> > default (numerically '0'). Also explicitely set the memory_policy field
> > to 0 in libxc.
> > 
> > On ARM, map Device-nGnRE as p2m_mmio_direct_dev (as it is already done
> 
> s/ARM/Arm/

OK


> > today) and WB cacheable memory as p2m_mmio_direct_c.
> > 
> > On x86, return error if the memory policy requested is not
> > MEMORY_POLICY_X86_UC_MINUS.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > CC: JBeulich@xxxxxxxx
> > CC: andrew.cooper3@xxxxxxxxxx
> > 
> > ---
> > 
> > Andrew suggested to remove MEMORY_POLICY_X86_UC_MINUS completely.
> > If that's the consensus I am happy to respin the series removing code.
> > 
> > 
> > Changes in v3:
> > - error handling in default label of the switch
> > - set memory_policy to 0 in libxc
> > - improve commit message
> > - improve comments
> > - s/Device-nGRE/Device-nGnRE/g
> > - add in-code comment
> > - s/MEMORY_POLICY_X86_UC/MEMORY_POLICY_X86_UC_MINUS/g
> > - #ifdef hypercall defines according to arch
> > 
> > Changes in v2:
> > - rebase
> > - use p2m_mmio_direct_c
> > - use EOPNOTSUPP
> > - rename cache_policy to memory policy
> > - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE
> > - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB
> > - add MEMORY_POLICY_X86_UC
> > - add MEMORY_POLICY_DEFAULT and use it
> > ---
> >   tools/libxc/xc_domain.c     |  1 +
> >   xen/common/domctl.c         | 24 ++++++++++++++++++++++--
> >   xen/include/public/domctl.h | 23 ++++++++++++++++++++++-
> >   3 files changed, 45 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 05d771f2ce..8531298563 100644
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -2070,6 +2070,7 @@ int xc_domain_memory_mapping(
> >       domctl.cmd = XEN_DOMCTL_memory_mapping;
> >       domctl.domain = domid;
> >       domctl.u.memory_mapping.add_mapping = add_mapping;
> > +    domctl.u.memory_mapping.memory_policy = 0;
> >       max_batch_sz = nr_mfns;
> >       do
> >       {
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index c6fd88d285..f21f6957b0 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -928,6 +928,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
> > u_domctl)
> >           unsigned long mfn_end = mfn + nr_mfns - 1;
> >           int add = op->u.memory_mapping.add_mapping;
> >           p2m_type_t p2mt;
> > +        uint32_t memory_policy = op->u.memory_mapping.memory_policy;
> >             ret = -EINVAL;
> >           if ( mfn_end < mfn || /* wrap? */
> > @@ -958,9 +959,28 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
> > u_domctl)
> >           if ( add )
> >           {
> >               printk(XENLOG_G_DEBUG
> > -                   "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
> > -                   d->domain_id, gfn, mfn, nr_mfns);
> > +                   "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx
> > cache=%u\n",
> > +                   d->domain_id, gfn, mfn, nr_mfns, memory_policy);
> >   +            switch ( memory_policy )
> > +            {
> > +#ifdef CONFIG_ARM
> > +                case MEMORY_POLICY_ARM_MEM_WB:
> > +                    p2mt = p2m_mmio_direct_c;
> > +                    break;
> > +                case MEMORY_POLICY_ARM_DEV_nGnRE:
> > +                    p2mt = p2m_mmio_direct_dev;
> > +                    break;
> > +#endif
> > +#ifdef CONFIG_X86
> > +                case MEMORY_POLICY_X86_UC_MINUS:
> > +                    p2mt = p2m_mmio_direct;
> > +                    break;
> > +#endif
> > +                default:
> 
> AFAICT, ret will be zero in this path and therefore the caller may think the
> mapping succeeded. I think we want to set 'ret' to -EINVAL.

Good idea, I'll do that


> > +                    goto domctl_out_unlock_domonly;
> > +            }
> >               ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn),
> > p2mt);
> >               if ( ret < 0 )
> >                   printk(XENLOG_G_WARNING


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