Re: [Xen-devel] [PATCH v5 4/4] iommu: add rmrr Xen command line option for extra rmrrs

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Tue, May 26, 2015 at 01:02:27PM +0100, Jan Beulich wrote:
> >>> On 23.05.15 at 03:33, <elena.ufimtseva@xxxxxxxxxx> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1185,6 +1185,19 @@ Specify the host reboot method.
> >  'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by
> >   default it will use that method first).
> >  
> > +### rmrr
> > +> '= 
> > start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
> > +
> > +Define RMRRs units that are missing from ACPI table along with device
> > +they belong to and use them for 1:1 mapping. End addresses can be omitted
> > +and one page will be mapped. The ranges are inclusive when start and end
> > +are specified.If segement of the first device is not specified, the 
> > default segment will be used.
> 

Thanks for review Jan.

> "specified. If the segment ..., segment zero will be used."
> 
> > +If segments are specified for every device and not equal, error will be 
> > reported.
> 
> "..., an error ..."
> 
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -50,6 +50,7 @@ static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
> >  static struct acpi_table_header *__read_mostly dmar_table;
> >  static int __read_mostly dmar_flags;
> >  static u64 __read_mostly igd_drhd_address;
> > +static void __init add_extra_rmrr(void);
> 
> Why do you need this declaration? (And if you really need it - no
> segment annotations on declarations please.)
> 
> > @@ -856,6 +857,78 @@ out:
> >      return ret;
> >  }
> >  
> > +#define MAX_EXTRA_RMRR_PAGES 16
> > +#define MAX_EXTRA_RMRR 10
> > +static __initdata unsigned int nr_rmrr;
> > +static struct __initdata extra_rmrr_unit rmrru[MAX_EXTRA_RMRR];
> > +
> > +static void __init add_extra_rmrr(void)
> > +{
> > +    struct acpi_rmrr_unit *rmrrn;
> > +    unsigned int dev, seg, addr;
> > +
> > +    for (unsigned int i = 0; i < nr_rmrr; i++ )
> 
> No C++ style constructs like this please. Instead please add the
> missing blank after the opening parenthesis.
> 
> > +    {
> > +        rmrrn = xmalloc(struct acpi_rmrr_unit);
> 
> acpi_parse_one_rmrr() uses xzalloc() here. For the avoidance of
> doubt, I'd be fine with you doing so provided this is correct (i.e. all
> fields end up properly initialized, just like is the case with the
> ->scope.devices allocation), if this wasn't introducing a latent bug
> (should a field get added).
Agree, will change this.
> 
> > +        if ( !rmrrn )
> > +            return;
> > +
> > +        rmrrn->scope.devices = xmalloc_array(typeof(*rmrrn->scope.devices),
> 
> I'm afraid I may have mislead you with comments elsewhere: In
> xmalloc() invocations, considering the typeful result it produces,
> using the spelled out type is preferred over typeof() like used
> here.
Thanks for explanation, I did not know that.
> 
> > +                                             rmrru[i].dev_count);
> > +        if ( !rmrrn->scope.devices )
> > +        {
> > +            xfree(rmrrn);
> > +            return;
> > +        }
> > +
> > +        if ( rmrru[i].end_address - rmrru[i].base_address > 
> > MAX_EXTRA_RMRR_PAGES )
> 
> Now this reads really odd: With the conversion to store page numbers
> in these fields, they should have got renamed from _address (and
> afaict no longer need to be of u64 type). Also note that you have an
> off-by-one error here: The end address being inclusive, you want to
> bail on >= max.
> 
> I also fail to see end < base being rejected somewhere. Nor are
> overlaps being dealt with (see acpi_parse_one_rmrr()).

Somehow I skipped that, possibly wrong brnach.
Will fix this and add overlap check.
> 
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "RMRR range exceeds 16 pages [%"PRIx64" - %"PRIx64"]\n",
> > +                   rmrru[i].base_address, rmrru[i].end_address);
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +            return;
> > +        }
> > +
> > +        for ( addr = rmrru[i].base_address; addr <= rmrru[i].end_address; 
> > addr++ )
> 
> And the loop variable here shouldn't be addr then (and certainly not
> of type "unsigned int").
> 
> > +        {
> > +            if ( iommu_verbose )
> > +                printk(XENLOG_ERR VTDPREFIX
> > +                       "Invalid mfn in RMRR range [%"PRIx64" - 
> > %"PRIx64"]\n",
> > +                       rmrru[i].base_address, rmrru[i].end_address);
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +            return;
> > +        }
> > +
> > +        seg = 0;
> > +        for ( dev = 0; dev < rmrru->dev_count; dev++ )
> > +        {
> > +            rmrrn->scope.devices[dev] = rmrru->sbdf[dev];
> > +            seg = seg | (rmrru->sbdf[dev] >> 16);
> 
> |=
> 
> Also with you having added PCI_SBDF() in patch 1, you should add
> the matched PCI_SEG() (or some such) instead of open coding it
> here.
Yes, will be also useful.
> 
> > +        }
> > +        if ( seg != (rmrru->sbdf[0] >> 16) )
> > +        {
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Segments are not equal for RMRR range  [%"PRIx64" - 
> > %"PRIx64"]\n",
> > +                   rmrru->base_address, rmrru->end_address);
> > +            xfree(rmrrn->scope.devices);
> > +            xfree(rmrrn);
> > +            continue;
> > +        }
> > +        rmrrn->segment = seg;
> > +        rmrrn->base_address = rmrru[i].base_address << PAGE_SHIFT;
> > +        rmrrn->end_address = (rmrru[i].end_address + 1) << PAGE_SHIFT;
> > +        rmrrn->scope.devices_cnt = rmrru[i].dev_count;
> > +
> > +        if ( register_one_rmrr(rmrrn) )
> > +            printk(XENLOG_ERR VTDPREFIX
> > +                   "Could not register RMMR range  [%"PRIx64" - 
> > %"PRIx64"]\n",
> > +                   rmrru[i].base_address, rmrru[i].end_address);
> 
> xfree(rmrrn)?
> 
> > @@ -874,6 +947,7 @@ int __init acpi_dmar_init(void)
> >                           PAGE_HYPERVISOR);
> >          dmar_table = __va(dmar_addr);
> >      }
> > +    add_extra_rmrr();
> >  
> >      return parse_dmar_table(acpi_parse_dmar);
> >  }
> 
> I'd really like to see the extra RMRRs being added after the ones
> parsed from the ACPI tables. 
> 
> > + * If segement of the first device is not specified, the default segment 
> > will be used.
> > + * If other segments are not specified, first device segment will be used.
> > + * If segments are specified for every device and not equal, error will be 
> > reported.
> 
> See patch description comments.
> 
> > +static void __init parse_rmrr_param(const char *str)
> > +{
> > +    const char *s = str, *cur, *stmp;
> > +    unsigned int seg, bus, dev, func;
> > +    int default_segment = 0;
> 
> This must be initialized on each iteration of the inner loop below.
> Best to simply move the declaration + initializer into that loop (as
> could be done for several of the other variables).
> 
> > +    u64 start, end;
> > +
> > +    do {
> > +        start = simple_strtoull(cur = s, &s, 0);
> 
> strtoul() is sufficient for parsing frame numbers.
> 
> > +        if ( cur == s )
> > +            break;
> > +
> > +        if ( *s == '-' )
> > +        {
> > +            end = simple_strtoull(cur = s + 1, &s, 0);
> > +            if ( cur == s )
> > +                break;
> > +        }
> > +        else
> > +            end = start;
> > +        if ( nr_rmrr < MAX_EXTRA_RMRR )
> > +        {
> > +            rmrru[nr_rmrr].base_address = start;
> > +            rmrru[nr_rmrr].end_address = end;
> > +            rmrru[nr_rmrr].dev_count = 0;
> > +        }
> > +        else
> > +            break;
> 
> Pointless if/else (else can be avoided by inverting the condition).
> And the resulting if() would then be moved up and as a result be
> folded (after inverting back) into the loop condition.

Agree.
> 
> > +        if ( *s != '=' )
> > +            continue;
> > +
> > +        do {
> > +            if ( rmrru[nr_rmrr].dev_count >= MAX_EXTRA_RMRR_DEV )
> > +                break;
> 
> Similarly this could be inverted and folded into the loop condition.
> 
> > +            if ( *s == ';' )
> > +                break;
> > +            stmp = __parse_pci(s + 1, &seg, &bus, &dev, &func, 
> > &default_segment);
> > +            if ( !stmp )
> > +                break;
> > +            /* Not specified segment will be replaced with one from first 
> > device. */
> > +            if ( rmrru[nr_rmrr].dev_count > 0 && (seg != 
> > (rmrru[nr_rmrr].sbdf[0] >> 16)) )
> > +                if ( default_segment )
> 
> The two if()-s should be folded. And then I don't see what the right
> side of the && is good for.

True, later I compare them anyways.
> 
> > --- a/xen/drivers/passthrough/vtd/dmar.h
> > +++ b/xen/drivers/passthrough/vtd/dmar.h
> > @@ -132,4 +132,14 @@ void disable_pmr(struct iommu *iommu);
> >  int is_usb_device(u16 seg, u8 bus, u8 devfn);
> >  int is_igd_drhd(struct acpi_drhd_unit *drhd);
> >  
> > +/* RMRR units derived from command line rmrr option */
> > +#define MAX_EXTRA_RMRR_DEV 20
> > +struct extra_rmrr_unit {
> > +    struct list_head list;
> > +    u64    base_address;
> > +    u64    end_address;
> > +    u16    dev_count;
> > +    u32    sbdf[MAX_EXTRA_RMRR_DEV];
> > +};
> 
> I'm pretty certain I had already indicated that types used in only a
> single source file should not be put in any header.

Yep, missed that.
> 
> Jan

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