Re: [PATCH v3 10/15] intel-iommu: Implement IOMMU_ATTR_MAX_IOVA get_attr() attribute

[Philippe Mathieu-Daudé]

On 31/5/23 11:54, Joao Martins wrote:
> On 30/05/2023 22:45, Philippe Mathieu-Daudé wrote:
> > On 30/5/23 19:59, Joao Martins wrote:
> > > From: Avihai Horon <avihaih@nvidia.com>
> > >
> > > Implement get_attr() method and use the address width property to report
> > > the IOMMU_ATTR_MAX_IOVA attribute.
> > >
> > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > ---
> > >    hw/i386/intel_iommu.c | 7 +++++++
> > >    1 file changed, 7 insertions(+)


> > > +    case IOMMU_ATTR_MAX_IOVA:
> > > +    {
> > > +        hwaddr *max_iova = data;
> >
> > Shouldn't we cast to uintptr_t to be safe?
> Perhaps you mean something like this:
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 829dd6eadc6c..479307f1228f 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3878,7 +3878,7 @@ static int vtd_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
>       }
>       case IOMMU_ATTR_MAX_IOVA:
>       {
> -        hwaddr *max_iova = data;
> +        hwaddr *max_iova = (hwaddr *)(uintptr_t) data;
>
>           *max_iova = (1ULL << s->aw_bits) - 1;
>           break;
>
> I guess the thinking is to prevent 32-bit failures.

Exactly.

> > > +        *max_iova = (1ULL << s->aw_bits) - 1;
> >
> > Alternatively:
> >
> >             *max_iova = MAKE_64BIT_MASK(0, s->aw_bits);
>
> I'll switch to your suggestion. Wasn't aware of this macro :)

Thanks, it is a no-brainer when reviewing.