| Message ID | 20220105041945.13459-3-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry | expand |
On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: > 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 4c6c016388..f2c7a23712 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1524,8 +1524,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. > + */ > return false; > } > return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); > -- > 2.25.1 > No strong opinion, but the thing is mostly all error_report_once() in this file is guest triggerable. If we remove this one then it's debatable on whether we want to remove all. IMHO we used the _once() variant just for this: it won't go into any form of DoS, meanwhile we'll still get some information (as hypervisor) that the guest OS may not be trustworthy. So from that pov it's still useful? Or is this error very special in some way? Thanks,
On Thu, Jan 13, 2022 at 11:35 AM Peter Xu <peterx@redhat.com> wrote: > > On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: > > 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 4c6c016388..f2c7a23712 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1524,8 +1524,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. > > + */ > > return false; > > } > > return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); > > -- > > 2.25.1 > > > > No strong opinion, but the thing is mostly all error_report_once() in this file > is guest triggerable. If we remove this one then it's debatable on whether we > want to remove all. > > IMHO we used the _once() variant just for this: it won't go into any form of > DoS, meanwhile we'll still get some information (as hypervisor) that the guest > OS may not be trustworthy. > > So from that pov it's still useful? Or is this error very special in some way? I want to be consistent with vtd_as_pt_enabled() where we don't even have error_report_once(). Thanks > > Thanks, > > -- > Peter Xu >
On Thu, Jan 13, 2022 at 02:16:35PM +0800, Jason Wang wrote: > On Thu, Jan 13, 2022 at 11:35 AM Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: > > > 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 4c6c016388..f2c7a23712 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -1524,8 +1524,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. > > > + */ > > > return false; > > > } > > > return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); > > > -- > > > 2.25.1 > > > > > > > No strong opinion, but the thing is mostly all error_report_once() in this file > > is guest triggerable. If we remove this one then it's debatable on whether we > > want to remove all. > > > > IMHO we used the _once() variant just for this: it won't go into any form of > > DoS, meanwhile we'll still get some information (as hypervisor) that the guest > > OS may not be trustworthy. > > > > So from that pov it's still useful? Or is this error very special in some way? > > I want to be consistent with vtd_as_pt_enabled() where we don't even > have error_report_once(). According to the comments (which, I think, left over by myself..) in vtd_as_pt_enabled() - maybe indeed it could be triggered during guest boot. Then I assume this can indeed a special case. Acked-by: Peter Xu <peterx@redhat.com> Thanks.
On Thu, Jan 13, 2022 at 11:35:19AM +0800, Peter Xu wrote: > On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: > > 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 4c6c016388..f2c7a23712 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1524,8 +1524,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. > > + */ > > return false; > > } > > return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); > > -- > > 2.25.1 > > > > No strong opinion, but the thing is mostly all error_report_once() in this file > is guest triggerable. If we remove this one then it's debatable on whether we > want to remove all. > > IMHO we used the _once() variant just for this: it won't go into any form of > DoS, meanwhile we'll still get some information (as hypervisor) that the guest > OS may not be trustworthy. > > So from that pov it's still useful? Or is this error very special in some way? > > Thanks, Well we have LOG_GUEST_ERROR for guest errors now. > -- > Peter Xu
On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: > 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 4c6c016388..f2c7a23712 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1524,8 +1524,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 typo And drop "We should" pls, just use direct voice: "Assume PT not enabled". > + * not enabled for safety. > + */ > return false; > } > return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); > -- > 2.25.1
在 2022/1/13 下午3:06, Michael S. Tsirkin 写道: > On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: >> 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 4c6c016388..f2c7a23712 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -1524,8 +1524,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 > typo > > And drop "We should" pls, just use direct voice: > "Assume PT not enabled". Fixed. Thanks > > >> + * not enabled for safety. >> + */ >> return false; >> } >> return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); >> -- >> 2.25.1
在 2022/1/13 下午3:05, Michael S. Tsirkin 写道: > On Thu, Jan 13, 2022 at 11:35:19AM +0800, Peter Xu wrote: >> On Wed, Jan 05, 2022 at 12:19:43PM +0800, Jason Wang wrote: >>> 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 4c6c016388..f2c7a23712 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -1524,8 +1524,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. >>> + */ >>> return false; >>> } >>> return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); >>> -- >>> 2.25.1 >>> >> No strong opinion, but the thing is mostly all error_report_once() in this file >> is guest triggerable. If we remove this one then it's debatable on whether we >> want to remove all. >> >> IMHO we used the _once() variant just for this: it won't go into any form of >> DoS, meanwhile we'll still get some information (as hypervisor) that the guest >> OS may not be trustworthy. >> >> So from that pov it's still useful? Or is this error very special in some way? >> >> Thanks, > > Well we have LOG_GUEST_ERROR for guest errors now. Ok, but this is not necessarily a guest error. (Inferring from the comment in vtd_as_pt_enabled()). Thanks > >> -- >> Peter Xu
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 4c6c016388..f2c7a23712 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1524,8 +1524,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. + */ return false; } return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT);
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(-)