Message-ID: <3fc23869-cc86-9c8a-0d89-643f1f6fc1d6@redhat.com>
Date:   Thu, 24 Aug 2017 09:09:36 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Will Deacon <will.deacon@....com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>, eric.auger.pro@...il.com,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        robin.murphy@....com, Jean-Philippe.Brucker@....com,
        christoffer.dall@...aro.org, Marc.Zyngier@....com,
        alex.williamson@...hat.com, peterx@...hat.com, tn@...ihalf.com,
        bharat.bhushan@....com
Subject: Re: [RFC v2 2/4] iommu/arm-smmu-v3: Add tlbi_on_map option

Hi Will,

On 23/08/2017 18:42, Will Deacon wrote:
> Hi Eric,
> 
> On Wed, Aug 23, 2017 at 02:36:53PM +0200, Auger Eric wrote:
>> On 23/08/2017 12:25, Will Deacon wrote:
>>> On Tue, Aug 22, 2017 at 10:09:15PM +0300, Michael S. Tsirkin wrote:
>>>> On Fri, Aug 18, 2017 at 05:49:42AM +0300, Michael S. Tsirkin wrote:
>>>>> On Thu, Aug 17, 2017 at 05:34:25PM +0100, Will Deacon wrote:
>>>>>> On Fri, Aug 11, 2017 at 03:45:28PM +0200, Eric Auger wrote:
>>>>>>> When running a virtual SMMU on a guest we sometimes need to trap
>>>>>>> all changes to the translation structures. This is especially useful
>>>>>>> to integrate with VFIO. This patch adds a new option that forces
>>>>>>> the IO_PGTABLE_QUIRK_TLBI_ON_MAP to be applied on LPAE page tables.
>>>>>>>
>>>>>>> TLBI commands then can be trapped.
>>>>>>>
>>>>>>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>>>>>>
>>>>>>> ---
>>>>>>> v1 -> v2:
>>>>>>> - rebase on v4.13-rc2
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt | 4 ++++
>>>>>>>  drivers/iommu/arm-smmu-v3.c                             | 5 +++++
>>>>>>>  2 files changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>>>>> index c9abbf3..ebb85e9 100644
>>>>>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>>>>> @@ -52,6 +52,10 @@ the PCIe specification.
>>>>>>>                          devicetree/bindings/interrupt-controller/msi.txt
>>>>>>>                        for a description of the msi-parent property.
>>>>>>>  
>>>>>>> +- tlbi-on-map       : invalidate caches whenever there is an update of
>>>>>>> +                      any remapping structure (updates to not-present or
>>>>>>> +                      present entries).
>>>>>>> +
>>>>>>
>>>>>> My position on this hasn't changed, so NAK for this patch. If you want to
>>>>>> emulate something outside of the SMMUv3 architecture, please do so, but
>>>>>> don't pretend that it's an SMMUv3.
>>>>>>
>>>>>> Will
>>>>>
>>>>> What if the emulated device does not list arm,smmu-v3, listing
>>>>> qemu,ssmu-v3 as compatible? Would that address the concern?
>>>>
>>>> Will, can you comment on this please? Are you open to reusing the code
>>>> in drivers/iommu/arm-smmu-v3.c to support a paravirtual device that does
>>>> not claim to be compatible with smmuv3 but does try to behave very close to
>>>> it except it can cache non-present structures? Or would you rather
>>>> the code to support this is forked to qemu-smmu-v3.c?
>>>
>>> I still don't understand why this is preferable to a PV IOMMU
>>> implementation. Not only is this proposing to issue TLB maintenance on
>>> map, but the maintenance command itself is entirely made up. Why not just
>>> have a map command? Anyway, I'm reluctant to add this hack to the driver until:
>>>
>>>   1. There is a compelling reason to pursue this approach instead of a
>>>      PV approach (including performance measurements).
>>>
>>>   2. There is a specification for the QEMU fork of the ARM SMMUv3
>>>      architecture, including the semantics of the new command being proposed
>>>      and what exactly the TLB maintenance requirements are on map (for
>>>      example, what if I change an STE or a CD -- are they cached too?).
>> I am not sure I catch this last point. At the moment whenever the smmuv3
>> driver issues data structure invalidation commands (CMD_CFGI_*), those
>> are trapped and I replay the mappings on host side. I have not changed
>> anything on that side.
> 
> But STEs and CDs have very similar rules to TLBs: you don't need to issue
> invalidation if the data structure is transitioning from invalid to valid.
> If you're caching those in QEMU, how do you keep them up-to-date? I can
> also guarantee you that there will be additional data structures added
> in future versions of the architecture, so you'll need to consider how
> you want to operate when running on newer hardware.
OK I get your point now. I thought the driver was currently sending
CMD_CFGI_STE on each config change and I did not notice any issue yet
with the various use cases. So that should be part of the FW quirk to
guarantee that each time this happens, commands are sent for QEMU to trap.

This FW quirk was devised to do exactly the same as the Intel vtd
caching mode. Technically I guess Intel driver implements the exact same
hooks (but they did to comply with the spec, I agree).

Thanks

Eric
> 
>> I introduced a new map implementation defined command because the per
>> page CMD_TLBI_NH_VA IOVA invalidation command was not efficient/usable
>> with use cases such as DPDK on guest. I understood the spec provisions
>> for such implementation defined commands.
> 
> Whilst there is a space for IMP DEF commands, this doesn't generally mean
> that they can be repurposed by software. What if the underlying hardware
> has an IMP DEF command that you want to export? Besides, my main points
> here are that your command isn't well-specified and if you have to add
> a command, why not just add a "map" command (i.e. implement a PV interface
> instead)?
> 
>>>   3. The ACPI IORT spec is updated to recognise this implementation
>>>
>>>   4. There is an implementation that can use the guest page tables directly,
>>>      because that may well make all of this moot.
>> Most probably I will come back to you with questions on stage 1 + stage2
>> enablement and "4.8 Virtualisation" chapter of smmuv3 spec. Besides I
>> also need to get access to some HW with smmuv3 ;-)
> 
> Ok.
> 
> Will
> 

Powered by blists - more mailing lists