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
