[PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[Jan Beulich]

None of these are regressions afaict, so considering how late we are
in the 4.15 process, I can see reasons to not take any of these. All
of them are backporting candidates though, imo.

1: correct off-by-1 in number-of-IOMMUs check
2: leave FECTL write to vtd_resume()
3: re-order register restoring in vtd_resume()
4: restore flush hooks when disabling qinval

Jan

[PATCH 1/4][4.15?] VT-d: correct off-by-1 in number-of-IOMMUs check

[Jan Beulich]

Otherwise, if we really run on a system with this many IOMMUs,
entering/leaving S3 would overrun iommu_state[].

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
There are more anomalies here, but since we were asked to not make any
cosmetic changes for patches to have a chance to go into 4.15, I've put
off correcting even the most obvious things (scope of MAX_IOMMUS and
nr_iommus) for a later patch.

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1168,10 +1168,10 @@ int __init iommu_alloc(struct acpi_drhd_
     unsigned long sagaw, nr_dom;
     int agaw;
 
-    if ( nr_iommus > MAX_IOMMUS )
+    if ( nr_iommus >= MAX_IOMMUS )
     {
         dprintk(XENLOG_ERR VTDPREFIX,
-                 "IOMMU: nr_iommus %d > MAX_IOMMUS\n", nr_iommus);
+                "IOMMU: nr_iommus %d > MAX_IOMMUS\n", nr_iommus + 1);
         return -ENOMEM;
     }
 

Re: [PATCH 1/4][4.15?] VT-d: correct off-by-1 in number-of-IOMMUs check

[Ian Jackson]

Jan Beulich writes ("[PATCH 1/4][4.15?] VT-d: correct off-by-1 in number-of-IOMMUs check"):
> Otherwise, if we really run on a system with this many IOMMUs,
> entering/leaving S3 would overrun iommu_state[].

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH 2/4][4.15?] VT-d: leave FECTL write to vtd_resume()

[Jan Beulich]

We shouldn't blindly unmask the interrupt when resuming. vtd_resume()
will restore the intended state.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2092,7 +2092,7 @@ static int adjust_vtd_irq_affinities(voi
 }
 __initcall(adjust_vtd_irq_affinities);
 
-static int __must_check init_vtd_hw(void)
+static int __must_check init_vtd_hw(bool resume)
 {
     struct acpi_drhd_unit *drhd;
     struct vtd_iommu *iommu;
@@ -2121,6 +2121,10 @@ static int __must_check init_vtd_hw(void
             disable_qinval(iommu);
         }
 
+        if ( resume )
+            /* FECTL write done by vtd_resume(). */
+            continue;
+
         spin_lock_irqsave(&iommu->register_lock, flags);
         sts = dmar_readl(iommu->reg, DMAR_FECTL_REG);
         sts &= ~DMA_FECTL_IM;
@@ -2320,7 +2324,7 @@ static int __init vtd_setup(void)
     P(iommu_hap_pt_share, "Shared EPT tables");
 #undef P
 
-    ret = init_vtd_hw();
+    ret = init_vtd_hw(false);
     if ( ret )
         goto error;
 
@@ -2590,7 +2594,7 @@ static void vtd_resume(void)
     if ( !iommu_enabled )
         return;
 
-    if ( init_vtd_hw() != 0  && force_iommu )
+    if ( init_vtd_hw(true) != 0 && force_iommu )
          panic("IOMMU setup failed, crash Xen for security purpose\n");
 
     for_each_drhd_unit ( drhd )

[PATCH 3/4][4.15?] VT-d: re-order register restoring in vtd_resume()

[Jan Beulich]

For one FECTL must be written last - the interrupt shouldn't be unmasked
without first having written the data and address needed to actually
deliver it. In the common case (when dma_msi_set_affinity() doesn't end
up bailing early) this happens from init_vtd_hw(), but for this to
actually have the intended effect we shouldn't subsequently overwrite
what was written there - this is only benign when old and new settings
match. Instead we should restore the registers ahead of calling
init_vtd_hw(), just for the unlikely case of dma_msi_set_affinity()
bailing early.

In the moved code drop some stray casts as well.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2594,6 +2594,21 @@ static void vtd_resume(void)
     if ( !iommu_enabled )
         return;
 
+    for_each_drhd_unit ( drhd )
+    {
+        iommu = drhd->iommu;
+        i = iommu->index;
+
+        spin_lock_irqsave(&iommu->register_lock, flags);
+        dmar_writel(iommu->reg, DMAR_FEDATA_REG,
+                    iommu_state[i][DMAR_FEDATA_REG]);
+        dmar_writel(iommu->reg, DMAR_FEADDR_REG,
+                    iommu_state[i][DMAR_FEADDR_REG]);
+        dmar_writel(iommu->reg, DMAR_FEUADDR_REG,
+                    iommu_state[i][DMAR_FEUADDR_REG]);
+        spin_unlock_irqrestore(&iommu->register_lock, flags);
+    }
+
     if ( init_vtd_hw(true) != 0 && force_iommu )
          panic("IOMMU setup failed, crash Xen for security purpose\n");
 
@@ -2605,12 +2620,6 @@ static void vtd_resume(void)
         spin_lock_irqsave(&iommu->register_lock, flags);
         dmar_writel(iommu->reg, DMAR_FECTL_REG,
                     (u32) iommu_state[i][DMAR_FECTL_REG]);
-        dmar_writel(iommu->reg, DMAR_FEDATA_REG,
-                    (u32) iommu_state[i][DMAR_FEDATA_REG]);
-        dmar_writel(iommu->reg, DMAR_FEADDR_REG,
-                    (u32) iommu_state[i][DMAR_FEADDR_REG]);
-        dmar_writel(iommu->reg, DMAR_FEUADDR_REG,
-                    (u32) iommu_state[i][DMAR_FEUADDR_REG]);
         spin_unlock_irqrestore(&iommu->register_lock, flags);
 
         iommu_enable_translation(drhd);

[PATCH 4/4][4.15?] VT-d: restore flush hooks when disabling qinval

[Jan Beulich]

Leaving the hooks untouched is at best a latent risk: There may well be
cases where some flush is needed, which then needs carrying out the
"register" way.

Switch from u<N> to uint<N>_t while needing to touch the function
headers anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -49,6 +49,16 @@ int iommu_flush_iec_global(struct vtd_io
 int iommu_flush_iec_index(struct vtd_iommu *iommu, u8 im, u16 iidx);
 void clear_fault_bits(struct vtd_iommu *iommu);
 
+int __must_check vtd_flush_context_reg(struct vtd_iommu *iommu, uint16_t did,
+                                       uint16_t source_id,
+                                       uint8_t function_mask, uint64_t type,
+                                       bool flush_non_present_entry);
+int __must_check vtd_flush_iotlb_reg(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);
+
 struct vtd_iommu *ioapic_to_iommu(unsigned int apic_id);
 struct vtd_iommu *hpet_to_iommu(unsigned int hpet_id);
 struct acpi_drhd_unit *ioapic_to_drhd(unsigned int apic_id);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -380,10 +380,9 @@ static void iommu_flush_write_buffer(str
 }
 
 /* return value determine if we need a write buffer flush */
-static int __must_check flush_context_reg(struct vtd_iommu *iommu, u16 did,
-                                          u16 source_id, u8 function_mask,
-                                          u64 type,
-                                          bool flush_non_present_entry)
+int vtd_flush_context_reg(struct vtd_iommu *iommu, uint16_t did,
+                          uint16_t source_id, uint8_t function_mask,
+                          uint64_t type, bool flush_non_present_entry)
 {
     u64 val = 0;
     unsigned long flags;
@@ -449,11 +448,9 @@ static int __must_check iommu_flush_cont
 }
 
 /* return value determine if we need a write buffer flush */
-static int __must_check flush_iotlb_reg(struct vtd_iommu *iommu, u16 did,
-                                        u64 addr,
-                                        unsigned int size_order, u64 type,
-                                        bool flush_non_present_entry,
-                                        bool flush_dev_iotlb)
+int vtd_flush_iotlb_reg(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)
 {
     int tlb_offset = ecap_iotlb_offset(iommu->ecap);
     u64 val = 0;
@@ -2144,8 +2141,8 @@ static int __must_check init_vtd_hw(bool
          */
         if ( enable_qinval(iommu) != 0 )
         {
-            iommu->flush.context = flush_context_reg;
-            iommu->flush.iotlb   = flush_iotlb_reg;
+            iommu->flush.context = vtd_flush_context_reg;
+            iommu->flush.iotlb   = vtd_flush_iotlb_reg;
         }
     }
 
--- a/xen/drivers/passthrough/vtd/qinval.c
+++ b/xen/drivers/passthrough/vtd/qinval.c
@@ -462,4 +462,7 @@ void disable_qinval(struct vtd_iommu *io
                   !(sts & DMA_GSTS_QIES), sts);
 out:
     spin_unlock_irqrestore(&iommu->register_lock, flags);
+
+    iommu->flush.context = vtd_flush_context_reg;
+    iommu->flush.iotlb   = vtd_flush_iotlb_reg;
 }

RE: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, March 18, 2021 6:12 PM
> 
> None of these are regressions afaict, so considering how late we are
> in the 4.15 process, I can see reasons to not take any of these. All
> of them are backporting candidates though, imo.
> 
> 1: correct off-by-1 in number-of-IOMMUs check
> 2: leave FECTL write to vtd_resume()
> 3: re-order register restoring in vtd_resume()
> 4: restore flush hooks when disabling qinval
> 

For the series:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[Jan Beulich]

On 23.03.2021 09:12, Tian, Kevin wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, March 18, 2021 6:12 PM
>>
>> None of these are regressions afaict, so considering how late we are
>> in the 4.15 process, I can see reasons to not take any of these. All
>> of them are backporting candidates though, imo.
>>
>> 1: correct off-by-1 in number-of-IOMMUs check
>> 2: leave FECTL write to vtd_resume()
>> 3: re-order register restoring in vtd_resume()
>> 4: restore flush hooks when disabling qinval
>>
> 
> For the series:
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Thanks Kevin. Ian - what are your thoughts here towards 4.15?

Jan

Re: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections"):
> Thanks Kevin. Ian - what are your thoughts here towards 4.15?

I looked at these four patches.

In general I am not sure of the implications.  There are two important
sets of implications: (i) upside: what is the bug this fixes and how
severe is that bug *in its actual impact on users of Xen* (ii) what
possible problems might there be and how have we made sure that the
patch is right ?

I want look at this not from the point of view of technical details
but in terms of user impact.  User impact is harder to predict but it
is what we actually care about.

For one of the patches it seemed obvious to me that there was very
little downside risk and the upside is not corrupting something
(perhaps something important).

For the others, all I could see, besides the general statement that
these aren't regressions, there was a lot of intensive discussion in
the commit messages of the specific technical details.  Frankly, that
all went quite over my head.

I would be prepared to give a release ack for the others if I can be
convinced of satisfactory answers to my questions (i) and (ii).  For
an idea of what kind of answer I'm looking for, see the kind of thing
Roger has been putting in his 4.15-targeted patches.  The more complex
and to-me-impenetrable the underlying technical details the more
sceptical I will be :-).

I hope that makes sense.

Thanks,
Ian.

Re: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections

[Jan Beulich]

On 23.03.2021 14:37, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH 0/4][4.15?] VT-d: mostly S3 related corrections"):
>> Thanks Kevin. Ian - what are your thoughts here towards 4.15?
> 
> I looked at these four patches.
> 
> In general I am not sure of the implications.  There are two important
> sets of implications: (i) upside: what is the bug this fixes and how
> severe is that bug *in its actual impact on users of Xen* (ii) what
> possible problems might there be and how have we made sure that the
> patch is right ?
> 
> I want look at this not from the point of view of technical details
> but in terms of user impact.  User impact is harder to predict but it
> is what we actually care about.
> 
> For one of the patches it seemed obvious to me that there was very
> little downside risk and the upside is not corrupting something
> (perhaps something important).
> 
> For the others, all I could see, besides the general statement that
> these aren't regressions, there was a lot of intensive discussion in
> the commit messages of the specific technical details.  Frankly, that
> all went quite over my head.
> 
> I would be prepared to give a release ack for the others if I can be
> convinced of satisfactory answers to my questions (i) and (ii).  For
> an idea of what kind of answer I'm looking for, see the kind of thing
> Roger has been putting in his 4.15-targeted patches.  The more complex
> and to-me-impenetrable the underlying technical details the more
> sceptical I will be :-).
> 
> I hope that makes sense.

Of course it does, and I'm sorry that I get to replying only now. Since
I didn't think I could make points towards these patches being important
enough to take at this point, I didn't see a point in making this a high
priority. Nevertheless, to address your requests:

(i) Very hard to tell. Patch 4 at least very likely is no more than a
"just in case" fix. The other two patches address issues where after
resume IOMMU faults may not work correctly anymore or where, if there
were any faults to be delivered while resuming, those may not get
delivered / handled correctly ("unpredictable behavior" is probably the
best description I could come up with). Users (admins) therefore may
not become aware of there being issues on their system / with one or
more of the guests.

(ii) I've tested that things still work on a system where S3 halfway
works. Beyond that I'm afraid it really was staring at the code that
both made me notice the issues and makes me believe things at least
aren't worse with the patches in place.

I'm intending to commit these to staging now, but not to 4.15-staging.
I'll queue them for backporting, though, as indicated previously.

Jan