On Tue, Feb 18, 2025 at 01:18:21PM -0400, Jason Gunthorpe wrote:
> On Fri, Jan 24, 2025 at 04:30:42PM -0800, Nicolin Chen wrote:
>
> > @@ -1831,31 +1831,30 @@ static int arm_smmu_handle_event(struct
> > arm_smmu_device *smmu,
> > return -EOPNOTSUPP;
> > }
>
> There is still the filter at the top:
>
> switch (event->id) {
> case EVT_ID_TRANSLATION_FAULT:
> case EVT_ID_ADDR_SIZE_FAULT:
> case EVT_ID_ACCESS_FAULT:
> case EVT_ID_PERMISSION_FAULT:
> break;
> default:
> return -EOPNOTSUPP;
> }
>
> Is that right here or should more event types be forwarded to the
> guest?
That doesn't seem to be right. Something like EVT_ID_BAD_CD_CONFIG
should be forwarded too. I will go through the list.
> > mutex_lock(&smmu->streams_mutex);
> [..]
>
> > - ret = iommu_report_device_fault(master->dev, &fault_evt);
> > + if (event->stall) {
> > + ret = iommu_report_device_fault(master->dev, &fault_evt);
> > + } else {
> > + down_read(&master->vmaster_rwsem);
>
> This already holds the streams_mutex across all of this, do you think
> we should get rid of the vmaster_rwsem and hold the streams_mutex on
> write instead?
They are per master v.s. per smmu. The latter one would make master
commits/attaches exclusive, which feels unnecessary to me, although
it would make the code here slightly cleaner..
Thanks
Nicolin
