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.
[...]
> > > 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
