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

2022-01-13 Thread Jason Wang 



在 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 
---
  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

2022-01-13 Thread Jason Wang 



在 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 
---
  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

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

2022-01-12 Thread 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 
> > ---
> >  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

2022-01-12 Thread 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 
> ---
>  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

2022-01-12 Thread 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  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 
> > > ---
> > >  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 

Thanks.

-- 
Peter Xu

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

2022-01-12 Thread Jason Wang 
On Thu, Jan 13, 2022 at 11:35 AM 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 
> > ---
> >  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

2022-01-12 Thread 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 
> ---
>  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

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

2022-01-04 Thread Jason Wang 
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 
---
 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