[PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[Chao Gao]

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 <chao.gao@intel.com>
---
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
---
 docs/misc/xen-command-line.pandoc    |  4 +++-
 xen/drivers/passthrough/vtd/iommu.c  | 27 +++++++++++++++++++++--
 xen/drivers/passthrough/vtd/iommu.h  |  7 ++++++
 xen/drivers/passthrough/vtd/qinval.c | 33 ++++++++++++++++++++++++++--
 4 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index deef6d0b4c..4ff4a87844 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1442,7 +1442,9 @@ The following options are specific to Intel VT-d hardware:
 *   The `qinval` boolean controls the Queued Invalidation sub-feature, and is
     active by default on compatible hardware.  Queued Invalidation is a
     feature in second-generation IOMMUs and is a functional prerequisite for
-    Interrupt Remapping.
+    Interrupt Remapping. Note that Xen disregards this setting for Intel VT-d
+    version 6 and greater as Registered-Based Invalidation isn't supported
+    by them.
 
 *   The `igfx` boolean is active by default, and controls whether the IOMMU in
     front of an Intel Graphics Device is enabled or not.
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 6428c8fe3e..94d1372903 100644
--- 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);
+        iommu_qinval = true;
+    }
 
     if ( iommu_verbose )
     {
@@ -2141,6 +2149,10 @@ static int __must_check init_vtd_hw(bool resume)
          */
         if ( enable_qinval(iommu) != 0 )
         {
+            /* Ensure register-based invalidation is available */
+            if ( !has_register_based_invalidation(iommu) )
+                return -EIO;
+
             iommu->flush.context = vtd_flush_context_reg;
             iommu->flush.iotlb   = vtd_flush_iotlb_reg;
         }
@@ -2231,6 +2243,7 @@ static int __init vtd_setup(void)
     struct acpi_drhd_unit *drhd;
     struct vtd_iommu *iommu;
     int ret;
+    bool reg_inval_supported = true;
 
     if ( list_empty(&acpi_drhd_units) )
     {
@@ -2252,8 +2265,8 @@ static int __init vtd_setup(void)
     }
 
     /* We enable the following features only if they are supported by all VT-d
-     * engines: Snoop Control, DMA passthrough, Queued Invalidation, Interrupt
-     * Remapping, and Posted Interrupt
+     * engines: Snoop Control, DMA passthrough, Register-based Invalidation,
+     * Queued Invalidation, Interrupt Remapping, and Posted Interrupt.
      */
     for_each_drhd_unit ( drhd )
     {
@@ -2275,6 +2288,9 @@ static int __init vtd_setup(void)
         if ( iommu_qinval && !ecap_queued_inval(iommu->ecap) )
             iommu_qinval = 0;
 
+        if ( !has_register_based_invalidation(iommu) )
+            reg_inval_supported = false;
+
         if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
             iommu_intremap = iommu_intremap_off;
 
@@ -2301,6 +2317,13 @@ static int __init vtd_setup(void)
 
     softirq_tasklet_init(&vtd_fault_tasklet, do_iommu_page_fault, NULL);
 
+    if ( !iommu_qinval && !reg_inval_supported )
+    {
+        dprintk(XENLOG_ERR VTDPREFIX, "No available invalidation interface.\n");
+        ret = -ENODEV;
+        goto error;
+    }
+
     if ( !iommu_qinval && iommu_intremap )
     {
         iommu_intremap = iommu_intremap_off;
diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
index 216791b3d6..57737b1a4a 100644
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -540,6 +540,7 @@ struct vtd_iommu {
     struct list_head ats_devices;
     unsigned long *domid_bitmap;  /* domain id bitmap */
     u16 *domid_map;               /* domain id mapping array */
+    uint32_t version;
 };
 
 #define INTEL_IOMMU_DEBUG(fmt, args...) \
@@ -549,4 +550,10 @@ struct vtd_iommu {
             dprintk(XENLOG_WARNING VTDPREFIX, fmt, ## args);    \
     } while(0)
 
+/* Register-based invalidation isn't supported by VT-d version 6 and beyond. */
+static inline bool has_register_based_invalidation(const struct vtd_iommu *vtd)
+{
+    return VER_MAJOR(vtd->version) < 6;
+}
+
 #endif
diff --git a/xen/drivers/passthrough/vtd/qinval.c b/xen/drivers/passthrough/vtd/qinval.c
index 764ef9f0fc..166c754b21 100644
--- 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;
+}
+
 void disable_qinval(struct vtd_iommu *iommu)
 {
     u32 sts;
@@ -463,6 +480,18 @@ void disable_qinval(struct vtd_iommu *iommu)
 out:
     spin_unlock_irqrestore(&iommu->register_lock, flags);
 
-    iommu->flush.context = vtd_flush_context_reg;
-    iommu->flush.iotlb   = vtd_flush_iotlb_reg;
+    /*
+     * Assign callbacks to noop to catch errors if register-based invalidation
+     * isn't supported.
+     */
+    if ( has_register_based_invalidation(iommu) )
+    {
+        iommu->flush.context = vtd_flush_context_reg;
+        iommu->flush.iotlb   = vtd_flush_iotlb_reg;
+    }
+    else
+    {
+        iommu->flush.context = vtd_flush_context_noop;
+        iommu->flush.iotlb   = vtd_flush_iotlb_noop;
+    }
 }
-- 
2.25.1

Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[Jan Beulich]

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 <chao.gao@intel.com>

In principle (with a minor nit further down)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[Julien Grall]

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

[Jan Beulich]

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

[Julien Grall]

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

[Chao Gao]

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 <chao.gao@intel.com>
>
>In principle (with a minor nit further down)
>Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>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

[Jan Beulich]

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

[Julien Grall]

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

[Roger Pau Monné]

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

[Jan Beulich]

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

[Roger Pau Monné]

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

[Jan Beulich]

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

[Chao Gao]

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

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> 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 <kevin.tian@intel.com>