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

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

----- elena.ufimtseva@xxxxxxxxxx wrote:

> On Wed, Mar 11, 2015 at 10:29:54AM +0000, Jan Beulich wrote:
> > >>> On 09.03.15 at 15:42, <elena.ufimtseva@xxxxxxxxxx> wrote:
> > > --- a/xen/drivers/passthrough/vtd/dmar.c
> > > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > > @@ -567,6 +567,66 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > +int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> > 
> > static - I don't see why the command line processing in patch 2
> can't
> > be put in this file.
> 
> I wanted to keep it close to iommu command line parser in iommu.c.
> Andrew mentioned that the parser should be defined as custom_param.
> Did you mean that as well and that it should be moved to dmar.c?

Never mind, I saw the answer.

> 
> > 
> > > +{
> > > +    u8 b, d, f;
> > > +    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) )
> > > +        i = UINT_MAX;
> > > +
> > > +    for ( ; i < rmrru->scope.devices_cnt; i++ )
> > > +    {
> > > +        b = PCI_BUS(rmrru->scope.devices[i]);
> > > +        d = PCI_SLOT(rmrru->scope.devices[i]);
> > > +        f = PCI_FUNC(rmrru->scope.devices[i]);
> > 
> > Please make this the declarations (with initializers) of these
> variables;
> > they don't appear to be used outside this scope.
> > 
> > > +
> > > +        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);
> > > +        ret = -EFAULT;
> > > +    }
> > > +    else if ( rmrru->base_address > rmrru->end_address )
> > > +    {
> > > +        dprintk(XENLOG_WARNING VTDPREFIX,
> > > +            "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> > > +            rmrru->base_address, rmrru->end_address);
> > > +        ret = -EFAULT;
> > > +    }
> > > +    else
> > > +    {
> > > +        if ( iommu_verbose )
> > > +            dprintk(VTDPREFIX,
> > > +                    "  RMRR region: base_addr %"PRIx64
> > > +                    " end_address %"PRIx64"\n",
> > > +                    rmrru->base_address, rmrru->end_address);
> > > +        acpi_register_rmrr_unit(rmrru);
> > > +    }
> > > +    return ret;
> > 
> > Blank line before return please.
> > 
> > > @@ -618,66 +678,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header
> *header)
> > >                                 &rmrru->scope, RMRR_TYPE,
> rmrr->segment);
> > >  
> > >      if ( ret || (rmrru->scope.devices_cnt == 0) )
> > > -        xfree(rmrru);
> > > -    else
> > >      {
> > > -        u8 b, d, f;
> > > -        bool_t ignore = 0;
> > > -        unsigned int i = 0;
> > > -
> > > -        /* Skip checking if segment is not accessible yet. */
> > > -        if ( !pci_known_segment(rmrr->segment) )
> > > -            i = UINT_MAX;
> > > -
> > > -        for ( ; i < rmrru->scope.devices_cnt; i++ )
> > > -        {
> > > -            b = PCI_BUS(rmrru->scope.devices[i]);
> > > -            d = PCI_SLOT(rmrru->scope.devices[i]);
> > > -            f = PCI_FUNC(rmrru->scope.devices[i]);
> > > -
> > > -            if ( pci_device_detect(rmrr->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",
> > > -                        rmrr->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);
> > > -        }
> > > -        else if ( base_addr > end_addr )
> > > -        {
> > > -            dprintk(XENLOG_WARNING VTDPREFIX,
> > > -                "  The RMRR (%"PRIx64", %"PRIx64") is
> incorrect!\n",
> > > -                rmrru->base_address, rmrru->end_address);
> > > -            xfree(rmrru);
> > > -            ret = -EFAULT;
> > > -        }
> > > -        else
> > > -        {
> > > -            if ( iommu_verbose )
> > > -                dprintk(VTDPREFIX,
> > > -                        "  RMRR region: base_addr %"PRIx64
> > > -                        " end_address %"PRIx64"\n",
> > > -                        rmrru->base_address,
> rmrru->end_address);
> > > -            acpi_register_rmrr_unit(rmrru);
> > > -        }
> > > +        xfree(rmrru);
> > > +        return ret;
> > >      }
> > >  
> > > +    ret = register_one_rmrr(rmrru);
> > > +    if ( ret )
> > > +        xfree(rmrru);
> > >      return ret;
> > 
> > Please try to make this a single xfree() and a single return (with
> > again a blank line preceding it).
> 
> Got it, I will make changes you mentioned.
> > 
> > Jan

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