Re: [Qemu-devel] [RFC v1 06/18] intel_iommu: support virtual command emulation and pasid request

[Liu, Yi L]

> From: Peter Xu [mailto:address@hidden]
> Sent: Thursday, July 11, 2019 9:13 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v1 06/18] intel_iommu: support virtual command emulation and
> pasid request
> 
> On Wed, Jul 10, 2019 at 11:51:17AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > > +        s->vcrsp = 1;
> > > > +        vtd_set_quad_raw(s, DMAR_VCRSP_REG,
> > > > +                         ((uint64_t) s->vcrsp));
> > >
> > > Do we really need to emulate the "In Progress" like this?  The vcpu is
> > > blocked here after all, and AFAICT all the rest of vcpus should not
> > > access these registers because obviously these registers cannot be
> > > accessed concurrently...
> >
> > Other vcpus should poll the IP bit before submitting vcmds. As IP bit
> > is set, other vcpus will not access these bits. but if not, they may submit
> > new vcmds, while we only have 1 response register, that is not we
> > support. That's why we need to set IP bit.
> 
> I still don't think another CPU can use this register even if it
> polled with IP==0...  The reason is simply as you described - we only
> have one pair of VCMD/VRSPD registers so IMHO the guest IOMMU driver
> must have a lock (probably a mutex) to guarantee sequential access of
> these registers otherwise race can happen.

Got it. So the case here is: other vcpus will not be able to access the VCMD/
VRSP due the lock in guest iommu driver. So IP bit is only used to block any
further VCMDs from the same vcpu which gained the lock. But we are emulating
VCMD/VRSP in a synchronize manner, so vcpu has no way to submit new VCMDs
before a prior VMCD is completed.

> >
> > >
> > > I think the IP bit is useful when some new vcmd would take plenty of
> > > time so that we can do the long vcmds in async way.  However here it
> > > seems not the case?
> >
> > no, so far, it is synchronize way. As mentioned above, IP bit is to ensure
> > only one vcmd is handled for a time. Other vcpus won't be able to submit
> > vcmds before IP is cleared.
> 
> [...]
> 
> > > > @@ -192,6 +198,7 @@
> > > >  #define VTD_ECAP_SRS                (1ULL << 31)
> > > >  #define VTD_ECAP_PASID              (1ULL << 40)
> > > >  #define VTD_ECAP_SMTS               (1ULL << 43)
> > > > +#define VTD_ECAP_VCS                (1ULL << 44)
> > > >  #define VTD_ECAP_SLTS               (1ULL << 46)
> > > >  #define VTD_ECAP_FLTS               (1ULL << 47)
> > > >
> > > > @@ -314,6 +321,29 @@ typedef enum VTDFaultReason {
> > > >
> > > >  #define VTD_CONTEXT_CACHE_GEN_MAX       0xffffffffUL
> > > >
> > > > +/* VCCAP_REG */
> > > > +#define VTD_VCCAP_PAS               (1UL << 0)
> > > > +#define VTD_MIN_HPASID              200
> > >
> > > Comment this value a bit?
> >
> > The basic idea is to let hypervisor to set a range for available PASIDs for
> > VMs. One of the reasons is PASID #0 is reserved by RID_PASID usage.
> > We have no idea how many reserved PASIDs in future, so here just a
> > evaluated value. Honestly, set it as "1" is enough at current stage.
> 
> That'll be a very nice initial comment for that (I mean, put it into
> the patch, of course :).

Got it. will add it in next version.

Thanks,
Yi Liu