Re: [PATCH v4 6/8] dm_op: convert HVMOP_set_mem_type

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 20 January 2017 18:28
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>
> Subject: Re: [PATCH v4 6/8] dm_op: convert HVMOP_set_mem_type
> 
> On 17/01/17 17:29, Paul Durrant wrote:
> > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> > index dd81116..b3c91f8 100644
> > --- a/xen/arch/x86/hvm/dm.c
> > +++ b/xen/arch/x86/hvm/dm.c
> > @@ -159,6 +159,82 @@ static int modified_memory(struct domain *d,
> xen_pfn_t *first_pfn,
> >      return rc;
> >  }
> >
> > +static bool allow_p2m_type_change(p2m_type_t old, p2m_type_t new)
> > +{
> > +    return p2m_is_ram(old) ||
> > +           (p2m_is_hole(old) && new == p2m_mmio_dm) ||
> > +           (old == p2m_ioreq_server && new == p2m_ram_rw);
> > +}
> > +
> > +static int set_mem_type(struct domain *d, hvmmem_type_t mem_type,
> > +                        xen_pfn_t *first_pfn, unsigned int *nr)
> > +{
> 
> Similarly as patch 5, this would be cleaner taking the whole struct
> xen_dm_op_set_mem_type

Ok, maybe. I guess the reduced stack usage may be of benefit but personally I prefer breaking out the fields as params. I'll modify patch #5 as well.

> 
> > +    xen_pfn_t last_pfn = *first_pfn + *nr - 1;
> > +    unsigned int iter = 0;
> > +    int rc = 0;
> > +
> > +    /* Interface types to internal p2m types */
> > +    static const p2m_type_t memtype[] = {
> > +        [HVMMEM_ram_rw]  = p2m_ram_rw,
> > +        [HVMMEM_ram_ro]  = p2m_ram_ro,
> > +        [HVMMEM_mmio_dm] = p2m_mmio_dm,
> > +        [HVMMEM_unused] = p2m_invalid,
> > +        [HVMMEM_ioreq_server] = p2m_ioreq_server
> 
> Please introduce a trailing comma here, as you are moving the code.

Ok.

> 
> With this done, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
> 

Thanks,

  Paul

> > +    };
> > +
> > +    if ( (*first_pfn > last_pfn) ||
> > +         (last_pfn > domain_get_maximum_gpfn(d)) )
> > +        return -EINVAL;
> > +
> > +    if ( mem_type >= ARRAY_SIZE(memtype) ||
> > +         unlikely(mem_type == HVMMEM_unused) )
> > +        return -EINVAL;
> > +
> > +    while ( iter < *nr )
> > +    {
> > +        unsigned long pfn = *first_pfn + iter;
> 
> :(  I am not going to request that you change it, but this highlights a
> large set of internal functions which could do with moving to using pfn_t.


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