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
