* AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
@ 2013-06-03 7:12 Jan Beulich
2013-06-03 9:43 ` Andrew Cooper
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2013-06-03 7:12 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 3582 bytes --]
The code this patch added is redundant with already present code in
set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
ones from using writel() to writeq() for consistency.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -154,11 +154,6 @@ static void register_iommu_cmd_buffer_in
IOMMU_CMD_BUFFER_LENGTH_MASK,
IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET);
-
- /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
- * automatically for us. */
- writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
}
static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
@@ -187,11 +182,6 @@ static void register_iommu_event_log_in_
IOMMU_EVENT_LOG_LENGTH_MASK,
IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET);
-
- /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
- * automatically for us. */
- writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
}
static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
@@ -220,9 +210,6 @@ static void register_iommu_ppr_log_in_mm
IOMMU_PPR_LOG_LENGTH_MASK,
IOMMU_PPR_LOG_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_HIGH_OFFSET);
-
- writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
}
@@ -267,8 +254,8 @@ static void set_iommu_command_buffer_con
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
}
@@ -326,8 +313,8 @@ static void set_iommu_event_log_control(
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
@@ -353,8 +340,8 @@ static void set_iommu_ppr_log_control(st
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
[-- Attachment #2: AMD-IOMMU-erratum-64-revert.patch --]
[-- Type: text/plain, Size: 3650 bytes --]
AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
The code this patch added is redundant with already present code in
set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
ones from using writel() to writeq() for consistency.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -154,11 +154,6 @@ static void register_iommu_cmd_buffer_in
IOMMU_CMD_BUFFER_LENGTH_MASK,
IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET);
-
- /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
- * automatically for us. */
- writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
}
static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
@@ -187,11 +182,6 @@ static void register_iommu_event_log_in_
IOMMU_EVENT_LOG_LENGTH_MASK,
IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET);
-
- /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
- * automatically for us. */
- writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
}
static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
@@ -220,9 +210,6 @@ static void register_iommu_ppr_log_in_mm
IOMMU_PPR_LOG_LENGTH_MASK,
IOMMU_PPR_LOG_LENGTH_SHIFT, &entry);
writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_HIGH_OFFSET);
-
- writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
- writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
}
@@ -267,8 +254,8 @@ static void set_iommu_command_buffer_con
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
}
@@ -326,8 +313,8 @@ static void set_iommu_event_log_control(
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
@@ -353,8 +340,8 @@ static void set_iommu_ppr_log_control(st
/*reset head and tail pointer manually before enablement */
if ( enable )
{
- writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
- writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
+ writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-03 7:12 AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers" Jan Beulich
@ 2013-06-03 9:43 ` Andrew Cooper
2013-06-04 4:41 ` Suravee Suthikulpanit
2013-06-04 9:24 ` George Dunlap
0 siblings, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2013-06-03 9:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: suravee.suthikulpanit, xen-devel
On 03/06/13 08:12, Jan Beulich wrote:
> The code this patch added is redundant with already present code in
> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
> ones from using writel() to writeq() for consistency.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Yikes - I clearly didn't do a good job looking to see whether this issue
had already been addressed.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -154,11 +154,6 @@ static void register_iommu_cmd_buffer_in
> IOMMU_CMD_BUFFER_LENGTH_MASK,
> IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry);
> writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET);
> -
> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
> - * automatically for us. */
> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
> }
>
> static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
> @@ -187,11 +182,6 @@ static void register_iommu_event_log_in_
> IOMMU_EVENT_LOG_LENGTH_MASK,
> IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry);
> writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET);
> -
> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
> - * automatically for us. */
> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
> }
>
> static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
> @@ -220,9 +210,6 @@ static void register_iommu_ppr_log_in_mm
> IOMMU_PPR_LOG_LENGTH_MASK,
> IOMMU_PPR_LOG_LENGTH_SHIFT, &entry);
> writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_HIGH_OFFSET);
> -
> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
> }
>
>
> @@ -267,8 +254,8 @@ static void set_iommu_command_buffer_con
> /*reset head and tail pointer manually before enablement */
> if ( enable )
> {
> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>
> iommu_set_bit(&entry, IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
> }
> @@ -326,8 +313,8 @@ static void set_iommu_event_log_control(
> /*reset head and tail pointer manually before enablement */
> if ( enable )
> {
> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>
> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
> @@ -353,8 +340,8 @@ static void set_iommu_ppr_log_control(st
> /*reset head and tail pointer manually before enablement */
> if ( enable )
> {
> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>
> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-03 9:43 ` Andrew Cooper
@ 2013-06-04 4:41 ` Suravee Suthikulpanit
2013-06-04 6:33 ` Jan Beulich
2013-06-04 9:24 ` George Dunlap
1 sibling, 1 reply; 10+ messages in thread
From: Suravee Suthikulpanit @ 2013-06-04 4:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Jan Beulich, xen-devel
Sorry, I also missed that :(
Suravee
On 6/3/2013 4:43 AM, Andrew Cooper wrote:
> On 03/06/13 08:12, Jan Beulich wrote:
>> The code this patch added is redundant with already present code in
>> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
>> ones from using writel() to writeq() for consistency.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Yikes - I clearly didn't do a good job looking to see whether this issue
> had already been addressed.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -154,11 +154,6 @@ static void register_iommu_cmd_buffer_in
>> IOMMU_CMD_BUFFER_LENGTH_MASK,
>> IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry);
>> writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET);
>> -
>> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
>> - * automatically for us. */
>> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>> }
>>
>> static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
>> @@ -187,11 +182,6 @@ static void register_iommu_event_log_in_
>> IOMMU_EVENT_LOG_LENGTH_MASK,
>> IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry);
>> writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET);
>> -
>> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not happen
>> - * automatically for us. */
>> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>> }
>>
>> static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
>> @@ -220,9 +210,6 @@ static void register_iommu_ppr_log_in_mm
>> IOMMU_PPR_LOG_LENGTH_MASK,
>> IOMMU_PPR_LOG_LENGTH_SHIFT, &entry);
>> writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_HIGH_OFFSET);
>> -
>> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>> }
>>
>>
>> @@ -267,8 +254,8 @@ static void set_iommu_command_buffer_con
>> /*reset head and tail pointer manually before enablement */
>> if ( enable )
>> {
>> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>>
>> iommu_set_bit(&entry, IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
>> }
>> @@ -326,8 +313,8 @@ static void set_iommu_event_log_control(
>> /*reset head and tail pointer manually before enablement */
>> if ( enable )
>> {
>> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>>
>> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
>> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
>> @@ -353,8 +340,8 @@ static void set_iommu_ppr_log_control(st
>> /*reset head and tail pointer manually before enablement */
>> if ( enable )
>> {
>> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>>
>> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
>> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 4:41 ` Suravee Suthikulpanit
@ 2013-06-04 6:33 ` Jan Beulich
2013-06-05 6:53 ` Suravee Suthikulpanit
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2013-06-04 6:33 UTC (permalink / raw)
To: Suravee Suthikulpanit; +Cc: Andrew Cooper, xen-devel
>>> On 04.06.13 at 06:41, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
> Sorry, I also missed that :(
Is this an unusual form of an acked-by/reviewed-by?
Jan
> On 6/3/2013 4:43 AM, Andrew Cooper wrote:
>> On 03/06/13 08:12, Jan Beulich wrote:
>>> The code this patch added is redundant with already present code in
>>> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
>>> ones from using writel() to writeq() for consistency.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Yikes - I clearly didn't do a good job looking to see whether this issue
>> had already been addressed.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -154,11 +154,6 @@ static void register_iommu_cmd_buffer_in
>>> IOMMU_CMD_BUFFER_LENGTH_MASK,
>>> IOMMU_CMD_BUFFER_LENGTH_SHIFT, &entry);
>>> writel(entry, iommu->mmio_base+IOMMU_CMD_BUFFER_BASE_HIGH_OFFSET);
>>> -
>>> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not
> happen
>>> - * automatically for us. */
>>> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>> - writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>>> }
>>>
>>> static void register_iommu_event_log_in_mmio_space(struct amd_iommu
> *iommu)
>>> @@ -187,11 +182,6 @@ static void register_iommu_event_log_in_
>>> IOMMU_EVENT_LOG_LENGTH_MASK,
>>> IOMMU_EVENT_LOG_LENGTH_SHIFT, &entry);
>>> writel(entry, iommu->mmio_base+IOMMU_EVENT_LOG_BASE_HIGH_OFFSET);
>>> -
>>> - /* Reset head/tail pointer. SR56x0 Erratum 64 means this might not
> happen
>>> - * automatically for us. */
>>> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>>> - writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>>> }
>>>
>>> static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
>>> @@ -220,9 +210,6 @@ static void register_iommu_ppr_log_in_mm
>>> IOMMU_PPR_LOG_LENGTH_MASK,
>>> IOMMU_PPR_LOG_LENGTH_SHIFT, &entry);
>>> writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_HIGH_OFFSET);
>>> -
>>> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>>> - writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>>> }
>>>
>>>
>>> @@ -267,8 +254,8 @@ static void set_iommu_command_buffer_con
>>> /*reset head and tail pointer manually before enablement */
>>> if ( enable )
>>> {
>>> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>> - writel(0x0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
>>>
>>> iommu_set_bit(&entry, IOMMU_CONTROL_COMMAND_BUFFER_ENABLE_SHIFT);
>>> }
>>> @@ -326,8 +313,8 @@ static void set_iommu_event_log_control(
>>> /*reset head and tail pointer manually before enablement */
>>> if ( enable )
>>> {
>>> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>>> - writel(0x0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_HEAD_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_EVENT_LOG_TAIL_OFFSET);
>>>
>>> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_INT_SHIFT);
>>> iommu_set_bit(&entry, IOMMU_CONTROL_EVENT_LOG_ENABLE_SHIFT);
>>> @@ -353,8 +340,8 @@ static void set_iommu_ppr_log_control(st
>>> /*reset head and tail pointer manually before enablement */
>>> if ( enable )
>>> {
>>> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>>> - writel(0x0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_HEAD_OFFSET);
>>> + writeq(0, iommu->mmio_base + IOMMU_PPR_LOG_TAIL_OFFSET);
>>>
>>> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_ENABLE_SHIFT);
>>> iommu_set_bit(&entry, IOMMU_CONTROL_PPR_INT_SHIFT);
>>>
>>>
>>>
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-03 9:43 ` Andrew Cooper
2013-06-04 4:41 ` Suravee Suthikulpanit
@ 2013-06-04 9:24 ` George Dunlap
2013-06-04 9:30 ` Andrew Cooper
1 sibling, 1 reply; 10+ messages in thread
From: George Dunlap @ 2013-06-04 9:24 UTC (permalink / raw)
To: Andrew Cooper; +Cc: suravee.suthikulpanit, Jan Beulich, xen-devel
On Mon, Jun 3, 2013 at 10:43 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 03/06/13 08:12, Jan Beulich wrote:
>> The code this patch added is redundant with already present code in
>> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
>> ones from using writel() to writeq() for consistency.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Yikes - I clearly didn't do a good job looking to see whether this issue
> had already been addressed.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
You couldn't make this an "Acked-by", could you?
-George
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 9:24 ` George Dunlap
@ 2013-06-04 9:30 ` Andrew Cooper
2013-06-04 9:38 ` George Dunlap
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2013-06-04 9:30 UTC (permalink / raw)
To: George Dunlap; +Cc: suravee.suthikulpanit, Jan Beulich, xen-devel
On 04/06/13 10:24, George Dunlap wrote:
> On Mon, Jun 3, 2013 at 10:43 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 03/06/13 08:12, Jan Beulich wrote:
>>> The code this patch added is redundant with already present code in
>>> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
>>> ones from using writel() to writeq() for consistency.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Yikes - I clearly didn't do a good job looking to see whether this issue
>> had already been addressed.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> You couldn't make this an "Acked-by", could you?
>
> -George
I could do that, but as I am not a maintainer, I was under the
impression that reviewed-by was my preferred way of saying "I think this
is sensible".
~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 9:30 ` Andrew Cooper
@ 2013-06-04 9:38 ` George Dunlap
2013-06-04 9:53 ` Ian Campbell
0 siblings, 1 reply; 10+ messages in thread
From: George Dunlap @ 2013-06-04 9:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: suravee.suthikulpanit, Jan Beulich, xen-devel
On 06/04/2013 10:30 AM, Andrew Cooper wrote:
> On 04/06/13 10:24, George Dunlap wrote:
>> On Mon, Jun 3, 2013 at 10:43 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 03/06/13 08:12, Jan Beulich wrote:
>>>> The code this patch added is redundant with already present code in
>>>> set_iommu_{command_buffer,{event,ppr}_log}_control(). Just switch those
>>>> ones from using writel() to writeq() for consistency.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Yikes - I clearly didn't do a good job looking to see whether this issue
>>> had already been addressed.
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> You couldn't make this an "Acked-by", could you?
>>
>> -George
>
> I could do that, but as I am not a maintainer, I was under the
> impression that reviewed-by was my preferred way of saying "I think this
> is sensible".
In this context, I would read "Acked-by" as "I agree that this should go
in", or at very least, "I am happy for this to go in"; whereas to me
"Reviewed-by" to me sounds like, "I took a close look at the code and
didn't see anything wrong, but otherwise have no opinion on the matter."
But maybe I'm behind the times here. :-)
-George
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 9:38 ` George Dunlap
@ 2013-06-04 9:53 ` Ian Campbell
2013-06-04 9:56 ` George Dunlap
0 siblings, 1 reply; 10+ messages in thread
From: Ian Campbell @ 2013-06-04 9:53 UTC (permalink / raw)
To: George Dunlap
Cc: Andrew Cooper, Jan Beulich, suravee.suthikulpanit, xen-devel
On Tue, 2013-06-04 at 10:38 +0100, George Dunlap wrote:
> In this context, I would read "Acked-by" as "I agree that this should go
> in", or at very least, "I am happy for this to go in"; whereas to me
> "Reviewed-by" to me sounds like, "I took a close look at the code and
> didn't see anything wrong, but otherwise have no opinion on the matter."
I think we decided (somewhat informally I think) that we interpreted
Foo-by according to Linux's Documentation/SubmittingPatches:
If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
arrange to have an Acked-by: line added to the patch's changelog.
Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.
Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
has at least reviewed the patch and has indicated acceptance. Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by:.
Acked-by: does not necessarily indicate acknowledgement of the entire patch.
For example, if a patch affects multiple subsystems and has an Acked-by: from
one subsystem maintainer then this usually indicates acknowledgement of just
the part which affects that maintainer's code. Judgement should be used here.
When in doubt people should refer to the original discussion in the mailing
list archives.
[...]
Reviewed-by is somehow more formal:
Reviewed-by:, instead, indicates that the patch has been reviewed and found
acceptable according to the Reviewer's Statement:
Reviewer's statement of oversight
By offering my Reviewed-by: tag, I state that:
(a) I have carried out a technical review of this patch to
evaluate its appropriateness and readiness for inclusion into
the mainline kernel.
(b) Any problems, concerns, or questions relating to the patch
have been communicated back to the submitter. I am satisfied
with the submitter's response to my comments.
(c) While there may be things that could be improved with this
submission, I believe that it is, at this time, (1) a
worthwhile modification to the kernel, and (2) free of known
issues which would argue against its inclusion.
(d) While I have reviewed the patch and believe it to be sound, I
do not (unless explicitly stated elsewhere) make any
warranties or guarantees that it will achieve its stated
purpose or function properly in any given situation.
A Reviewed-by tag is a statement of opinion that the patch is an
appropriate modification of the kernel without any remaining serious
technical issues. Any interested reviewer (who has done the work) can
offer a Reviewed-by tag for a patch. This tag serves to give credit to
reviewers and to inform maintainers of the degree of review which has been
done on the patch. Reviewed-by: tags, when supplied by reviewers known to
understand the subject area and to perform thorough reviews, will normally
increase the likelihood of your patch getting into the kernel.
Acked-by is supposedly something which is offered by the maintainer of
the relevant code to indicate they are happy for it to go in. In the
Linux world that might be via a different maintainer's tree (for cross
subsystem stuff) or be an indication from e.g. a driver maintainer to
the subsystem maintainer that the patch can be applied. In the Xen world
I think we interpret an Ack from someone in MAINTAINERS as a signal to
the committers that the patch should be committed.
Ian.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 9:53 ` Ian Campbell
@ 2013-06-04 9:56 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2013-06-04 9:56 UTC (permalink / raw)
To: Ian Campbell; +Cc: Andrew Cooper, Jan Beulich, suravee.suthikulpanit, xen-devel
On 06/04/2013 10:53 AM, Ian Campbell wrote:
> On Tue, 2013-06-04 at 10:38 +0100, George Dunlap wrote:
>> In this context, I would read "Acked-by" as "I agree that this should go
>> in", or at very least, "I am happy for this to go in"; whereas to me
>> "Reviewed-by" to me sounds like, "I took a close look at the code and
>> didn't see anything wrong, but otherwise have no opinion on the matter."
>
> I think we decided (somewhat informally I think) that we interpreted
> Foo-by according to Linux's Documentation/SubmittingPatches:
>
> If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> arrange to have an Acked-by: line added to the patch's changelog.
>
> Acked-by: is often used by the maintainer of the affected code when that
> maintainer neither contributed to nor forwarded the patch.
>
> Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
> has at least reviewed the patch and has indicated acceptance. Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by:.
>
> Acked-by: does not necessarily indicate acknowledgement of the entire patch.
> For example, if a patch affects multiple subsystems and has an Acked-by: from
> one subsystem maintainer then this usually indicates acknowledgement of just
> the part which affects that maintainer's code. Judgement should be used here.
> When in doubt people should refer to the original discussion in the mailing
> list archives.
>
> [...]
>
> Reviewed-by is somehow more formal:
>
> Reviewed-by:, instead, indicates that the patch has been reviewed and found
> acceptable according to the Reviewer's Statement:
>
> Reviewer's statement of oversight
>
> By offering my Reviewed-by: tag, I state that:
>
> (a) I have carried out a technical review of this patch to
> evaluate its appropriateness and readiness for inclusion into
> the mainline kernel.
>
> (b) Any problems, concerns, or questions relating to the patch
> have been communicated back to the submitter. I am satisfied
> with the submitter's response to my comments.
>
> (c) While there may be things that could be improved with this
> submission, I believe that it is, at this time, (1) a
> worthwhile modification to the kernel, and (2) free of known
> issues which would argue against its inclusion.
>
> (d) While I have reviewed the patch and believe it to be sound, I
> do not (unless explicitly stated elsewhere) make any
> warranties or guarantees that it will achieve its stated
> purpose or function properly in any given situation.
>
> A Reviewed-by tag is a statement of opinion that the patch is an
> appropriate modification of the kernel without any remaining serious
> technical issues. Any interested reviewer (who has done the work) can
> offer a Reviewed-by tag for a patch. This tag serves to give credit to
> reviewers and to inform maintainers of the degree of review which has been
> done on the patch. Reviewed-by: tags, when supplied by reviewers known to
> understand the subject area and to perform thorough reviews, will normally
> increase the likelihood of your patch getting into the kernel.
>
> Acked-by is supposedly something which is offered by the maintainer of
> the relevant code to indicate they are happy for it to go in. In the
> Linux world that might be via a different maintainer's tree (for cross
> subsystem stuff) or be an indication from e.g. a driver maintainer to
> the subsystem maintainer that the patch can be applied. In the Xen world
> I think we interpret an Ack from someone in MAINTAINERS as a signal to
> the committers that the patch should be committed.
I stand corrected.
-G
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers"
2013-06-04 6:33 ` Jan Beulich
@ 2013-06-05 6:53 ` Suravee Suthikulpanit
0 siblings, 0 replies; 10+ messages in thread
From: Suravee Suthikulpanit @ 2013-06-05 6:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On 6/4/2013 1:33 AM, Jan Beulich wrote:
>>>> On 04.06.13 at 06:41, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> wrote:
>> Sorry, I also missed that :(
> Is this an unusual form of an acked-by/reviewed-by?
>
> Jan
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Thanks,
Suravee
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-06-05 6:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-03 7:12 AMD/IOMMU: revert "SR56x0 Erratum 64 - Reset all head & tail pointers" Jan Beulich
2013-06-03 9:43 ` Andrew Cooper
2013-06-04 4:41 ` Suravee Suthikulpanit
2013-06-04 6:33 ` Jan Beulich
2013-06-05 6:53 ` Suravee Suthikulpanit
2013-06-04 9:24 ` George Dunlap
2013-06-04 9:30 ` Andrew Cooper
2013-06-04 9:38 ` George Dunlap
2013-06-04 9:53 ` Ian Campbell
2013-06-04 9:56 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.