linux-kernel - Re: [PATCH V5 1/9] ACPI: Honor ACPI

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: <56378692.9060208@amd.com>
Date:	Mon, 2 Nov 2015 09:51:46 -0600
From:	Suravee Suthikulanit <suravee.suthikulpanit@....com>
To:	Hanjun Guo <hanjun.guo@...aro.org>,
	Dennis Chen <dennis.chen@....com>, <bhelgaas@...gle.com>,
	<rjw@...ysocki.net>, <lenb@...nel.org>, <catalin.marinas@....com>,
	<will.deacon@....com>
CC:	<thomas.lendacky@....com>, <herbert@...dor.apana.org.au>,
	<davem@...emloft.net>, <linux-acpi@...r.kernel.org>,
	<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <jeremy.linton@....com>,
	<robh+dt@...nel.org>
Subject: Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

Hi Dennis / Hanjun,

On 11/2/2015 5:58 AM, Hanjun Guo wrote:
> Hi Dennis,
>
> On 11/02/2015 12:02 PM, Dennis Chen wrote:
>> On Thu, Oct 29, 2015 at 6:50 AM, Suravee Suthikulpanit
>> <Suravee.Suthikulpanit@....com> wrote:
>>> From: Jeremy Linton <jeremy.linton@....com>
>>>
>>> ACPI configurations can now mark devices as noncoherent,
>>> support that choice.
>>>
>>> NOTE: This is required to support USB on ARM Juno Development Board.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
>>> CC: Bjorn Helgaas <bhelgaas@...gle.com>
>>> CC: Catalin Marinas <catalin.marinas@....com>
>>> CC: Rob Herring <robh+dt@...nel.org>
>>> CC: Will Deacon <will.deacon@....com>
>>> CC: Rafael J. Wysocki <rjw@...ysocki.net>
>>> ---
>>>   include/acpi/acpi_bus.h | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>>> index d11eff8..0f131d2 100644
>>> --- a/include/acpi/acpi_bus.h
>>> +++ b/include/acpi/acpi_bus.h
>>> @@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct
>>> acpi_device *adev, bool *coherent)
>>>        * case 1. Do not support and disable DMA.
>>>        * case 2. Support but rely on arch-specific cache maintenance for
>>>        *         non-coherence DMA operations.
>>> -     * Currently, we implement case 1 above.
>>> +     * Currently, we implement case 2 above.
>>>        *
>>>        * For the case when _CCA is missing (i.e. cca_seen=0) and
>>>        * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
>>> @@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct
>>> acpi_device *adev, bool *coherent)
>>>        *
>>>        * See acpi_init_coherency() for more info.
>>>        */
>>> -    if (adev->flags.coherent_dma) {
>>> +    if (adev->flags.coherent_dma ||
>>> +        (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
>>>           ret = true;
>>>           if (coherent)
>>>               *coherent = adev->flags.coherent_dma;
>>
>> Hi Suravee,
>>
>> The acpi_check_dma function has been removed in patch 6 of this patch
>> set, why it is still be used
>> here, am I missing something? If the acpi_check_dma will be used in
>> the future, personally I'd like
>
> I think this patch just to let people know that there is
> case that arch-specific cache maintenance is still needed
> for ACPI (such as Juno board), and in the later patches will
> cover this case.
>
> acpi_check_dma() will be replaced by acpi_get_dma_attr(),
> and in acpi_get_dma_attr() will cover that case and will
> be easily understood. (Suravee, correct me if I'm wrong :) )

Thanks Hanjun for filling in the info.

Yes, this is mainly to document the logic changes required by Juno, 
which would be more clear than just merging this change in the later patch.

>> to use IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) while not CONFIG_ARM64
>> macro here,

We could have used CONFIG_ACPI_CCA_REQUIRED here, but this will be 
replaced by logic in patch 5, and removed in patch 6 anyways. So, I 
think it is okay. Eventually, the rest of the logic will be using 
CONFIG_ACPI_CCA_REQUIRED.

or since _CCA attribute
>> is arch-specific, it's reasonable to leave the _CCA handling policy to
>> the arch-specific code. For example,
>> with a link weak function like acpi_arch_check_dma() as a default
>> handling if no arch-specific code
>> provided, the actual _CCA handling will be implemented in the ARM,
>> Intel or other Arch if required.
>
> Actually Intel platform don't need _CCA and it's coherent
> in default, since _CCA is in ACPI spec, I would like it's
> handled in ACPI core.
>
> Thanks
> Hanjun

I also agree with Hanjun that the CCA parsing should be handled by the 
ACPI core driver. Since we are using the CONFIG_ACPI_CCA_REQUIRED, we 
should not need to have arch-specific code. If the ACPI spec gets more 
complicate in the future, we can revisit this. :)

Thanks,
Suravee

--
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/