[PATCH] VT-d: correct a comment and remove useless if() statement

[PATCH] VT-d: correct a comment and remove useless if() statement

[Chao Gao]

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(-)

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);
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Chao Gao]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Chao Gao]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH] VT-d: correct a comment and remove useless if() statement

[Jan Beulich]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel