linux-kernel - Re: [Patch Part3 V5 1/8] iommu/vt-d: Introduce helper function dmar_walk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5417EFFA.3090804@huawei.com>
Date:	Tue, 16 Sep 2014 16:08:26 +0800
From:	Yijing Wang <wangyijing@...wei.com>
To:	Jiang Liu <jiang.liu@...ux.intel.com>,
	Joerg Roedel <joro@...tes.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Yinghai Lu <yinghai@...nel.org>,
	"Bjorn Helgaas" <bhelgaas@...gle.com>,
	Dan Williams <dan.j.williams@...el.com>,
	"Vinod Koul" <vinod.koul@...el.com>,
	"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>
CC:	Ashok Raj <ashok.raj@...el.com>, Tony Luck <tony.luck@...el.com>,
	<iommu@...ts.linux-foundation.org>, <linux-pci@...r.kernel.org>,
	<linux-hotplug@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<dmaengine@...r.kernel.org>
Subject: Re: [Patch Part3 V5 1/8] iommu/vt-d: Introduce helper function dmar_walk_resources()

>>>  #include "irq_remapping.h"
>>>  
>>> +typedef int (*dmar_res_handler_t)(struct acpi_dmar_header *, void *);
>>> +struct dmar_res_callback {
>>> +	dmar_res_handler_t	cb[ACPI_DMAR_TYPE_RESERVED];
>>> +	void			*arg[ACPI_DMAR_TYPE_RESERVED];
>>> +	bool			ignore_unhandled;
>>> +	bool			print_entry;
>>
>> Why do we need a switch to control print ?
> We will walk DMAR entries several times during hotplug and only
> want to print once.

Fine, thanks for your explanation.

> 
>>
>>> +};
>>> +
>>>  
>>> +static int dmar_walk_resources(struct acpi_dmar_header *start, size_t len,
>>> +			       struct dmar_res_callback *cb)
>>
>> The name dmar_walk_resources easily make people think this is related with I/O or memory resources.
>> Do you have a better idea of this ?  What about dmar_walk_remapping_entry() or dmar_walk_remapping_structure() ?
> Good suggestion, I like dmar_walk_remapping_entries().
>>
>>> +{
>>> +	int ret = 0;
>>> +	struct acpi_dmar_header *iter, *next;
>>> +	struct acpi_dmar_header *end = ((void *)start) + len;
>>> +
>>> +	for (iter = start; iter < end && ret == 0; iter = next) {
>>> +		next = (void *)iter + iter->length;
>>> +		if (iter->length == 0) {
>>> +			/* Avoid looping forever on bad ACPI tables */
>>> +			pr_debug(FW_BUG "Invalid 0-length structure\n");
>>
>> What about use pr_warn() instead of pr_debug(), pr_debug() default is off.
> It seems a common practice for BIOS engineer to mark the last entry with
> zero length. So it will be annoying if we generate this debug message
> on product kernel.

OK.

> 
>>
>>> +			break;
>>> +		} else if (next > end) {
>>> +			/* Avoid passing table end */
>>> +			pr_warn(FW_BUG "record passes table end\n");
>>> +			ret = -EINVAL;
>>> +			break;
>>> +		}
>>> +
>>> +		if (cb->print_entry)
>>> +			dmar_table_print_dmar_entry(iter);
>>> +
>>> +		if (iter->type >= ACPI_DMAR_TYPE_RESERVED) {
>>> +			/* continue for forward compatibility */
>>> +			pr_debug("Unknown DMAR structure type %d\n",
>>> +				 iter->type);
>>
>> Same as above.
> This is typically caused by new DMAR specification. It will also be
> annoying too if  we also generate this debug message on an newer
> hardware platform with older linux kernel.

OK.

> 
>>
>>> +		} else if (cb->cb[iter->type]) {
>>> +			ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
>>> +		} else if (!cb->ignore_unhandled) {
>>> +			pr_warn("No handler for DMAR structure type %d\n",

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/