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.
>
> >
> > 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 :).
Regards,
--
Peter Xu
