Re: [PATCH V2 1/4] intel-iommu: don't warn guest errors when getting rid2pasid entry

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Mar 29, 2022 at 12:52:08PM +0800, Jason Wang wrote:
> 
> 在 2022/3/28 下午4:53, Yi Liu 写道:
> > 
> > 
> > On 2022/3/28 10:27, Jason Wang wrote:
> > > On Thu, Mar 24, 2022 at 4:21 PM Tian, Kevin <kevin.tian@intel.com>
> > > wrote:
> > > > 
> > > > > From: Jason Wang
> > > > > Sent: Monday, March 21, 2022 1:54 PM
> > > > > 
> > > > > We use to warn on wrong rid2pasid entry. But this error could be
> > > > > triggered by the guest and could happens during initialization. So
> > > > > let's don't warn in this case.
> > > > > 
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > >   hw/i386/intel_iommu.c | 6 ++++--
> > > > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > > index 874d01c162..90964b201c 100644
> > > > > --- a/hw/i386/intel_iommu.c
> > > > > +++ b/hw/i386/intel_iommu.c
> > > > > @@ -1554,8 +1554,10 @@ static bool vtd_dev_pt_enabled(IntelIOMMUState
> > > > > *s, VTDContextEntry *ce)
> > > > >       if (s->root_scalable) {
> > > > >           ret = vtd_ce_get_rid2pasid_entry(s, ce, &pe);
> > > > >           if (ret) {
> > > > > -            error_report_once("%s:
> > > > > vtd_ce_get_rid2pasid_entry error: %"PRId32,
> > > > > -                              __func__, ret);
> > > > > +            /*
> > > > > +             * This error is guest triggerable. We should assumt PT
> > > > > +             * not enabled for safety.
> > > > > +             */
> > > > 
> > > > suppose a VT-d fault should be queued in this case besides
> > > > returning false:
> > > > 
> > > > SPD.1: A hardware attempt to access the scalable-mode PASID-directory
> > > > entry referenced through the PASIDDIRPTR field in scalable-mode
> > > > context-entry resulted in an error
> > > > 
> > > > SPT.1: A hardware attempt to access a scalable-mode PASID-table entry
> > > > referenced through the SMPTBLPTR field in a scalable-mode
> > > > PASID-directory
> > > > entry resulted in an error.
> > > 
> > > Probably, but this issue is not introduced in this patch. We can fix
> > > it on top if necessary.
> > 
> > agreed.
> > 
> > > > 
> > > > Currently the implementation of vtd_ce_get_rid2pasid_entry() is also
> > > > problematic. According to VT-d spec, RID2PASID field is effective only
> > > > when ecap.rps is true otherwise PASID#0 is used for RID2PASID. I didn't
> > > > see ecap.rps is set, neither is it checked in that function. It
> > > > works possibly
> > > > just because Linux currently programs 0 to RID2PASID...
> > > 
> > > This seems to be another issue since the introduction of scalable mode.
> > 
> > yes. this is not introduced in this series. The current scalable mode
> > vIOMMU support was following 3.0 spec, while RPS is added in 3.1. Needs
> > to be fixed.
> 
> 
> Interesting, so this is more complicated when dealing with migration
> compatibility. So what I suggest is probably something like:
> 
> -device intel-iommu,version=$version
> 
> Then we can maintain migration compatibility correctly. For 3.0 we can go
> without RPS and 3.1 and above we need to implement RPS.
> 
> Since most of the advanced features has not been implemented, we may
> probably start just from 3.4 (assuming it's the latest version). And all of
> the following effort should be done for 3.4 in order to productize it.
> 
> Thanks

I would advise calling it x-version. And declare it unstable for now.  I
think that we don't at this point want to support users tweaking version
to arbitrary values.


> 
> > 
> > > Thanks
> > > 
> > > > 
> > > > >               return false;
> > > > >           }
> > > > >           return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT);
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > 
> > > 
> >