RE: [PATCH v7 08/11] IOMMU/VT-d: wire common device reserved memory API

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Marczykowski, Marek <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, September 27, 2022 7:54 AM
> 
> On Fri, Sep 23, 2022 at 07:21:04AM +0000, Tian, Kevin wrote:
> > > From: Marek Marczykowski-Górecki
> > > Sent: Saturday, September 17, 2022 10:51 AM
> > >
> > > Re-use rmrr= parameter handling code to handle common device
> reserved
> > > memory.
> > >
> > > Signed-off-by: Marek Marczykowski-Górecki
> > > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > Changes in v3:
> > > - make MAX_USER_RMRR_PAGES applicable only to user-configured
> RMRR
> > > ---
> > >  xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++------------
> -
> > >  1 file changed, 119 insertions(+), 82 deletions(-)
> > >
> > > diff --git a/xen/drivers/passthrough/vtd/dmar.c
> > > b/xen/drivers/passthrough/vtd/dmar.c
> > > index 367304c8739c..3df5f6b69719 100644
> > > --- a/xen/drivers/passthrough/vtd/dmar.c
> > > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > > @@ -861,111 +861,139 @@ static struct user_rmrr __initdata
> > > user_rmrrs[MAX_USER_RMRR];
> > >
> > >  /* Macro for RMRR inclusive range formatting. */
> > >  #define ERMRRU_FMT "[%lx-%lx]"
> > > -#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn
> > > +#define ERMRRU_ARG base_pfn, end_pfn
> > > +
> > > +static int __init add_one_user_rmrr(unsigned long base_pfn,
> > > +                                    unsigned long end_pfn,
> > > +                                    unsigned int dev_count,
> > > +                                    uint32_t *sbdf);
> >
> > Just move the function here then no need of a declaration.
> 
> Ok.
> 
> > >
> > >  static int __init add_user_rmrr(void)
> > >  {
> > > +    unsigned int i;
> > > +    int ret;
> > > +
> > > +    for ( i = 0; i < nr_rmrr; i++ )
> > > +    {
> > > +        ret = add_one_user_rmrr(user_rmrrs[i].base_pfn,
> > > +                                user_rmrrs[i].end_pfn,
> > > +                                user_rmrrs[i].dev_count,
> > > +                                user_rmrrs[i].sbdf);
> > > +        if ( ret < 0 )
> > > +            return ret;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > > +/* Returns 1 on success, 0 when ignoring and < 0 on error. */
> >
> > I'm not sure the point of 'ignoring'. If user specifies a wrong RMRR
> > range (overlap, mfn invalid, etc.) just return an error similar to
> > -ENOMEM. Ignoring a user-specified range implies that something
> > would potentially get broken hence better fail it early.
> 
> That's the behaviour that was here before, I simply added a comment
> about it explicitly (previously it used 'continue' heavily, now it's a
> separate function so it's a return value).
> While I agree in principle, I don't think such change should be part of
> this patch.
> 
> (...)
> 
> > > @@ -1108,6 +1136,15 @@ static int __init cf_check
> parse_rmrr_param(const
> > > char *str)
> > >          else
> > >              end = start;
> > >
> > > +        if ( (end - start) >= MAX_USER_RMRR_PAGES )
> > > +        {
> > > +            printk(XENLOG_ERR VTDPREFIX
> > > +                    "RMRR range "ERMRRU_FMT" exceeds "\
> > > +                    __stringify(MAX_USER_RMRR_PAGES)" pages\n",
> > > +                    start, end);
> > > +            return -E2BIG;
> > > +        }
> > > +
> >
> > why moving this error check out of add_one_user_rmrr()? I didn't
> > get why it's special from other checks there, e.g. having base>end...
> 
> To allow internal callers bypass MAX_USER_RMRR_PAGES, and make it apply
> really only to user-provided ranges.
> 

With above clarification and order adjustment,

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>