Patchwork VT-d: correct a comment and remove useless if() statement

login
register
mail settings
Submitter Chao Gao
Date April 12, 2017, 12:04 a.m.
Message ID <1491955450-21560-1-git-send-email-chao.gao@intel.com>
Download mbox | patch
Permalink /patch/222023/
State New
Headers show

Comments

Chao Gao - April 12, 2017, 12:04 a.m.
fix two flaws in the patch (93358e8e VT-d: introduce update_irte to update
irte safely).

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/drivers/passthrough/vtd/intremap.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Chao Gao - April 12, 2017, 2:41 a.m.
On Wed, Apr 12, 2017 at 02:57:23AM -0600, Jan Beulich wrote:
>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -200,8 +200,9 @@ static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
>>      else
>>      {
>>          /*
>> -         * If the caller requests an atomic update but we can't meet it, 
>> -         * a bug will be raised.
>> +         * VT-d hardware doesn't update IRTEs behind us, nor the software.
>
>Hmm, so far I was under the impression that in posted mode the
>IRTE could be updated by hardware. Is that not the case? As to

No. I have confirmed this with VT-d architect that VT-d hardware 
doesn't update IRTEs. For posted format, VT-d hardware will atomically
update Posted Interrupt Descriptor, an address recorded in posted
format IRTE.

>software not updating, with there not being any synchronization
>clearly visible around here, I'm afraid this also needs expanding
>on (in the commit message at least, not necessarily in the
>comment).

Will do.

>
>> +         * If a caller want an atomic update from the views of VT-d
>
>wants
>
>Also what do you mean by "from the views of VT-d"?

OK. will fix this too. Do you mean it is (and should be) atomic from software's view
, so these words are redundant.

Thanks
Chao
Chao Gao - April 12, 2017, 3:56 a.m.
On Wed, Apr 12, 2017 at 04:32:06AM -0600, Jan Beulich wrote:
>>>> On 12.04.17 at 04:41, <chao.gao@intel.com> wrote:
>> On Wed, Apr 12, 2017 at 02:57:23AM -0600, Jan Beulich wrote:
>>>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>>>> +         * If a caller want an atomic update from the views of VT-d
>>>
>>>wants
>>>
>>>Also what do you mean by "from the views of VT-d"?
>> 
>> OK. will fix this too. Do you mean it is (and should be) atomic from 
>> software's view
>> , so these words are redundant.
>
>I don't mean anything here until I understand what you mean.
>

I think making this update presented to VT-d hardware as an atomic update 
is this function's goal. Is that right? That's why I said "from the views fo
VT-d".

Thanks
Chao
Jan Beulich - April 12, 2017, 8:57 a.m.
>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -200,8 +200,9 @@ static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
>      else
>      {
>          /*
> -         * If the caller requests an atomic update but we can't meet it, 
> -         * a bug will be raised.
> +         * VT-d hardware doesn't update IRTEs behind us, nor the software.

Hmm, so far I was under the impression that in posted mode the
IRTE could be updated by hardware. Is that not the case? As to
software not updating, with there not being any synchronization
clearly visible around here, I'm afraid this also needs expanding
on (in the commit message at least, not necessarily in the
comment).

> +         * If a caller want an atomic update from the views of VT-d

wants

Also what do you mean by "from the views of VT-d"?

Jan
Jan Beulich - April 12, 2017, 10:32 a.m.
>>> On 12.04.17 at 04:41, <chao.gao@intel.com> wrote:
> On Wed, Apr 12, 2017 at 02:57:23AM -0600, Jan Beulich wrote:
>>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>>> +         * If a caller want an atomic update from the views of VT-d
>>
>>wants
>>
>>Also what do you mean by "from the views of VT-d"?
> 
> OK. will fix this too. Do you mean it is (and should be) atomic from 
> software's view
> , so these words are redundant.

I don't mean anything here until I understand what you mean.

Jan
Andrew Cooper - April 12, 2017, 10:58 a.m.
On 12/04/17 09:57, Jan Beulich wrote:
>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -200,8 +200,9 @@ static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
>>      else
>>      {
>>          /*
>> -         * If the caller requests an atomic update but we can't meet it, 
>> -         * a bug will be raised.
>> +         * VT-d hardware doesn't update IRTEs behind us, nor the software.
> Hmm, so far I was under the impression that in posted mode the
> IRTE could be updated by hardware. Is that not the case?

No.  The IRTE is just a translation structure, so conceptually similar
to a pagetable.

With posted interrupts, it is the PI block in memory (referenced by the
IRTE) which gets written to by hardware.

~Andrew
Jan Beulich - April 12, 2017, 12:04 p.m.
>>> On 12.04.17 at 12:58, <andrew.cooper3@citrix.com> wrote:
> On 12/04/17 09:57, Jan Beulich wrote:
>>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>>> --- a/xen/drivers/passthrough/vtd/intremap.c
>>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>>> @@ -200,8 +200,9 @@ static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
>>>      else
>>>      {
>>>          /*
>>> -         * If the caller requests an atomic update but we can't meet it, 
>>> -         * a bug will be raised.
>>> +         * VT-d hardware doesn't update IRTEs behind us, nor the software.
>> Hmm, so far I was under the impression that in posted mode the
>> IRTE could be updated by hardware. Is that not the case?
> 
> No.  The IRTE is just a translation structure, so conceptually similar
> to a pagetable.

Not the best analogy, as page tables do get written by the CPU.

> With posted interrupts, it is the PI block in memory (referenced by the
> IRTE) which gets written to by hardware.

Okay, then I apparently misremember Feng saying otherwise in
the early stages of the original series.

Jan
Jan Beulich - April 12, 2017, 12:05 p.m.
>>> On 12.04.17 at 05:56, <chao.gao@intel.com> wrote:
> On Wed, Apr 12, 2017 at 04:32:06AM -0600, Jan Beulich wrote:
>>>>> On 12.04.17 at 04:41, <chao.gao@intel.com> wrote:
>>> On Wed, Apr 12, 2017 at 02:57:23AM -0600, Jan Beulich wrote:
>>>>>>> On 12.04.17 at 02:04, <chao.gao@intel.com> wrote:
>>>>> +         * If a caller want an atomic update from the views of VT-d
>>>>
>>>>wants
>>>>
>>>>Also what do you mean by "from the views of VT-d"?
>>> 
>>> OK. will fix this too. Do you mean it is (and should be) atomic from 
>>> software's view
>>> , so these words are redundant.
>>
>>I don't mean anything here until I understand what you mean.
>>
> 
> I think making this update presented to VT-d hardware as an atomic update 
> is this function's goal. Is that right? That's why I said "from the views fo
> VT-d".

"If the caller wants VT-d hardware to always see a consistent
entry ..." perhaps?

Jan

Patch

diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index 699239b..8649666 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -200,8 +200,9 @@  static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
     else
     {
         /*
-         * If the caller requests an atomic update but we can't meet it, 
-         * a bug will be raised.
+         * VT-d hardware doesn't update IRTEs behind us, nor the software.
+         * If a caller want an atomic update from the views of VT-d
+         * hardware, but we can't meet it, a bug will be raised.
          */
         if ( entry->lo == new_ire->lo )
             write_atomic(&entry->hi, new_ire->hi);
@@ -690,8 +691,7 @@  static int msi_msg_to_remap_entry(
     remap_rte->data = index - i;
 
     update_irte(iommu, iremap_entry, &new_ire, msi_desc->irte_initialized);
-    if ( !msi_desc->irte_initialized )
-        msi_desc->irte_initialized = true;
+    msi_desc->irte_initialized = true;
 
     iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
     iommu_flush_iec_index(iommu, 0, index);