Xen devel - Oct 2012 - [PATCH] VT-d: make remap_entry_to_msi_msg() return consistent message

During debugging of another problem I found that in x2APIC mode, the
destination field of the low address value wasn''t passed back
correctly. While this is benign in most cases (as the value isn''t being
used anywhere), it can be confusing (and misguiding) when printing the
value read or when comparing it to the one previously passed into the
inverse function.

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

--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -504,7 +504,11 @@ static int remap_entry_to_msi_msg(
             MSI_ADDR_REDIRECTION_CPU:
             MSI_ADDR_REDIRECTION_LOWPRI);
     if ( x2apic_enabled )
+    {
         msg->dest32 = iremap_entry->lo.dst;
+        msg->address_lo |+            (iremap_entry->lo.dst & 0xff)
<< MSI_ADDR_DEST_ID_SHIFT;
+    }
     else
         msg->address_lo |             ((iremap_entry->lo.dst >> 8)
& 0xff ) << MSI_ADDR_DEST_ID_SHIFT;





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

On 02/10/2012 15:55, "Jan Beulich" <JBeulich@suse.com> wrote:
> During debugging of another problem I found that in x2APIC mode, the
> destination field of the low address value wasn''t passed back
> correctly. While this is benign in most cases (as the value isn''t
being
> used anywhere), it can be confusing (and misguiding) when printing the
> value read or when comparing it to the one previously passed into the
> inverse function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -504,7 +504,11 @@ static int remap_entry_to_msi_msg(
>              MSI_ADDR_REDIRECTION_CPU:
>              MSI_ADDR_REDIRECTION_LOWPRI);
>      if ( x2apic_enabled )
> +    {
>          msg->dest32 = iremap_entry->lo.dst;
> +        msg->address_lo |> +            (iremap_entry->lo.dst
& 0xff) << MSI_ADDR_DEST_ID_SHIFT;
> +    }
>      else
>          msg->address_lo |>              ((iremap_entry->lo.dst
>> 8) & 0xff ) << MSI_ADDR_DEST_ID_SHIFT;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> Keir Fraser <keir@xen.org> 10/02/12 7:01 PM >>>
>On 02/10/2012 15:55, "Jan Beulich" <JBeulich@suse.com>
wrote:
>
>> During debugging of another problem I found that in x2APIC mode, the
>> destination field of the low address value wasn''t passed back
>> correctly. While this is benign in most cases (as the value
isn''t being
>> used anywhere), it can be confusing (and misguiding) when printing the
>> value read or when comparing it to the one previously passed into the
>> inverse function.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>Acked-by: Keir Fraser <keir@xen.org>
Actually on my way home yesterday I realized that this is not consistent, i.e.
fails to cover symmetrically the !x2apic case. Therefore, I''d like to
adjust this
to pull out the msg->dest32 assignment from the conditional. Will that be
okay
to commit without re-submission?

Jan
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -504,7 +504,11 @@ static int remap_entry_to_msi_msg(
>              MSI_ADDR_REDIRECTION_CPU:
>              MSI_ADDR_REDIRECTION_LOWPRI);
>      if ( x2apic_enabled )
> +    {
>          msg->dest32 = iremap_entry->lo.dst;
> +        msg->address_lo |> +            (iremap_entry->lo.dst
& 0xff) << MSI_ADDR_DEST_ID_SHIFT;
> +    }
>      else
>          msg->address_lo |>              ((iremap_entry->lo.dst
>> 8) & 0xff ) << MSI_ADDR_DEST_ID_SHIFT;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On 03/10/2012 13:58, "Jan Beulich" <jbeulich@suse.com> wrote:
>>>> Keir Fraser <keir@xen.org> 10/02/12 7:01 PM >>>
>> On 02/10/2012 15:55, "Jan Beulich" <JBeulich@suse.com>
wrote:
>> 
>>> During debugging of another problem I found that in x2APIC mode,
the
>>> destination field of the low address value wasn''t passed
back
>>> correctly. While this is benign in most cases (as the value
isn''t being
>>> used anywhere), it can be confusing (and misguiding) when printing
the
>>> value read or when comparing it to the one previously passed into
the
>>> inverse function.
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Acked-by: Keir Fraser <keir@xen.org>
> 
> Actually on my way home yesterday I realized that this is not consistent,
i.e.
> fails to cover symmetrically the !x2apic case. Therefore, I''d like
to adjust
> this
> to pull out the msg->dest32 assignment from the conditional. Will that
be okay
> to commit without re-submission?
Yes, that''s fine by me.

 -- Keir
> Jan
> 
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -504,7 +504,11 @@ static int remap_entry_to_msi_msg(
>>              MSI_ADDR_REDIRECTION_CPU:
>>              MSI_ADDR_REDIRECTION_LOWPRI);
>>      if ( x2apic_enabled )
>> +    {
>>          msg->dest32 = iremap_entry->lo.dst;
>> +        msg->address_lo |>> +           
(iremap_entry->lo.dst & 0xff) << MSI_ADDR_DEST_ID_SHIFT;
>> +    }
>>      else
>>          msg->address_lo |>>             
((iremap_entry->lo.dst >> 8) & 0xff ) <<
MSI_ADDR_DEST_ID_SHIFT;
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> 
> 
> 
>