[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] iommu VT-d: separate rmrr addition function
----- 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |