On Wed, Feb 13, 2019 at 04:03:05PM +0800, Peter Xu wrote: > On Wed, Feb 13, 2019 at 03:38:55PM +0800, Yi Sun wrote: > > [...] > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > index 8b72735..396ac8e 100644 > > > > --- a/hw/i386/intel_iommu.c > > > > +++ b/hw/i386/intel_iommu.c > > > > @@ -37,6 +37,34 @@ > > > > #include "kvm_i386.h" > > > > #include "trace.h" > > > > > > > > +#define vtd_devfn_check(devfn) ((devfn & VTD_DEVFN_CHECK_MASK) ? true > > > > : false) > > > > > > "vtd_devfn_check(devfn)" is merely as long as "devfn & > > > VTD_DEVFN_CHECK_MASK", isn't it? :) > > > > > > I would just drop the macro. > > > > > There are two places to call this macro. Is that valuable to keep it? > > The point here is "A ? true : false" is exactly "A" when used in > condition checks. So IMHO it's clean enough to write: > > if (devfn & VTD_DEVFN_CHECK_MASK) { > ... > } > > Comparing to: > > if (vtd_devfn_check(devfn)) { > ... > } > > And imho actually the name "check"/"mask" is confusing itself > already... So maybe even dropping both vtd_devfn_check() and > VTD_DEVFN_CHECK_MASK (note: there's nothing about any validity checks, > and it's not a mask at all!) and simply: > > if (devfn >= UINT8_MAX / 2) > > That's even clearer to me that it's splitted into two halves.
Sorry I didn't notice UINT8_MAX is 255 rather than 256, then it should be ">"... anyway, these are a bit nitpicking, feel free to use your own preference at last. > > [...] > > > > > diff --git a/include/hw/i386/intel_iommu.h > > > > b/include/hw/i386/intel_iommu.h > > > > index a321cc9..ff13ff27 100644 > > > > --- a/include/hw/i386/intel_iommu.h > > > > +++ b/include/hw/i386/intel_iommu.h > > > > @@ -66,11 +66,12 @@ typedef struct VTDIOTLBEntry VTDIOTLBEntry; > > > > typedef struct VTDBus VTDBus; > > > > typedef union VTD_IR_TableEntry VTD_IR_TableEntry; > > > > typedef union VTD_IR_MSIAddress VTD_IR_MSIAddress; > > > > +typedef struct VTDPASIDDirEntry VTDPASIDDirEntry; > > > > +typedef struct VTDPASIDEntry VTDPASIDEntry; > > > > > > > > /* Context-Entry */ > > > > struct VTDContextEntry { > > > > - uint64_t lo; > > > > - uint64_t hi; > > > > + uint64_t val[4]; > > > > > > You can actually make it an enum, two benefits: > > > > > Thanks for the suggestion! DYM 'union'? > > Yes. :) > > Regards, > > -- > Peter Xu Regards, -- Peter Xu