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

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Mon, Jun 01, 2015 at 09:53:55AM +0100, Jan Beulich wrote:
> >>> 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.

Yes, and I agreed that it did not make sense to set ret here, wishful
typing I guess ) 

> 
> 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).

yes, changing this.

> 
> Jan
> 

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