Re: [Xen-devel] [PATCH v6 2/4] iommu VT-d: separate rmrr addition function

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.05.15 at 23:38, <elena.ufimtseva@xxxxxxxxxx> wrote:
> In preparation for auxiliary RMRR data provided on Xen
> command line, make RMRR adding a separate function.
> Also free memery for rmrr device scope in error path.
> No changes since v5.

Certainly there is. (And the statement wouldn't belong here anyway,
but below the first --- separator.)

> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

And certainly I didn't approve it in this shape:

> +static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> +{
> +    bool_t ignore = 0;
> +    unsigned int i = 0;
> +    int ret = 0;
> +
> +    /* Skip checking if segment is not accessible yet. */
> +    if ( !pci_known_segment(rmrru->segment) )
> +    {
> +        dprintk(XENLOG_WARNING VTDPREFIX, "UNKNOWN Prefix! %04x", 
> rmrru->segment);
> +        i = UINT_MAX;
> +    }
> +
> +    for ( ; i < rmrru->scope.devices_cnt; i++ )
> +    {
> +        u8 b = PCI_BUS(rmrru->scope.devices[i]);
> +        u8 d = PCI_SLOT(rmrru->scope.devices[i]);
> +        u8 f = PCI_FUNC(rmrru->scope.devices[i]);
> +
> +        if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
> +        {
> +            dprintk(XENLOG_WARNING VTDPREFIX,
> +                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
> +                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> +                    rmrru->segment, b, d, f,
> +                    rmrru->base_address, rmrru->end_address);
> +            ignore = 1;
> +        }
> +        else
> +        {
> +            ignore = 0;
> +            break;
> +        }
> +    }
> +
> +    if ( ignore )
> +    {
> +        dprintk(XENLOG_WARNING VTDPREFIX,
> +            "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> +            "devices under its scope are not PCI discoverable!\n",
> +            rmrru->base_address, rmrru->end_address);
> +        xfree(rmrru->scope.devices);
> +        xfree(rmrru);
> +        ret = -EFAULT;

You _again_ made this an error, which it wasn't before. A little more
care please.

Also you folded the leak fix into here without saying so. As said on
the solitary leak fix patch - that change belongs there (not the least
because we will want to backport that but not this one).

Jan


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