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

["Marczykowski, Marek" <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature