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: <a02a3cee-773a-da00-616f-04652c0905ce@arm.com>
Date:   Tue, 4 Oct 2022 16:31:38 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Matthew Rosato <mjrosato@...ux.ibm.com>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>, iommu@...ts.linux.dev
Cc:     linux-s390@...r.kernel.org, borntraeger@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        svens@...ux.ibm.com, joro@...tes.org, will@...nel.org,
        jgg@...dia.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/5] iommu/s390: Fix incorrect pgsize_bitmap

On 2022-10-04 16:12, Matthew Rosato wrote:
> On 10/4/22 11:02 AM, Robin Murphy wrote:
>> On 2022-10-04 13:07, Niklas Schnelle wrote:
>>> The .pgsize_bitmap property of struct iommu_ops is not a page mask but
>>> rather has a bit set for each size of pages the IOMMU supports. As the
>>> comment correctly pointed out at this moment the code only support 4K
>>> pages so simply use SZ_4K here.
>>
>> Unless it's already been done somewhere else, you'll want to switch over to the {map,unmap}_pages() interfaces as well to avoid taking a hit on efficiency here. The "page mask" thing was an old hack to trick the core API into making fewer map/unmap calls where the driver could map arbitrary numbers of pages at once anyway. The multi-page interfaces now do that more honestly and generally better (since they work for non-power-of-two sizes as well).
> 
> Thanks for the heads up -- Niklas has some additional series coming soon as described here:
> 
> https://lore.kernel.org/linux-iommu/a10424adbe01a0fd40372cbd0736d11e517951a1.camel@linux.ibm.com/
> 
> So implementing the _pages() interfaces is soon up on the roadmap.  But given what you say I wonder if this patch should just wait until the series that implements {map,unmap}_pages().

Perhaps, although the full change should be trivial enough that there's 
probably just as much argument for doing the whole thing in its own 
right for the sake of this cleanup. The main point is that 
S390_IOMMU_PGSIZES is not incorrect as such, it's just not spelling out 
the deliberate trick that it's achieving - everyone copied it from 
intel-iommu, but since that got converted to the new interfaces the 
original explanation is now gone. The only effect of "fixing" it in 
isolation right now will be to make large VFIO mappings slower.

Robin.

>>> Reviewed-by: Jason Gunthorpe <jgg@...dia.com>
>>> Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>
>>> ---
>>>    drivers/iommu/s390-iommu.c | 9 +--------
>>>    1 file changed, 1 insertion(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>>> index 94c444b909bd..6bf23e7830a2 100644
>>> --- a/drivers/iommu/s390-iommu.c
>>> +++ b/drivers/iommu/s390-iommu.c
>>> @@ -12,13 +12,6 @@
>>>    #include <linux/sizes.h>
>>>    #include <asm/pci_dma.h>
>>>    -/*
>>> - * Physically contiguous memory regions can be mapped with 4 KiB alignment,
>>> - * we allow all page sizes that are an order of 4KiB (no special large page
>>> - * support so far).
>>> - */
>>> -#define S390_IOMMU_PGSIZES    (~0xFFFUL)
>>> -
>>>    static const struct iommu_ops s390_iommu_ops;
>>>      struct s390_domain {
>>> @@ -350,7 +343,7 @@ static const struct iommu_ops s390_iommu_ops = {
>>>        .probe_device = s390_iommu_probe_device,
>>>        .release_device = s390_iommu_release_device,
>>>        .device_group = generic_device_group,
>>> -    .pgsize_bitmap = S390_IOMMU_PGSIZES,
>>> +    .pgsize_bitmap = SZ_4K,
>>>        .get_resv_regions = s390_iommu_get_resv_regions,
>>>        .default_domain_ops = &(const struct iommu_domain_ops) {
>>>            .attach_dev    = s390_iommu_attach_device,
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ