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

["Arun Kodilkar, Sairaj" <sarunkod@amd.com>]

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.
> 
> Thank you,
> Alejandro
> 

Hi Alejandro,
Yes indeed, GENMASK64() is more readable than the MAKE_64BIT_MASK().
and Since you are going to port it to generic layer, its better
to use GENMASK64().
Feel free to replace existing MAKE_64BIT_MASK() in my recent patches.
otherwise, I can modify it during next patch series.

Regards
Sairaj Kodilkar

>>
>>>   /* Capability registers */
>>>   #define AMDVI_CAPAB_BAR_LOW           0x04
>>>   #define AMDVI_CAPAB_BAR_HIGH          0x08
>>> @@ -162,9 +164,10 @@
>>>   #define AMDVI_FEATURE_PC                  (1ULL << 9) /* Perf 
>>> counters       */
>>>   /* reserved DTE bits */
>>> -#define AMDVI_DTE_LOWER_QUAD_RESERVED  0x80300000000000fc
>>> -#define AMDVI_DTE_MIDDLE_QUAD_RESERVED 0x0000000000000100
>>> -#define AMDVI_DTE_UPPER_QUAD_RESERVED  0x08f0000000000000
>>> +#define AMDVI_DTE_QUAD0_RESERVED        (GENMASK64(6, 2) | 
>>> GENMASK64(63, 63))
>>> +#define AMDVI_DTE_QUAD1_RESERVED        0
>>> +#define AMDVI_DTE_QUAD2_RESERVED        GENMASK64(53, 52)
>>> +#define AMDVI_DTE_QUAD3_RESERVED        (GENMASK64(14, 0) | 
>>> GENMASK64(53, 48))
>>>   /* AMDVI paging mode */
>>>   #define AMDVI_GATS_MODE                 (2ULL <<  12)
>>
>> Regards
>> Sairaj Kodilkar
>