1. Patchew
  2. QEMU
  3. [PATCH 0/3] PASID support for Intel IOMMU
  4. View patch

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

Jason Wang posted 3 patches 4 years, 1 month ago
There is a newer version of this series
[PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Jason Wang 4 years, 1 month ago 
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
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Peter Xu 4 years ago 
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,

-- 
Peter Xu
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Jason Wang 4 years ago 
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
>
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Michael S. Tsirkin 4 years ago 
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
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Jason Wang 4 years ago 
在 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
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Michael S. Tsirkin 4 years ago 
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
Re: [PATCH 1/3] intel-iommu: don't warn guest errors when getting rid2pasid entry
Posted by Jason Wang 4 years ago 
在 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.