From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elena Ufimtseva Subject: Re: [PATCH 1/2] iommu VT-d: separate rmrr addition function Date: Thu, 12 Mar 2015 16:48:52 -0400 Message-ID: <20150312204852.GA3047@elena.ufimtseva> References: <1425912177-2890-1-git-send-email-elena.ufimtseva@oracle.com> <1425912177-2890-2-git-send-email-elena.ufimtseva@oracle.com> <5500273202000078000686D2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <5500273202000078000686D2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, boris.ostrovsky@oracle.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, Mar 11, 2015 at 10:29:54AM +0000, Jan Beulich wrote: > >>> On 09.03.15 at 15:42, 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? > > > +{ > > + 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