|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 4/4] iommu: add rmrr Xen command line option for extra rmrrs
>>> 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.
"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).
> + 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.
> + 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()).
> + {
> + 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.
> + }
> + 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.
> + 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.
> --- 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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |