linux-kernel - Re: [PATCH v2 4/4] vfio: vfio_iommu_type1: implement VFIO_IOMMU_INFO

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]

Date:   Mon, 20 May 2019 13:19:23 +0200
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     sebott@...ux.vnet.ibm.com, gerald.schaefer@...ibm.com,
        pasic@...ux.vnet.ibm.com, borntraeger@...ibm.com,
        walling@...ux.ibm.com, linux-s390@...r.kernel.org,
        iommu@...ts.linux-foundation.org, joro@...tes.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        robin.murphy@....com
Subject: Re: [PATCH v2 4/4] vfio: vfio_iommu_type1: implement
 VFIO_IOMMU_INFO_CAPABILITIES

On 17/05/2019 20:04, Pierre Morel wrote:
> On 17/05/2019 18:41, Alex Williamson wrote:
>> On Fri, 17 May 2019 18:16:50 +0200
>> Pierre Morel <pmorel@...ux.ibm.com> wrote:
>>
>>> We implement the capability interface for VFIO_IOMMU_GET_INFO.
>>>
>>> When calling the ioctl, the user must specify
>>> VFIO_IOMMU_INFO_CAPABILITIES to retrieve the capabilities and
>>> must check in the answer if capabilities are supported.
>>>
>>> The iommu get_attr callback will be used to retrieve the specific
>>> attributes and fill the capabilities.
>>>
>>> Currently two Z-PCI specific capabilities will be queried and
>>> filled by the underlying Z specific s390_iommu:
>>> VFIO_IOMMU_INFO_CAP_QFN for the PCI query function attributes
>>> and
>>> VFIO_IOMMU_INFO_CAP_QGRP for the PCI query function group.
>>>
>>> Other architectures may add new capabilities in the same way
>>> after enhancing the architecture specific IOMMU driver.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@...ux.ibm.com>
>>> ---
>>>   drivers/vfio/vfio_iommu_type1.c | 122 
>>> +++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 121 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c 
>>> b/drivers/vfio/vfio_iommu_type1.c
>>> index d0f731c..9435647 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -1658,6 +1658,97 @@ static int 
>>> vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>>>       return ret;
>>>   }
>>> +static int vfio_iommu_type1_zpci_fn(struct iommu_domain *domain,
>>> +                    struct vfio_info_cap *caps, size_t size)
>>> +{
>>> +    struct vfio_iommu_type1_info_pcifn *info_fn;
>>> +    int ret;
>>> +
>>> +    info_fn = kzalloc(size, GFP_KERNEL);
>>> +    if (!info_fn)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_ZPCI_FN,
>>> +                    &info_fn->response);
>>
>> What ensures that the 'struct clp_rsp_query_pci' returned from this
>> get_attr remains consistent with a 'struct vfio_iommu_pci_function'?
>> Why does the latter contains so many reserved fields (beyond simply
>> alignment) for a user API?  What fields of these structures are
>> actually useful to userspace?  Should any fields not be exposed to the
>> user?  Aren't BAR sizes redundant to what's available through the vfio
>> PCI API?  I'm afraid that simply redefining an internal structure as
>> the API leaves a lot to be desired too.  Thanks,
>>
>> Alex
>>
> Hi Alex,
> 
> I simply used the structure returned by the firmware to be sure to be 
> consistent with future evolutions and facilitate the copy from CLP and 
> to userland.
> 
> If you prefer, and I understand that this is the case, I can define a 
> specific VFIO_IOMMU structure with only the fields relevant to the user, 
> leaving future enhancement of the user's interface being implemented in 
> another kernel patch when the time has come.
> 
> In fact, the struct will have all defined fields I used but not the BAR 
> size and address (at least for now because there are special cases we do 
> not support yet with bars).
> All the reserved fields can go away.
> 
> Is it more conform to your idea?
> 
> Also I have 2 interfaces:
> 
> s390_iommu.get_attr <-I1-> VFIO_IOMMU <-I2-> userland
> 
> Do you prefer:
> - 2 different structures, no CLP raw structure
> - the CLP raw structure for I1 and a VFIO specific structure for I2

Hi Alex,

I am back again on this.
This solution here above seems to me the best one but in this way I must 
include S390 specific include inside the iommu_type1, which is AFAIU not 
a good thing.
It seems that the powerpc architecture use a solution with a dedicated 
VFIO_IOMMU, the vfio_iommu_spar_tce.

Wouldn't it be a solution for s390 too, to use the vfio_iommu_type1 as a 
basis to have a s390 dedicated solution.
Then it becomes easier to have on one side the s390_iommu interface, 
S390 specific, and on the other side a VFIO interface without a blind 
copy of the firmware values.

Do you think it is a viable solution?

Thanks,
Pierre



> - the same VFIO structure for both I1 and I2
> 
> Thank you if you could give me a direction for this.
> 
> Thanks for the comments, and thanks a lot to have answered so quickly.
> 
> Pierre
> 
> 
> 
> 
> 
> 
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany