RE: [RFC v2 10/22] intel_iommu: add virtual command capability support

From: "Liu, Yi L" <yi.l.liu@intel.com>
To: Peter Xu <peterx@redhat.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	"jacob.jun.pan@linux.intel.com" <jacob.jun.pan@linux.intel.com>,
	Yi Sun <yi.y.sun@linux.intel.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"mst@redhat.com" <mst@redhat.com>,
	"Tian, Jun J" <jun.j.tian@intel.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"Sun, Yi Y" <yi.y.sun@intel.com>,
	"david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>
Subject: RE: [RFC v2 10/22] intel_iommu: add virtual command capability support
Date: Tue, 12 Nov 2019 06:27:22 +0000	[thread overview]
Message-ID: <A2975661238FB949B60364EF0F2C25743A0F64FB@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20191106140055.GA29717@xz-x1>

> From: Peter Xu <peterx@redhat.com>
> Sent: Wednesday, November 6, 2019 10:01 PM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [RFC v2 10/22] intel_iommu: add virtual command capability support
> 
> On Wed, Nov 06, 2019 at 12:40:41PM +0000, Liu, Yi L wrote:
> > >
> > > Do you know what should happen on bare-metal from spec-wise that when
> > > the guest e.g. writes 2 bytes to these mmio regions?
> >
> > I've no idea to your question. It is not a bare-metal capability. Personally, I
> > prefer to have a toggle bit to mark the full written of a cmd to VMCD_REG.
> > Reason is that we have no control on guest software. It may write new cmd
> > to VCMD_REG in a bad manner. e.g. write high 32 bits first and then write the
> > low 32 bits. Then it will have two traps. Apparently, for the first trap, it fills
> > in the VCMD_REG and no need to handle it since it is not a full written. I'm
> > checking it and evaluating it. How do you think on it?
> 
> Oh I just noticed that vtd_mem_ops.min_access_size==4 now so writting
> 2B should never happen at least.  Then we'll bail out at
> memory_region_access_valid().  Seems fine.

got it.

> 
> >
> > >
> > > > +        if (!vtd_handle_vcmd_write(s, val)) {
> > > > +            vtd_set_long(s, addr, val);
> > > > +        }
> > > > +        break;
> > > > +
> > > >      default:
> > > >          if (size == 4) {
> > > >              vtd_set_long(s, addr, val);
> > > > @@ -3617,7 +3769,8 @@ static void vtd_init(IntelIOMMUState *s)
> > > >              s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
> > > >          } else if (!strcmp(s->scalable_mode, "modern")) {
> > > >              s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_PASID
> > > > -                       | VTD_ECAP_FLTS | VTD_ECAP_PSS;
> > > > +                       | VTD_ECAP_FLTS | VTD_ECAP_PSS | VTD_ECAP_VCS;
> > > > +            s->vccap |= VTD_VCCAP_PAS;
> > > >          }
> > > >      }
> > > >
> > >
> > > [...]
> > >
> > > > +#define VTD_VCMD_CMD_MASK           0xffUL
> > > > +#define VTD_VCMD_PASID_VALUE(val)   (((val) >> 8) & 0xfffff)
> > > > +
> > > > +#define VTD_VCRSP_RSLT(val)         ((val) << 8)
> > > > +#define VTD_VCRSP_SC(val)           (((val) & 0x3) << 1)
> > > > +
> > > > +#define VTD_VCMD_UNDEFINED_CMD         1ULL
> > > > +#define VTD_VCMD_NO_AVAILABLE_PASID    2ULL
> > >
> > > According to 10.4.44 - should this be 1?
> >
> > It's 2 now per VT-d spec 3.1 (2019 June). I should have mentioned it in the cover
> > letter...
> 
> Well you're right... I hope there won't be other "major" things get
> changed otherwise it'll be really a pain of working on all of these
> before things settle...

As far as I know, only this part has significant change. Other parts are consistent.
I'll mention spec version next time in the cover letter.

Thanks,
Yi Liu