Re: [PATCH 3/6] amd_iommu: Update bitmasks representing DTE reserved fields

[Vasant Hegde <vasant.hegde@amd.com>]

Hi ,


On 3/13/2025 7:53 PM, Alejandro Jimenez wrote:
> 
> 
> On 3/12/25 12:12 AM, Arun Kodilkar, Sairaj wrote:
>> Hi Alejandro,
>>
>> On 3/11/2025 8:54 PM, Alejandro Jimenez wrote:
> 
> [...]
> 
>>> --- a/hw/i386/amd_iommu.h
>>> +++ b/hw/i386/amd_iommu.h
>>> @@ -25,6 +25,8 @@
>>>   #include "hw/i386/x86-iommu.h"
>>>   #include "qom/object.h"
>>> +#define GENMASK64(h, l)  (((~0ULL) >> (63 - (h) + (l))) << (l))
>>> +
>>
>> qemu provides the similar macro 'MAKE_64BIT_MASK' in file
>> 'include/qemu/bitops.h', you can use this existing macro
>> instead of redefining.
> 
> Hi Sairaj,
> 
> I became aware of MAKE_64BIT_MASK() because you used it in your recent patch,
> but as you mentioned they are similar but not the same. I personally find that
> using bit indexes is less prone to errors since that is the same format the spec
> uses to define the fields.
> So translating a RSVD[6:2] field from the spec becomes:
> 
> GENMASK64(6, 2);
> vs
> MAKE_64BIT_MASK(6, 5);
> 
> The latter is more prone to off-by-one errors in my opinion, specially when you
> are defining lots of masks. Perhaps more importantly, I'd like to progressively
> convert the amd_iommu definitions to use GENMASK() and the code that retrieves
> bit fields to use FIELD_GET().
> 
> I am planning on later porting the generic GENMASK* definitions (from the kernel
> into "qemu/bitops.h", since the RISC-V IOMMU is also a consumer of GENMASK, but
> I am trying to keep the focus on the AMD vIOMMU for this series.

I like GENMASK. Its easy to read.

RISCV has GENMASK_ULL. As you said, we can consider consolidating them later.

-Vasant