Re: [Qemu-devel] [RFC v1 1/3] intel_iommu: scalable mode emulation

[Peter Xu]

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