Re: [Xen-devel] [PATCH v4 2/2] iommu: add rmrr Xen command line option for misc rmrrs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.15 at 01:50, <elena.ufimtseva@xxxxxxxxxx> wrote:
> Format for rmrr Xen command line option:
> rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]]
> If grub used and multiple ranges are specified, ';' should be 
> quoted/escaped,
> refer to grub man for more information.

I think you mean grub2 here?

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1170,6 +1170,18 @@ 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. Note: grub requires to escape or use quotations if special
> +characters are used, namely ';', refer to the grub documentation if multiple
> +ranges are specified.

Same here then.

> @@ -846,6 +847,72 @@ out:
>      return ret;
>  }
>  
> +static __initdata LIST_HEAD(misc_rmrr_units);
> +
> +static void __init add_misc_rmrr(void)

s/misc/aux/ maybe? Also I don't really understand what you need that
list head for - the presently otherwise unused nr_rmrr allows you to
know how many valid entries there are in rmrru[].

> +{
> +    struct acpi_rmrr_unit *rmrrn;
> +    struct misc_rmrr_unit *rmrru;
> +    unsigned int dev;
> +    unsigned int seg;
> +
> +    list_for_each_entry( rmrru, &misc_rmrr_units, list )
> +    {
> +        rmrrn = xmalloc(struct acpi_rmrr_unit);
> +        if ( !rmrrn )
> +            return;
> +
> +        rmrrn->scope.devices = xmalloc_array(typeof(*rmrrn->scope.devices),
> +                                             rmrru->dev_count);
> +        if ( !rmrrn->scope.devices )
> +        {
> +            xfree(rmrrn);
> +            return;
> +        }
> +        if ( mfn_valid(rmrru->base_address << PAGE_SHIFT) ||
> +             mfn_valid(rmrru->end_address << PAGE_SHIFT) )

Please use PFN_UP() and PFN_DOWN() as appropriate. Also, how
large do we expect custom RMRRs to reasonably get? If no more
than a few pages, I think it would be quite reasonable to check
all involved pages here rather than just first and last one. (And
even if there can be many pages here, leaving at least a comment
saying that this could do with improvement would be nice.)

> +        {
> +            if ( iommu_verbose )
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Invalid RMRR range [%"PRIx64" - %"PRIx64"]\n",
> +                       rmrru->base_address, rmrru->end_address);

I think this should be issued regardless of iommu_verbose.

> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +            return;
> +        }
> +
> +        rmrrn->base_address = rmrru->base_address;
> +        rmrrn->end_address = rmrru->end_address;
> +        seg = rmrru->sbdf[0] >> 16;
> +
> +        for ( dev = 0; dev < rmrru->dev_count; dev++ )
> +        {
> +            rmrrn->scope.devices[dev] = rmrru->sbdf[dev];
> +            seg = seg | (rmrru->sbdf[dev] >> 16);

|=

The loop starting at 0 there also is no need to set up seg to other
than plain zero before the loop.

But what I'm worried about is invalid uses like

rmrr=<addr>=0001:bb:dd.f,0000:bb:dd.f

(as opposed to the valid

rmrr=<addr>=0001:bb:dd.f,bb:dd.f

). On an earlier version I had specifically pointed out that dealing
with an omitted vs a zero segment requires special care.

> +        }
> +
> +        if ( seg != (rmrru->sbdf[0] >> 16) )
> +        {
> +            if ( iommu_verbose )
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Segments are not equal for RMRR range  [%"PRIx64" - 
> %"PRIx64"]\n",
> +                       rmrru->base_address, rmrru->end_address);

Same here regarding iommu_verbose.

> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);
> +            return;
> +        }
> +
> +        rmrrn->segment = seg;
> +        rmrrn->scope.devices_cnt = rmrru->dev_count;
> +
> +        if ( register_one_rmrr(rmrrn) )
> +        {
> +            xfree(rmrrn->scope.devices);
> +            xfree(rmrrn);

Perhaps also worth an error message?

> +static void __init parse_rmrr_param(const char *str)
> +{
> +    const char *s = str, *cur, *stmp;
> +    unsigned int i = 0, seg, bus, dev, func;
> +    u64 start, end;
> +
> +    do {
> +        start = simple_strtoull(cur = s, &s, 0);
> +        if ( cur == s )
> +            break;
> +
> +        if ( *s == '-' )
> +        {
> +            end = simple_strtoull(cur = s + 1, &s, 0);
> +            if ( cur == s )
> +                break;
> +        }
> +        else
> +            end = start;
> +        if ( i < MAX_MISC_RMRR )
> +        {
> +            rmrru[i].base_address = start << PAGE_SHIFT;
> +            rmrru[i].end_address = (end + 1) << PAGE_SHIFT;

Considering an earlier comment perhaps it would be better to simply
store frame numbers here, and convert to addresses only in
add_misc_rmrr()?

> +            rmrru[i].dev_count = 0;
> +        }
> +        else
> +            break;

Please invert the if() condition (allowing to do things without "else")
and move the check up to the top of the loop (or even into the
loop condition further down).

> +
> +        if ( *s != '=' )
> +            continue;
> +
> +        do {
> +            if ( rmrru[i].dev_count >= MAX_MISC_RMRR_DEV )
> +                break;
> +            if ( *s == ';' )
> +                break;
> +            stmp = parse_pci(s + 1, &seg, &bus, &dev, &func);
> +            if ( !stmp )
> +                break;
> +            rmrru[i].sbdf[rmrru[i].dev_count] = PCI_BDF(bus, dev, func) | 
> seg << 16;

Parentheses around the shift please.

> --- 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_MISC_RMRR_DEV 20
> +struct misc_rmrr_unit {
> +    struct list_head list;
> +    u64    base_address;
> +    u64    end_address;
> +    u16    dev_count;
> +    u32    sbdf[MAX_MISC_RMRR_DEV];
> +};

This type is used in only one source file - no need to expose it here.

Jan

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