linux-kernel - Re: [PART2 PATCH v6 12/12] svm: Implements update_pi

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: <57BACF73.8020401@amd.com>
Date:   Mon, 22 Aug 2016 17:09:55 +0700
From:   Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:     Radim Krčmář <rkrcmar@...hat.com>
CC:     <joro@...tes.org>, <pbonzini@...hat.com>,
        <alex.williamson@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <sherry.hurwitz@....com>
Subject: Re: [PART2 PATCH v6 12/12] svm: Implements update_pi_irte hook to
 setup posted interrupt

Hi Radim,

On 08/22/2016 04:19 PM, Suravee Suthikulpanit wrote:
>> he problem with wrappers is that we don't know what list we should
>> remove the "struct amd_ir_data" from;  we would need to add another
>> tracking structure or go through all VCPUs.
>>
>> Having "struct list_head" in "struct amd_ir_data" would allow us to know
>> the current list and remove it from there:
>> One "struct amd_ir_data" should never be used by more than one caller of
>> amd_iommu_update_ga(), because they would have to be cooperating anyway,
>> which would mean a single mediator, so we can add a "struct list_head"
>> into "struct amd_ir_data".
>>
>>    Minor design note:
>>    To make the usage of "struct amd_ir_data" safer, we could pass "struct
>>    list_head" into irq_set_vcpu_affinity(), instead of returning "struct
>>    amd_ir_data *".
>>
>>    irq_set_vcpu_affinity() would add "struct amd_ir_data" to the list
>> only
>>    if ir_data was not already in some list and report whether the list
>>    was modified.
>>
>> I think that adding "struct list_head" into "struct amd_ir_data" is
>> nicer than having wrappers.
>>
>> Joerg, Paolo, what do you think?
>>
>
> I think modifying irq_set_vcpu_affinity() to also pass struct list_head
> seems a bit redundant since it is currently design to allow passing in
> void *, which leaves the other option where we might just need to pass
> in a wrapper (e.g. going back to the previous design where we pass in
> struct amd_iommu_pi_data) and also add a pointer to the ir_list in the
> wrapper as well. Then, IOMMU is responsible for adding/deleting ir_data
> to/from this list instead of SVM. This should be fine since we only need
> to coordinate b/w SVM and AMD-IOMMU.
>
> Thanks,
> Suravee

Actually, thinking about this again, going back to keeping the per-vcpu 
list of struct amd_iommu_pi_data is probably the simplest here.

* We avoid having to expose the amd_ir_data to SVM.
* We can match using amd_ir_data * when traversing the list.
* We can easily add the code to manage the list in the SVM. We can make 
sure that the struct amd_iommu_pi_data is not already mapped before 
adding it to a new per-vcpu list. If it is currently mapped, we can 
simply unmapped it. Doing this from IOMMU would be more complicate and 
require lots of parameter passing.

Thanks,
Suravee