Date:   Mon, 6 Feb 2023 19:45:05 +0000
From:   Jean-Philippe Brucker <jean-philippe@linaro.org>
To:     Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        iommu@lists.linux.dev, robin.murphy@arm.com, will@kernel.org,
        joro@8bytes.org, darren@os.amperecomputing.com,
        scott@os.amperecomputing.com
Subject: Re: [PATCH] iommu/arm-smmu-v3: Enable PCI ATS in passthrough mode as
 well
Message-ID: <Y+FYwbWgOaWyHySj@myrica>
References: <20230202124053.848792-1-gankulkarni@os.amperecomputing.com>
 <Y9z3UG+wEBn4i3G2@myrica>
 <3a681b82-d840-6ef1-40dd-358f34c8be9c@os.amperecomputing.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3a681b82-d840-6ef1-40dd-358f34c8be9c@os.amperecomputing.com>
Precedence: bulk

On Mon, Feb 06, 2023 at 10:50:00PM +0530, Ganapatrao Kulkarni wrote:
> 
> 
> On 03-02-2023 05:30 pm, Jean-Philippe Brucker wrote:
> > On Thu, Feb 02, 2023 at 04:40:53AM -0800, Ganapatrao Kulkarni wrote:
> > > The current smmu-v3 driver does not enable PCI ATS for physical functions
> > > of ATS capable End Points when booted in smmu bypass mode
> > > (iommu.passthrough=1). This will not allow virtual functions to enable
> > > ATS(even though EP supports it) while they are attached to a VM using
> > > VFIO driver.
> > > 
> > > This patch adds changes to enable ATS support for physical functions
> > > in passthrough/bypass mode as well.
> > [...]
> > > @@ -2453,8 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > >   	master->domain = smmu_domain;
> > > -	if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> > > -		master->ats_enabled = arm_smmu_ats_supported(master);
> > > +	master->ats_enabled = arm_smmu_ats_supported(master);
> > 
> > I should have added a comment for this. Only found the reason in an old
> > cover letter [1]:
> > 
> > "When no translation stages are enabled (0b100), ATS Translation Requests
> > (and Translated traffic, if SMMU_CR0.ATSCHK == 1) are denied as though
> > EATS == 0b00; the actual value of the EATS field is IGNORED. Such a
> > Translation Request causes F_BAD_ATS_TREQ and Translated traffic causes
> > F_TRANSL_FORBIDDEN."
> > 
> > (See 3.9.1.1 "Responses to ATS Translation Requests and Translated
> > transactions" and 5.2 "Stream Table Entry")
> > 
> > So I don't think we can enable ATS for bypass domains :/ The PF needs to
> > be in translated mode in that case.
> 
> Are you intending to say smmu-v3 driver/spec will not support ATS to a VF,
> if it's PF is in bypass?

What I meant was that if, in order to enable the VF ATS capability, the
PCIe device requires to first enable the PF ATS capability, then given
that the SMMU does not allow enabling ATS for streams in bypass, we need
to enable translation on both PF and VF SMMU configurations.

But reading the PCIe spec, it looks like the capabilities are not
necessarily tied together:

 "The ATS Capabilities in VFs and their associated PFs are permitted to be
 enabled independently." 10.5.1 ATS Extended Capability

That wording doesn't indicate that all implementations must allow enabling
the caps independently, only that they are allowed to do so. If a device
provides independent capabilities, then I don't think you need to enable
ATS in the PF.

Then the ATS Control Register seems to contradict this with the STU field:

 "For VFs, this field must be hardwired to Zero. The associated PF's value
 applies." 10.5.1.3 ATS Control Register (Offset 06h)

So pci_enable_ats() requires that pdev->ats_stu is set in order to enable
ATS on the VF:

        /*
         * Note that enabling ATS on a VF fails unless it's already enabled
         * with the same STU on the PF.
         */
        ...
        if (dev->is_virtfn) {
                pdev = pci_physfn(dev);
                if (pdev->ats_stu != ps)
                        return -EINVAL;

But I suspect it's done this way only because it's easier to implement.
The spec doesn't require the PF ATS capability to be enabled, it just says
that the PF's STU value counts as the VF's one. It looks like we're
allowed to enable ATS on the VF without enabling it on the PF, right?
If you rework the PCI driver to only write the PF's STU without enabling
the capability, then you could enable it in the VF.

Thanks,
Jean

> 
> > 
> > I can send a patch adding the comment next cycle.
> 
> I am more keen to know, how we enable ATS to a VF of ATS capable EP when
> it's PF is in bypass?
> or it is mandatory to have a PF also translated? then that should be
> captured somewhere in documentation.
> 
> 
> > 
> > Thanks,
> > Jean
> > 
> > [1] https://lore.kernel.org/linux-iommu/20190409165245.26500-1-jean-philippe.brucker@arm.com/
> > 
> 
> Thanks,
> Ganapat