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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Stefano Stabellini <stefanos@xilinx.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v3 2/5] xen: extend XEN_DOMCTL_memory_mapping to handle memory policy
Date: Tue, 6 Aug 2019 16:57:50 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.1908061644200.2451@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <5D09E8CB020000780023987F@prv1-mh.provo.novell.com>

On Wed, 19 Jun 2019, Jan Beulich wrote:
> >>> On 19.06.19 at 01:20, <sstabellini@kernel.org> wrote:
> > --- 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;
> 
> Why not MEMORY_POLICY_DEFAULT?

OK

> > --- 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;
> 
> I can't see the need for a fixed-width type here.

OK

> > @@ -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);
> 
> Why "cache=" when it's a "policy" value?

OK

> > +            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:
> 
> FTR - I could certainly live with this becoming MEMORY_POLICY_DEFAULT
> for now, if that's really what Andrew prefers for x86.

All right, I'll remove MEMORY_POLICY_X86_UC_MINUS then.

> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -571,12 +571,33 @@ struct xen_domctl_bind_pt_irq {
> >  */
> >  #define DPCI_ADD_MAPPING         1
> >  #define DPCI_REMOVE_MAPPING      0
> > +/*
> > + * Default memory policy. Corresponds to:
> > + * Arm: MEMORY_POLICY_ARM_DEV_nGnRE
> > + * x86: MEMORY_POLICY_X86_UC_MINUS
> > + */
> > +#define MEMORY_POLICY_DEFAULT         0
> > +#if defined(__i386__) || defined(__x86_64__)
> > +/* x86 only. Memory type UNCACHABLE */
> > +# define MEMORY_POLICY_X86_UC_MINUS    0
> > +#elif defined(__arm__) || defined (__aarch64__)
> > +/* Arm only. Outer Shareable, Device-nGnRE memory (Device Memory on Armv7) 
> > */
> > +# define MEMORY_POLICY_ARM_DEV_nGnRE      0
> > +/* Arm only. Outer Shareable, Outer/Inner Write-Back Cacheable memory */
> > +# define MEMORY_POLICY_ARM_MEM_WB         1
> > +/*
> > + * On ARM, MEMORY_POLICY selects the stage-2 memory attributes, but note
> 
> Further up it's Arm - why all upper case here?

I fixed it

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