Re: [RFC PATCH 2/6] iommu/amd: Add support for hw_info for iommu capability query

[Suthikulpanit, Suravee]

Hi Jason

On 12/13/2023 8:27 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 10:01:35AM -0600, Suravee Suthikulpanit wrote:
> > AMD IOMMU Extended Feature (EFR) and Extended Feature 2 (EFR2) registers
> >
> > ...
> >
> > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > index 4e4ff1550cf3..c41932e9f16a 100644
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -2822,8 +2822,46 @@ static const struct iommu_dirty_ops amd_dirty_ops = {
> >   	.read_and_clear_dirty = amd_iommu_read_and_clear_dirty,
> >   };
> >   
> > +void amd_iommu_build_efr(u64 *efr, u64 *efr2)
> > +{
> > +	if (efr) {
> > +		*efr = (FEATURE_GT | FEATURE_GIOSUP | FEATURE_PPR);
> > +
> > +		/* 5-level v2 page table support */
> > +		*efr |= ((FEATURE_GATS_5LEVEL << FEATURE_GATS_SHIFT) &
> > +			 FEATURE_GATS_MASK);
> > +
> > +		/* 3-level GCR3 table support */
> > +		*efr |= ((FEATURE_GLX_3LEVEL << FEATURE_GLX_SHIFT) &
> > +			 FEATURE_GLX_MASK);
> > +
> > +		/* 16-bit PASMAX support */
> > +		*efr |= ((FEATURE_PASMAX_16 << FEATURE_PASMAX_SHIFT) &
> > +			 FEATURE_PASMAX_MASK);
> > +	}
> > +
> > +	if (efr2)
> > +		*efr2 = 0;
>
> Why are you checking for null here? It is never called with null

In subsequent patches of the part 2, this helper function will be used 
to help populate the EFR and/or EFR2 in different call paths, which can 
pass only efr or efr2 parameter individually.

> > +/**
> > + * struct iommu_hw_info_amd - AMD IOMMU device info
> > + *
> > + * @efr : Value of AMD IOMMU Extended Feature Register (EFR)
> > + * @efr2: Value of AMD IOMMU Extended Feature 2 Register (EFR2)
>
> Please reference a section in the spec for what these are just for
> clarity

Sure

> > + */
> > +struct iommu_hw_info_amd {
> > +	__u64 efr;
> > +	__u64 efr2;
> > +};
>
> __aligned_u64

Okey.

Thanks,
Suravee