From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> To: Jacob Pan <jacob.jun.pan@linux.intel.com>, Robin Murphy <robin.murphy@arm.com> Cc: yi.l.liu@linux.intel.com, ashok.raj@intel.com, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, alex.williamson@redhat.com Subject: Re: [PATCH 2/4] iommu: Introduce device fault data Date: Fri, 24 May 2019 17:14:30 +0100 [thread overview] Message-ID: <3f512c57-de7c-dc3b-049c-2c4745757636@arm.com> (raw) In-Reply-To: <20190524064924.0cc92ae3@jacob-builder> On 24/05/2019 14:49, Jacob Pan wrote: > On Thu, 23 May 2019 19:43:46 +0100 > Robin Murphy <robin.murphy@arm.com> wrote: >>> +/** >>> + * struct iommu_fault_event - Generic fault event >>> + * >>> + * Can represent recoverable faults such as a page requests or >>> + * unrecoverable faults such as DMA or IRQ remapping faults. >>> + * >>> + * @fault: fault descriptor >>> + * @iommu_private: used by the IOMMU driver for storing >>> fault-specific >>> + * data. Users should not modify this field before >>> + * sending the fault response. >> >> Sorry if I'm a bit late to the party, but given that description, if >> users aren't allowed to touch this then why expose it to them at all? >> I.e. why not have iommu_report_device_fault() pass just the fault >> itself to the fault handler: >> >> ret = fparam->handler(&evt->fault, fparam->data); >> >> and let the IOMMU core/drivers decapsulate it again later if need be. >> AFAICS drivers could also just embed the entire generic event in >> their own private structure anyway, just as we do for domains. >> > I can't remember all the discussion history but I think iommu_private > is used similarly to the page request private data (device private). Hm yes, we already have iommu_fault_page_request::private_data for that. I think I used to stash flags in iommu_private (is_stall and needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them from the device structure, but I removed them. If VT-d doesn't need iommu_private either, maybe we can remove it entirely? In any case I agree that device drivers should only need to know about evt->fault. > We > need to inject the data to the guest and the guest will send the > unmodified data back along with response. By the way, does private_data need to go back through the iommu_page_response() path? The current series doesn't do that. > The private data can be used > to tag internal device/iommu context. > I think we can do the way you said by keeping them within iommu core > and recover it based on the response but that would require tracking > each fault report, right? That's already the case: we decided in thread [1] to track recoverable faults in the IOMMU core, in order to check that the response is sane and to set a quota and/or timeout. (I didn't include your timeout patches here because I think they need a little more work. They are on my sva/api branch.) I already dropped iommu_private from the iommu_page_response structure. In patch 4 iommu_page_response() retrieves the fault event and pass the corresponding iommu_private back to the IOMMU driver. [1] https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/ Thanks, Jean > > If we pass on the private data, we only need to check if the response > belong to the device but not exact match of a specific fault since the > damage is contained in the assigned device. In case of injection > fault into the guest, the response will come asynchronously after the > handler completes.
WARNING: multiple messages have this Message-ID (diff)
From: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> To: Jacob Pan <jacob.jun.pan@linux.intel.com>, Robin Murphy <robin.murphy@arm.com> Cc: yi.l.liu@linux.intel.com, iommu@lists.linux-foundation.org, alex.williamson@redhat.com, ashok.raj@intel.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] iommu: Introduce device fault data Date: Fri, 24 May 2019 17:14:30 +0100 [thread overview] Message-ID: <3f512c57-de7c-dc3b-049c-2c4745757636@arm.com> (raw) In-Reply-To: <20190524064924.0cc92ae3@jacob-builder> On 24/05/2019 14:49, Jacob Pan wrote: > On Thu, 23 May 2019 19:43:46 +0100 > Robin Murphy <robin.murphy@arm.com> wrote: >>> +/** >>> + * struct iommu_fault_event - Generic fault event >>> + * >>> + * Can represent recoverable faults such as a page requests or >>> + * unrecoverable faults such as DMA or IRQ remapping faults. >>> + * >>> + * @fault: fault descriptor >>> + * @iommu_private: used by the IOMMU driver for storing >>> fault-specific >>> + * data. Users should not modify this field before >>> + * sending the fault response. >> >> Sorry if I'm a bit late to the party, but given that description, if >> users aren't allowed to touch this then why expose it to them at all? >> I.e. why not have iommu_report_device_fault() pass just the fault >> itself to the fault handler: >> >> ret = fparam->handler(&evt->fault, fparam->data); >> >> and let the IOMMU core/drivers decapsulate it again later if need be. >> AFAICS drivers could also just embed the entire generic event in >> their own private structure anyway, just as we do for domains. >> > I can't remember all the discussion history but I think iommu_private > is used similarly to the page request private data (device private). Hm yes, we already have iommu_fault_page_request::private_data for that. I think I used to stash flags in iommu_private (is_stall and needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them from the device structure, but I removed them. If VT-d doesn't need iommu_private either, maybe we can remove it entirely? In any case I agree that device drivers should only need to know about evt->fault. > We > need to inject the data to the guest and the guest will send the > unmodified data back along with response. By the way, does private_data need to go back through the iommu_page_response() path? The current series doesn't do that. > The private data can be used > to tag internal device/iommu context. > I think we can do the way you said by keeping them within iommu core > and recover it based on the response but that would require tracking > each fault report, right? That's already the case: we decided in thread [1] to track recoverable faults in the IOMMU core, in order to check that the response is sane and to set a quota and/or timeout. (I didn't include your timeout patches here because I think they need a little more work. They are on my sva/api branch.) I already dropped iommu_private from the iommu_page_response structure. In patch 4 iommu_page_response() retrieves the fault event and pass the corresponding iommu_private back to the IOMMU driver. [1] https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/ Thanks, Jean > > If we pass on the private data, we only need to check if the response > belong to the device but not exact match of a specific fault since the > damage is contained in the assigned device. In case of injection > fault into the guest, the response will come asynchronously after the > handler completes. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2019-05-24 16:14 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-23 18:06 [PATCH 0/4] iommu: Add device fault reporting API Jean-Philippe Brucker 2019-05-23 18:06 ` Jean-Philippe Brucker 2019-05-23 18:06 ` [PATCH 1/4] driver core: Add per device iommu param Jean-Philippe Brucker 2019-05-23 18:06 ` Jean-Philippe Brucker 2019-05-23 18:06 ` [PATCH 2/4] iommu: Introduce device fault data Jean-Philippe Brucker 2019-05-23 18:06 ` Jean-Philippe Brucker 2019-05-23 18:43 ` Robin Murphy 2019-05-23 18:43 ` Robin Murphy 2019-05-24 13:49 ` Jacob Pan 2019-05-24 13:49 ` Jacob Pan 2019-05-24 16:14 ` Jean-Philippe Brucker [this message] 2019-05-24 16:14 ` Jean-Philippe Brucker 2019-05-24 17:44 ` Jacob Pan 2019-05-24 17:44 ` Jacob Pan 2019-05-23 18:06 ` [PATCH 3/4] iommu: Introduce device fault report API Jean-Philippe Brucker 2019-05-23 18:06 ` Jean-Philippe Brucker 2019-05-23 18:56 ` Robin Murphy 2019-05-23 18:56 ` Robin Murphy 2019-05-24 18:00 ` Jacob Pan 2019-05-24 18:00 ` Jacob Pan 2019-05-31 13:37 ` Jean-Philippe Brucker 2019-05-31 13:37 ` Jean-Philippe Brucker 2019-05-23 18:06 ` [PATCH 4/4] iommu: Add recoverable fault reporting Jean-Philippe Brucker 2019-05-23 18:06 ` Jean-Philippe Brucker 2019-05-24 18:14 ` Jacob Pan 2019-05-24 18:14 ` Jacob Pan 2019-05-31 11:05 ` Jean-Philippe Brucker 2019-05-31 11:05 ` Jean-Philippe Brucker
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=3f512c57-de7c-dc3b-049c-2c4745757636@arm.com \ --to=jean-philippe.brucker@arm.com \ --cc=alex.williamson@redhat.com \ --cc=ashok.raj@intel.com \ --cc=iommu@lists.linux-foundation.org \ --cc=jacob.jun.pan@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=robin.murphy@arm.com \ --cc=yi.l.liu@linux.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.