RE: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
> From: Jan Beulich
> Sent: Wednesday, April 21, 2021 5:23 PM
>
> On 20.04.2021 18:17, Roger Pau Monné wrote:
> > On Tue, Apr 20, 2021 at 05:38:51PM +0200, Jan Beulich wrote:
> >> On 20.04.2021 17:08, Roger Pau Monné wrote:
> >>> On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
> return 0;
> }
>
> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t
> did,
> + uint16_t source_id, uint8_t
> function_mask,
> + uint64_t type, bool
> flush_non_present_entry)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush
> CONTEXT.\n");
> +return -EIO;
> +}
> +
> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t
> did,
> +uint64_t addr, unsigned int size_order,
> +uint64_t type, bool
> flush_non_present_entry,
> +bool flush_dev_iotlb)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
> +return -EIO;
> +}
> >>>
> >>> I think I would add an ASSERT_UNREACHABLE() to both noop handlers
> >>> above, as I would expect trying to use them without the proper mode
> >>> being configured would point to an error elsewhere?
> >>
> >> If such an assertion triggered e.g. during S3 suspend/resume, it may
> >> lead to the box simply not doing anything useful, without there being
> >> any way to know what went wrong. If instead the system at least
> >> managed to resume, the log message could be observed.
> >
> > Oh, OK then. I'm simply worried that people might ignore such one line
> > messages, maybe add a WARN?
>
> Hmm, yes, perhaps - would allow seeing right away where the call
> came from. Chao, I'd again be fine to flip the dprintk()-s to
> WARN()-s while committing. But of course only provided you and
> Kevin (as the maintainer) agree.
>
Looks good.
Reviewed-by: Kevin Tian
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On Wed, Apr 21, 2021 at 11:23:13AM +0200, Jan Beulich wrote:
>On 20.04.2021 18:17, Roger Pau Monné wrote:
>> On Tue, Apr 20, 2021 at 05:38:51PM +0200, Jan Beulich wrote:
>>> On 20.04.2021 17:08, Roger Pau Monné wrote:
On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
> return 0;
> }
>
> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
> + uint16_t source_id, uint8_t
> function_mask,
> + uint64_t type, bool
> flush_non_present_entry)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
> +return -EIO;
> +}
> +
> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
> +uint64_t addr, unsigned int size_order,
> +uint64_t type, bool
> flush_non_present_entry,
> +bool flush_dev_iotlb)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
> +return -EIO;
> +}
I think I would add an ASSERT_UNREACHABLE() to both noop handlers
above, as I would expect trying to use them without the proper mode
being configured would point to an error elsewhere?
>>>
>>> If such an assertion triggered e.g. during S3 suspend/resume, it may
>>> lead to the box simply not doing anything useful, without there being
>>> any way to know what went wrong. If instead the system at least
>>> managed to resume, the log message could be observed.
>>
>> Oh, OK then. I'm simply worried that people might ignore such one line
>> messages, maybe add a WARN?
>
>Hmm, yes, perhaps - would allow seeing right away where the call
>came from. Chao, I'd again be fine to flip the dprintk()-s to
>WARN()-s while committing. But of course only provided you and
>Kevin (as the maintainer) agree.
Sure, please go ahead.
Thanks
Chao
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 20.04.2021 18:17, Roger Pau Monné wrote:
> On Tue, Apr 20, 2021 at 05:38:51PM +0200, Jan Beulich wrote:
>> On 20.04.2021 17:08, Roger Pau Monné wrote:
>>> On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
return 0;
}
+static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
+ uint16_t source_id, uint8_t
function_mask,
+ uint64_t type, bool
flush_non_present_entry)
+{
+dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
+return -EIO;
+}
+
+static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
+uint64_t addr, unsigned int size_order,
+uint64_t type, bool
flush_non_present_entry,
+bool flush_dev_iotlb)
+{
+dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
+return -EIO;
+}
>>>
>>> I think I would add an ASSERT_UNREACHABLE() to both noop handlers
>>> above, as I would expect trying to use them without the proper mode
>>> being configured would point to an error elsewhere?
>>
>> If such an assertion triggered e.g. during S3 suspend/resume, it may
>> lead to the box simply not doing anything useful, without there being
>> any way to know what went wrong. If instead the system at least
>> managed to resume, the log message could be observed.
>
> Oh, OK then. I'm simply worried that people might ignore such one line
> messages, maybe add a WARN?
Hmm, yes, perhaps - would allow seeing right away where the call
came from. Chao, I'd again be fine to flip the dprintk()-s to
WARN()-s while committing. But of course only provided you and
Kevin (as the maintainer) agree.
> Would it make sense to mark as tainted which could help identify the
> issue on production builds? Maybe that's too much.
Yeah, for something we expect shouldn't ever happen ...
Jan
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On Tue, Apr 20, 2021 at 05:38:51PM +0200, Jan Beulich wrote:
> On 20.04.2021 17:08, Roger Pau Monné wrote:
> > On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
> >> --- a/xen/drivers/passthrough/vtd/qinval.c
> >> +++ b/xen/drivers/passthrough/vtd/qinval.c
> >> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
> >> return 0;
> >> }
> >>
> >> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
> >> + uint16_t source_id, uint8_t
> >> function_mask,
> >> + uint64_t type, bool
> >> flush_non_present_entry)
> >> +{
> >> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
> >> +return -EIO;
> >> +}
> >> +
> >> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
> >> +uint64_t addr, unsigned int size_order,
> >> +uint64_t type, bool
> >> flush_non_present_entry,
> >> +bool flush_dev_iotlb)
> >> +{
> >> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
> >> +return -EIO;
> >> +}
> >
> > I think I would add an ASSERT_UNREACHABLE() to both noop handlers
> > above, as I would expect trying to use them without the proper mode
> > being configured would point to an error elsewhere?
>
> If such an assertion triggered e.g. during S3 suspend/resume, it may
> lead to the box simply not doing anything useful, without there being
> any way to know what went wrong. If instead the system at least
> managed to resume, the log message could be observed.
Oh, OK then. I'm simply worried that people might ignore such one line
messages, maybe add a WARN?
Would it make sense to mark as tainted which could help identify the
issue on production builds? Maybe that's too much.
Thanks, Roger.
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 20.04.2021 17:08, Roger Pau Monné wrote:
> On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
>> --- a/xen/drivers/passthrough/vtd/qinval.c
>> +++ b/xen/drivers/passthrough/vtd/qinval.c
>> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
>> return 0;
>> }
>>
>> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
>> + uint16_t source_id, uint8_t function_mask,
>> + uint64_t type, bool
>> flush_non_present_entry)
>> +{
>> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
>> +return -EIO;
>> +}
>> +
>> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
>> +uint64_t addr, unsigned int size_order,
>> +uint64_t type, bool flush_non_present_entry,
>> +bool flush_dev_iotlb)
>> +{
>> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
>> +return -EIO;
>> +}
>
> I think I would add an ASSERT_UNREACHABLE() to both noop handlers
> above, as I would expect trying to use them without the proper mode
> being configured would point to an error elsewhere?
If such an assertion triggered e.g. during S3 suspend/resume, it may
lead to the box simply not doing anything useful, without there being
any way to know what went wrong. If instead the system at least
managed to resume, the log message could be observed.
Jan
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
> return 0;
> }
>
> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
> + uint16_t source_id, uint8_t function_mask,
> + uint64_t type, bool
> flush_non_present_entry)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
> +return -EIO;
> +}
> +
> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
> +uint64_t addr, unsigned int size_order,
> +uint64_t type, bool flush_non_present_entry,
> +bool flush_dev_iotlb)
> +{
> +dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
> +return -EIO;
> +}
I think I would add an ASSERT_UNREACHABLE() to both noop handlers
above, as I would expect trying to use them without the proper mode
being configured would point to an error elsewhere?
Thanks, Roger.
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 20/04/2021 13:50, Jan Beulich wrote: Alternatively, you could be a bit more verbose in your request so the other understand the reasoning behind it. Well, yes, perhaps. But then there's the desire to not repeat oneself all the time. Most likely, the time you try to save not expanding your thought are going to be lost when the contributor will come back asking why you are requesting it. ;) Cheers, -- Julien Grall
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 20.04.2021 14:39, Julien Grall wrote:
> On 20/04/2021 13:25, Jan Beulich wrote:
>> On 20.04.2021 14:14, Julien Grall wrote:
>>> It is not really my area of expertise but I wanted to jump on one
>>> comment below...
>>>
>>> On 20/04/2021 12:38, Jan Beulich wrote:
On 01.04.2020 22:06, Chao Gao wrote:
> ---
> Changes in v2:
>- verify system suspension and resumption with this patch applied
>- don't fall back to register-based interface if enabling qinval
> failed.
> see the change in init_vtd_hw().
>- remove unnecessary "queued_inval_supported" variable
>- constify the "struct vtd_iommu *" of
> has_register_based_invalidation()
>- coding-style changes
... while this suggests this is v2 of a recently sent patch, the
submission is dated a little over a year ago. This is confusing.
It is additionally confusing that there were two copies of it in
my inbox, despite mails coming from a list normally getting
de-duplicated somewhere at our end (I believe).
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>
>iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
>iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
> +iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
> +
> +if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
> +{
> +printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
> Invalidation.\n",
> + iommu->index);
Here (and at least once more yet further down): We don't normally end
log messages with a full stop. Easily addressable while committing, of
course.
>>>
>>> I can find a large number of cases where log messages are ended with a
>>> full stop... Actually it looks more natural to me than your suggestion.
>>
>> Interesting. From purely a language perspective it indeed looks more
>> natural, I agree. But when considering (serial) console bandwidth, we
>> ought to try to eliminate _any_ output that's there without conveying
>> information or making the conveyed information understandable. In fact
>> I recall a number of cases (without having commit hashes to hand)
>> where we deliberately dropped full stops. (The messages here aren't at
>> risk of causing bandwidth issues, but as with any such generic item I
>> think the goal ought to be consistency, and hence either full stops
>> everywhere, or nowhere. If bandwidth was an issue here, I might also
>> have suggested to shorten "Queued Invalidation" to just "QI".)
> I wasn't aware of such requirement in Xen... Although, I can see how
> this can be a concern. If you really want to enforce it, then it should
> be written in the CODING_STYLE.
Agreed, but since I've had no success with prior adjustments to that
file (not even worth a reply to tell me why a change might be a bad
one, in at least some of the cases), I'm afraid I've given up making
attempts to get adjustments into there.
> Alternatively, you could be a bit more
> verbose in your request so the other understand the reasoning behind it.
Well, yes, perhaps. But then there's the desire to not repeat oneself
all the time.
Jan
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
Hi,
On 20/04/2021 13:25, Jan Beulich wrote:
On 20.04.2021 14:14, Julien Grall wrote:
Hi,
It is not really my area of expertise but I wanted to jump on one
comment below...
On 20/04/2021 12:38, Jan Beulich wrote:
On 01.04.2020 22:06, Chao Gao wrote:
---
Changes in v2:
- verify system suspension and resumption with this patch applied
- don't fall back to register-based interface if enabling qinval failed.
see the change in init_vtd_hw().
- remove unnecessary "queued_inval_supported" variable
- constify the "struct vtd_iommu *" of has_register_based_invalidation()
- coding-style changes
... while this suggests this is v2 of a recently sent patch, the
submission is dated a little over a year ago. This is confusing.
It is additionally confusing that there were two copies of it in
my inbox, despite mails coming from a list normally getting
de-duplicated somewhere at our end (I believe).
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
+iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
+
+if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
+{
+printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
Invalidation.\n",
+ iommu->index);
Here (and at least once more yet further down): We don't normally end
log messages with a full stop. Easily addressable while committing, of
course.
I can find a large number of cases where log messages are ended with a
full stop... Actually it looks more natural to me than your suggestion.
Interesting. From purely a language perspective it indeed looks more
natural, I agree. But when considering (serial) console bandwidth, we
ought to try to eliminate _any_ output that's there without conveying
information or making the conveyed information understandable. In fact
I recall a number of cases (without having commit hashes to hand)
where we deliberately dropped full stops. (The messages here aren't at
risk of causing bandwidth issues, but as with any such generic item I
think the goal ought to be consistency, and hence either full stops
everywhere, or nowhere. If bandwidth was an issue here, I might also
have suggested to shorten "Queued Invalidation" to just "QI".)
I wasn't aware of such requirement in Xen... Although, I can see how
this can be a concern. If you really want to enforce it, then it should
be written in the CODING_STYLE. Alternatively, you could be a bit more
verbose in your request so the other understand the reasoning behind it.
Also, when you say "large number" - did you compare to the number of
cases where there are no full stops? (I sincerely hope we don't have
that many full stops left.)
42sh> ack "printk\(\"" | ack "\..n\"" | wc -l
130
42sh> ack "printk\(\"" | wc -l
1708
So ~7.5% of the printks.
--
Julien Grall
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On Tue, Apr 20, 2021 at 01:38:26PM +0200, Jan Beulich wrote:
>On 01.04.2020 22:06, Chao Gao wrote:
>> According to Intel VT-d SPEC rev3.3 Section 6.5, Register-based Invalidation
>> isn't supported by Intel VT-d version 6 and beyond.
>>
>> This hardware change impacts following two scenarios: admin can disable
>> queued invalidation via 'qinval' cmdline and use register-based interface;
>> VT-d switches to register-based invalidation when queued invalidation needs
>> to be disabled, for example, during disabling x2apic or during system
>> suspension or after enabling queued invalidation fails.
>>
>> To deal with this hardware change, if register-based invalidation isn't
>> supported, queued invalidation cannot be disabled through Xen cmdline; and
>> if queued invalidation has to be disabled temporarily in some scenarios,
>> VT-d won't switch to register-based interface but use some dummy functions
>> to catch errors in case there is any invalidation request issued when queued
>> invalidation is disabled.
>>
>> Signed-off-by: Chao Gao
>
>In principle (with a minor nit further down)
>Reviewed-by: Jan Beulich
>
>However, ...
>
>> ---
>> Changes in v2:
>> - verify system suspension and resumption with this patch applied
>> - don't fall back to register-based interface if enabling qinval failed.
>>see the change in init_vtd_hw().
>> - remove unnecessary "queued_inval_supported" variable
>> - constify the "struct vtd_iommu *" of has_register_based_invalidation()
>> - coding-style changes
>
>... while this suggests this is v2 of a recently sent patch, the
>submission is dated a little over a year ago. This is confusing.
>It is additionally confusing that there were two copies of it in
>my inbox, despite mails coming from a list normally getting
>de-duplicated somewhere at our end (I believe).
You are right. I messed the system time of my server somehow. Sorry for this.
If it is possible, please help to update the date of this patch also.
>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>>
>> iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
>> iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
>> +iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
>> +
>> +if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
>> +{
>> +printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
>> Invalidation.\n",
>> + iommu->index);
>
>Here (and at least once more yet further down): We don't normally end
>log messages with a full stop. Easily addressable while committing, of
>course.
Okay. Please go ahead.
Thanks
Chao
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 20.04.2021 14:14, Julien Grall wrote:
> Hi,
>
> It is not really my area of expertise but I wanted to jump on one
> comment below...
>
> On 20/04/2021 12:38, Jan Beulich wrote:
>> On 01.04.2020 22:06, Chao Gao wrote:
>>> ---
>>> Changes in v2:
>>> - verify system suspension and resumption with this patch applied
>>> - don't fall back to register-based interface if enabling qinval failed.
>>> see the change in init_vtd_hw().
>>> - remove unnecessary "queued_inval_supported" variable
>>> - constify the "struct vtd_iommu *" of has_register_based_invalidation()
>>> - coding-style changes
>>
>> ... while this suggests this is v2 of a recently sent patch, the
>> submission is dated a little over a year ago. This is confusing.
>> It is additionally confusing that there were two copies of it in
>> my inbox, despite mails coming from a list normally getting
>> de-duplicated somewhere at our end (I believe).
>>
>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>> @@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>>>
>>> iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
>>> iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
>>> +iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
>>> +
>>> +if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
>>> +{
>>> +printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
>>> Invalidation.\n",
>>> + iommu->index);
>>
>> Here (and at least once more yet further down): We don't normally end
>> log messages with a full stop. Easily addressable while committing, of
>> course.
>
> I can find a large number of cases where log messages are ended with a
> full stop... Actually it looks more natural to me than your suggestion.
Interesting. From purely a language perspective it indeed looks more
natural, I agree. But when considering (serial) console bandwidth, we
ought to try to eliminate _any_ output that's there without conveying
information or making the conveyed information understandable. In fact
I recall a number of cases (without having commit hashes to hand)
where we deliberately dropped full stops. (The messages here aren't at
risk of causing bandwidth issues, but as with any such generic item I
think the goal ought to be consistency, and hence either full stops
everywhere, or nowhere. If bandwidth was an issue here, I might also
have suggested to shorten "Queued Invalidation" to just "QI".)
Also, when you say "large number" - did you compare to the number of
cases where there are no full stops? (I sincerely hope we don't have
that many full stops left.)
Jan
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
Hi,
It is not really my area of expertise but I wanted to jump on one
comment below...
On 20/04/2021 12:38, Jan Beulich wrote:
On 01.04.2020 22:06, Chao Gao wrote:
---
Changes in v2:
- verify system suspension and resumption with this patch applied
- don't fall back to register-based interface if enabling qinval failed.
see the change in init_vtd_hw().
- remove unnecessary "queued_inval_supported" variable
- constify the "struct vtd_iommu *" of has_register_based_invalidation()
- coding-style changes
... while this suggests this is v2 of a recently sent patch, the
submission is dated a little over a year ago. This is confusing.
It is additionally confusing that there were two copies of it in
my inbox, despite mails coming from a list normally getting
de-duplicated somewhere at our end (I believe).
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
+iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
+
+if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
+{
+printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
Invalidation.\n",
+ iommu->index);
Here (and at least once more yet further down): We don't normally end
log messages with a full stop. Easily addressable while committing, of
course.
I can find a large number of cases where log messages are ended with a
full stop... Actually it looks more natural to me than your suggestion.
Cheers,
--
Julien Grall
Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
On 01.04.2020 22:06, Chao Gao wrote:
> According to Intel VT-d SPEC rev3.3 Section 6.5, Register-based Invalidation
> isn't supported by Intel VT-d version 6 and beyond.
>
> This hardware change impacts following two scenarios: admin can disable
> queued invalidation via 'qinval' cmdline and use register-based interface;
> VT-d switches to register-based invalidation when queued invalidation needs
> to be disabled, for example, during disabling x2apic or during system
> suspension or after enabling queued invalidation fails.
>
> To deal with this hardware change, if register-based invalidation isn't
> supported, queued invalidation cannot be disabled through Xen cmdline; and
> if queued invalidation has to be disabled temporarily in some scenarios,
> VT-d won't switch to register-based interface but use some dummy functions
> to catch errors in case there is any invalidation request issued when queued
> invalidation is disabled.
>
> Signed-off-by: Chao Gao
In principle (with a minor nit further down)
Reviewed-by: Jan Beulich
However, ...
> ---
> Changes in v2:
> - verify system suspension and resumption with this patch applied
> - don't fall back to register-based interface if enabling qinval failed.
>see the change in init_vtd_hw().
> - remove unnecessary "queued_inval_supported" variable
> - constify the "struct vtd_iommu *" of has_register_based_invalidation()
> - coding-style changes
... while this suggests this is v2 of a recently sent patch, the
submission is dated a little over a year ago. This is confusing.
It is additionally confusing that there were two copies of it in
my inbox, despite mails coming from a list normally getting
de-duplicated somewhere at our end (I believe).
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1193,6 +1193,14 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
>
> iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG);
> iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG);
> +iommu->version = dmar_readl(iommu->reg, DMAR_VER_REG);
> +
> +if ( !iommu_qinval && !has_register_based_invalidation(iommu) )
> +{
> +printk(XENLOG_WARNING VTDPREFIX "IOMMU %d: cannot disable Queued
> Invalidation.\n",
> + iommu->index);
Here (and at least once more yet further down): We don't normally end
log messages with a full stop. Easily addressable while committing, of
course.
Jan
