qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Minwoo Im <minwoo.im@samsung.com>
To: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"zhenzhong.duan@intel.com" <zhenzhong.duan@intel.com>,
	"kevin.tian@intel.com" <kevin.tian@intel.com>,
	"yi.l.liu@intel.com" <yi.l.liu@intel.com>,
	"joao.m.martins@oracle.com" <joao.m.martins@oracle.com>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"mst@redhat.com" <mst@redhat.com>,
	minwoo.im@samsung.com, jongh2.jeong@samsung.com
Subject: Re: [PATCH ats_vtd v5 20/22] pci: add a pci-level API for ATS
Date: Thu, 18 Jul 2024 08:44:02 +0900	[thread overview]
Message-ID: <ZphXQjbDzezleP8R@localhost> (raw)
In-Reply-To: <a1d82288-a3a7-46f0-b6bd-bb636ecc147d@eviden.com>

[-- Attachment #1: Type: text/plain, Size: 6581 bytes --]

On 24-07-11 19:00:58, CLEMENT MATHIEU--DRIF wrote:
> 
> 
> On 11/07/2024 10:04, Minwoo Im wrote:
> > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> >
> >
> > On 24-07-10 05:17:42, CLEMENT MATHIEU--DRIF wrote:
> >>
> >> On 09/07/2024 23:17, Minwoo Im wrote:
> >>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> >>>
> >>>
> >>> On 24-07-09 11:58:53, CLEMENT MATHIEU--DRIF wrote:
> >>>> On 09/07/2024 12:15, Minwoo Im wrote:
> >>>>> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> >>>>>
> >>>>>
> >>>>> On 24-07-02 05:52:45, CLEMENT MATHIEU--DRIF wrote:
> >>>>>> From: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >>>>>>
> >>>>>> Devices implementing ATS can send translation requests using
> >>>>>> pci_ats_request_translation_pasid.
> >>>>>>
> >>>>>> The invalidation events are sent back to the device using the iommu
> >>>>>> notifier managed with pci_register_iommu_tlb_event_notifier and
> >>>>>> pci_unregister_iommu_tlb_event_notifier
> >>>>>>
> >>>>>> Signed-off-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>
> >>>>>> ---
> >>>>>>     hw/pci/pci.c         | 44 +++++++++++++++++++++++++++++++++++++
> >>>>>>     include/hw/pci/pci.h | 52 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>>>     2 files changed, 96 insertions(+)
> >>>>>>
> >>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>>> index 7a483dd05d..93b816aff2 100644
> >>>>>> --- a/hw/pci/pci.c
> >>>>>> +++ b/hw/pci/pci.c
> >>>>>> @@ -2833,6 +2833,50 @@ void pci_device_unset_iommu_device(PCIDevice *dev)
> >>>>>>         }
> >>>>>>     }
> >>>>>>
> >>>>>> +ssize_t pci_ats_request_translation_pasid(PCIDevice *dev, uint32_t pasid,
> >>>>>> +                                          bool priv_req, bool exec_req,
> >>>>>> +                                          hwaddr addr, size_t length,
> >>>>>> +                                          bool no_write, IOMMUTLBEntry *result,
> >>>>>> +                                          size_t result_length,
> >>>>>> +                                          uint32_t *err_count)
> >>>>>> +{
> >>>>>> +    assert(result_length);
> >>>>>> +    IOMMUMemoryRegion *iommu_mr = pci_device_iommu_memory_region_pasid(dev,
> >>>>>> +                                                                        pasid);
> >>>>>> +    if (!iommu_mr || !pcie_ats_enabled(dev)) {
> >>>>>> +        return -EPERM;
> >>>>>> +    }
> >>>>>> +    return memory_region_iommu_ats_request_translation(iommu_mr, priv_req,
> >>>>>> +                                                       exec_req, addr, length,
> >>>>>> +                                                       no_write, result,
> >>>>>> +                                                       result_length,
> >>>>>> +                                                       err_count);
> >>>>>> +}
> >>>>> Can we use this function not from the endpoint PCI device, but inside of the pci
> >>>>> subsystem (hw/pci/pci.c) to make transparent abstraction for ATS request from
> >>>>> PCI endpoint device POV?  I guess it would be better to have PCI subsystem to
> >>>>> issue ATS request if pcie_ats_enabled(dev) rather than calling from the endpoint
> >>>>> side.
> >>>> Hi,
> >>>>
> >>>> This series aims to bring support for SVM (we are trying to integrate
> >>>> the patches bit by bit).
> >>>>    From a spec point of view, I don't know if it would make sense to
> >>>> implement the SVM logic at the PCI level
> >>>> as it's supposed to be implemented by endpoint devices.
> >>> Understood that this series is targeting the SVM usage.  But ATS feature is
> >>> something general to PCI devices, not only just for SVM, so I guess it would be
> >>> better to have caller to `pci_ats_request_translation_pasid()` in pci subsystem
> >>> like pci_dma_rw() to avoid duplicated implementation in the future for the
> >>> other PCI enpoint devices.
> >> Would we store the ATC directly in the PCI subsytem?
> > Yes, endpoint device (e.g., svm.c) should call pci_* helpers in PCI subsystem
> > with `PCIDevice *pdev instance` which represents the endpoint device itself.
> > By the instance, we can look up the IOTLB entry from the ATC in the PCI
> > subsystem, not the current caller side.
> >
> >>>> However, we could consider providing a reference/reusable/encapsulated
> >>>> implementation of SVM with a simplified API
> >>>> that would call the pci_* functions under the hood.
> >>> I would prefer that PCI devices which want to request ATS translation has no
> >>> additional implementation for ATS, but only pcie_ats_init().
> >> Hi,
> >>
> >> I think both strategies can coexist.
> >> Keeping control can be interesting for people who use Qemu for hardware
> >> prototyping and who generally want to experiment.
> >> We can keep the current PCI-level API for devices that want to
> >> reimplement the logic themselves
> >> and add a kind of "DMA module"/"ATS+PRI module" that works out of the box.
> > I think we should proivde hybrid mode on this.  One for a `generic` cache
> > policy mode for every PCI endpoint devices which can be controlled in the PCI
> > subsystem for ATC, the other one is that device-specific cache policy mode
> > which let each device implement their own ATC lookup behaviors to optimize
> > their own caching impact.
> >
> >> That module could be called "struct PciDmaModule" and expose a simple
> >> set of functions like pci_dma_module_init, pci_dma_module_read,
> >> pci_dma_module_write.
> >> I think it's important to keep existing DMA API as is to allow devices
> >> to do both "with ATS" and "without ATS" operations.
> >>
> >> Do you agree with that?
> > Indeed.  Keeping the existing APIs is a good choice, but I would like to have
> > endpoint devices code much more simpler for the generic usages :)
> That's a good point, we will se what we can do once the current work is 
> integrated.
> Thanks for your comment :)

Do you have a plan to repost this series soon? I would like to apply your
patches and review/test this series.  It looks like you've been through some of
fix patches for VT-d, but I'm curious your further plan for the actual SVM
feature :)

> >
> >>>> Do you have a specific use case in mind?
> >>> ATS/PRI is the actual use case, and it's not that different what you are
> >>> targeting for :)
> >>>
> >>>>    >cmd
> >>>>
> >>>>>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



  reply	other threads:[~2024-07-17 23:58 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-02  5:52 [PATCH ats_vtd v5 00/22] ATS support for VT-d CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 01/22] intel_iommu: fix FRCD construction macro CLEMENT MATHIEU--DRIF
2024-07-02 13:01   ` Yi Liu
2024-07-02 15:10     ` CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 02/22] intel_iommu: make types match CLEMENT MATHIEU--DRIF
2024-07-02 13:20   ` Yi Liu
2024-07-02  5:52 ` [PATCH ats_vtd v5 03/22] intel_iommu: return page walk level even when the translation fails CLEMENT MATHIEU--DRIF
2024-07-03 11:59   ` Yi Liu
2024-07-04  4:23     ` CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 04/22] intel_iommu: do not consider wait_desc as an invalid descriptor CLEMENT MATHIEU--DRIF
2024-07-02 13:33   ` Yi Liu
2024-07-02 15:29     ` CLEMENT MATHIEU--DRIF
2024-07-02 15:40       ` cmd
2024-07-03  7:29       ` Yi Liu
2024-07-03  8:28         ` cmd
2024-07-04  4:23         ` CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 05/22] memory: add permissions in IOMMUAccessFlags CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 06/22] pcie: add helper to declare PASID capability for a pcie device CLEMENT MATHIEU--DRIF
2024-07-03 12:04   ` Yi Liu
2024-07-04  4:25     ` CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 08/22] intel_iommu: declare supported PASID size CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 07/22] pcie: helper functions to check if PASID and ATS are enabled CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 09/22] pci: cache the bus mastering status in the device CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 11/22] memory: store user data pointer in the IOMMU notifiers CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 10/22] pci: add IOMMU operations to get address spaces and memory regions with PASID CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 12/22] pci: add a pci-level initialization function for iommu notifiers CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 13/22] intel_iommu: implement the get_address_space_pasid iommu operation CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 15/22] memory: Allow to store the PASID in IOMMUTLBEntry CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 14/22] intel_iommu: implement the get_memory_region_pasid iommu operation CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 16/22] intel_iommu: fill the PASID field when creating an instance of IOMMUTLBEntry CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 17/22] atc: generic ATC that can be used by PCIe devices that support SVM CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 18/22] atc: add unit tests CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 19/22] memory: add an API for ATS support CLEMENT MATHIEU--DRIF
2024-07-03 12:14   ` Yi Liu
2024-07-04  4:30     ` CLEMENT MATHIEU--DRIF
2024-07-04 12:52       ` Yi Liu
2024-07-02  5:52 ` [PATCH ats_vtd v5 20/22] pci: add a pci-level API for ATS CLEMENT MATHIEU--DRIF
2024-07-09 10:15   ` Minwoo Im
2024-07-09 11:58     ` CLEMENT MATHIEU--DRIF
2024-07-09 21:17       ` Minwoo Im
2024-07-10  5:17         ` CLEMENT MATHIEU--DRIF
2024-07-11  8:04           ` Minwoo Im
2024-07-11 19:00             ` CLEMENT MATHIEU--DRIF
2024-07-17 23:44               ` Minwoo Im [this message]
2024-07-18  7:46                 ` CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 22/22] intel_iommu: add support " CLEMENT MATHIEU--DRIF
2024-07-02  5:52 ` [PATCH ats_vtd v5 21/22] intel_iommu: set the address mask even when a translation fails CLEMENT MATHIEU--DRIF
2024-07-02 12:16 ` [PATCH ats_vtd v5 00/22] ATS support for VT-d Michael S. Tsirkin
2024-07-02 15:09   ` CLEMENT MATHIEU--DRIF
2024-07-02 13:44 ` Yi Liu
2024-07-02 15:12   ` CLEMENT MATHIEU--DRIF
2024-07-03 12:32 ` Yi Liu
2024-07-04  4:36   ` CLEMENT MATHIEU--DRIF
2024-07-04  8:14     ` Yi Liu
  -- strict thread matches above, loose matches on Subject: below --
2024-06-03  5:59 CLEMENT MATHIEU--DRIF
2024-06-03  5:59 ` [PATCH ats_vtd v5 20/22] pci: add a pci-level API for ATS CLEMENT MATHIEU--DRIF

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZphXQjbDzezleP8R@localhost \
    --to=minwoo.im@samsung.com \
    --cc=clement.mathieu--drif@eviden.com \
    --cc=jasowang@redhat.com \
    --cc=joao.m.martins@oracle.com \
    --cc=jongh2.jeong@samsung.com \
    --cc=kevin.tian@intel.com \
    --cc=mst@redhat.com \
    --cc=peterx@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=yi.l.liu@intel.com \
    --cc=zhenzhong.duan@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).