Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
On 10.02.2020 15:39, Andrew Cooper wrote:
> On 05/02/2020 09:57, Jan Beulich wrote:
>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu
>>> *iommu)
>>> loop_count = 1000;
>>> do {
>>> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>>> -comp_wait = get_field_from_reg_u32(status,
>>> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
>>> -
>>> IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
>>> +comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
>> Unless you also change comp_wait to bool, this just happens to
>> be correct this way because of the bit checked being at a low
>> enough position.
>>
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
>>> BUG_ON(!iommu || ((log != >event_log) && (log !=
>>> >ppr_log)));
>>>
>>> run_bit = ( log == >event_log ) ?
>>> -IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
>>> -IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
>>> +IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>>>
>>> /* wait until EventLogRun bit = 0 */
>>> do {
>>> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>>> -log_run = iommu_get_bit(entry, run_bit);
>>> +log_run = entry & run_bit;
>> Same here for log_run then. I also think run_bit would better
>> become unsigned int then.
>>
>> With these taken care of
>> Reviewed-by: Jan Beulich
>
> I have separate cleanup doing that (and more). I don't want to conflate
> unrelated changes - this patch is complicated enough to follow.
But strictly speaking the assignments end up wrong this way. If
you really think such (benign) wrongness is okay, then may I
please ask that you point out this aspect in half a sentence in
the description?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
On 05/02/2020 09:57, Jan Beulich wrote:
> On 03.02.2020 15:43, Andrew Cooper wrote:
>> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
>> loop_count = 1000;
>> do {
>> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> -comp_wait = get_field_from_reg_u32(status,
>> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
>> -
>> IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
>> +comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
> Unless you also change comp_wait to bool, this just happens to
> be correct this way because of the bit checked being at a low
> enough position.
>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
>> BUG_ON(!iommu || ((log != >event_log) && (log !=
>> >ppr_log)));
>>
>> run_bit = ( log == >event_log ) ?
>> -IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
>> -IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
>> +IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>>
>> /* wait until EventLogRun bit = 0 */
>> do {
>> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> -log_run = iommu_get_bit(entry, run_bit);
>> +log_run = entry & run_bit;
> Same here for log_run then. I also think run_bit would better
> become unsigned int then.
>
> With these taken care of
> Reviewed-by: Jan Beulich
I have separate cleanup doing that (and more). I don't want to conflate
unrelated changes - this patch is complicated enough to follow.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
On 03.02.2020 15:43, Andrew Cooper wrote:
> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
> loop_count = 1000;
> do {
> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> -comp_wait = get_field_from_reg_u32(status,
> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
> - IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
> +comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
Unless you also change comp_wait to bool, this just happens to
be correct this way because of the bit checked being at a low
enough position.
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
> BUG_ON(!iommu || ((log != >event_log) && (log !=
> >ppr_log)));
>
> run_bit = ( log == >event_log ) ?
> -IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
> -IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
> +IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>
> /* wait until EventLogRun bit = 0 */
> do {
> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> -log_run = iommu_get_bit(entry, run_bit);
> +log_run = entry & run_bit;
Same here for log_run then. I also think run_bit would better
become unsigned int then.
With these taken care of
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
These are obfuscations around simple bit operations, and the compiler really
can do a better job when it can see them normally:
add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-181 (-181)
Function old new delta
guest_iommu_add_ppr_log 266 251 -15
guest_iommu_add_event_log266 251 -15
iommu_reset_log 274 250 -24
guest_iommu_process_command 16021544 -58
guest_iommu_mmio_write 11231054 -69
Total: Before=3014099, After=3013918, chg -0.01%
Drop all status register MASK/SHIFT constants, and enumerate the bits
normally. Rename EVENT_OVERFLOW to EVENT_LOG_OVERFLOW for consistency. (The
field name in the spec is inconsistent, despite the description referring to
an overflow of the event log.)
The only semantic change is in iommu_reset_log() where 'run_bit' changes from
being a bit position to being a single-bit mask.
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
---
xen/drivers/passthrough/amd/iommu-defs.h | 34 +---
xen/drivers/passthrough/amd/iommu.h | 15 ---
xen/drivers/passthrough/amd/iommu_cmd.c | 8 +++---
xen/drivers/passthrough/amd/iommu_guest.c | 43 +--
xen/drivers/passthrough/amd/iommu_init.c | 21 +++
5 files changed, 43 insertions(+), 78 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu-defs.h
b/xen/drivers/passthrough/amd/iommu-defs.h
index f8b62cb033..963009de6a 100644
--- a/xen/drivers/passthrough/amd/iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -437,28 +437,18 @@ union amd_iommu_x2apic_control {
/* Status Register*/
#define IOMMU_STATUS_MMIO_OFFSET 0x2020
-#define IOMMU_STATUS_EVENT_OVERFLOW_MASK 0x0001
-#define IOMMU_STATUS_EVENT_OVERFLOW_SHIFT 0
-#define IOMMU_STATUS_EVENT_LOG_INT_MASK0x0002
-#define IOMMU_STATUS_EVENT_LOG_INT_SHIFT 1
-#define IOMMU_STATUS_COMP_WAIT_INT_MASK0x0004
-#define IOMMU_STATUS_COMP_WAIT_INT_SHIFT 2
-#define IOMMU_STATUS_EVENT_LOG_RUN_MASK0x0008
-#define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT 3
-#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK 0x0010
-#define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT 4
-#define IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK 0x0020
-#define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT 5
-#define IOMMU_STATUS_PPR_LOG_INT_MASK 0x0040
-#define IOMMU_STATUS_PPR_LOG_INT_SHIFT 6
-#define IOMMU_STATUS_PPR_LOG_RUN_MASK 0x0080
-#define IOMMU_STATUS_PPR_LOG_RUN_SHIFT 7
-#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK0x0100
-#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_SHIFT 8
-#define IOMMU_STATUS_GAPIC_LOG_INT_MASK 0x0200
-#define IOMMU_STATUS_GAPIC_LOG_INT_SHIFT9
-#define IOMMU_STATUS_GAPIC_LOG_RUN_MASK 0x0400
-#define IOMMU_STATUS_GAPIC_LOG_RUN_SHIFT10
+
+#define IOMMU_STATUS_EVENT_LOG_OVERFLOW 0x0001
+#define IOMMU_STATUS_EVENT_LOG_INT0x0002
+#define IOMMU_STATUS_COMP_WAIT_INT0x0004
+#define IOMMU_STATUS_EVENT_LOG_RUN0x0008
+#define IOMMU_STATUS_CMD_BUFFER_RUN 0x0010
+#define IOMMU_STATUS_PPR_LOG_OVERFLOW 0x0020
+#define IOMMU_STATUS_PPR_LOG_INT 0x0040
+#define IOMMU_STATUS_PPR_LOG_RUN 0x0080
+#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW 0x0100
+#define IOMMU_STATUS_GAPIC_LOG_INT0x0200
+#define IOMMU_STATUS_GAPIC_LOG_RUN0x0400
/* I/O Page Table */
#define IOMMU_PAGE_TABLE_ENTRY_SIZE8
diff --git a/xen/drivers/passthrough/amd/iommu.h
b/xen/drivers/passthrough/amd/iommu.h
index f590de8cbf..81b6812d3a 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -374,21 +374,6 @@ static inline void __free_amd_iommu_tables(void *table,
int order)
free_xenheap_pages(table, order);
}
-static inline void iommu_set_bit(uint32_t *reg, uint32_t bit)
-{
-set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, *reg, 1U << bit, bit, reg);
-}
-
-static inline void iommu_clear_bit(uint32_t *reg, uint32_t bit)
-{
-set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *reg, 1U << bit, bit, reg);
-}
-
-static inline uint32_t iommu_get_bit(uint32_t reg, uint32_t bit)
-{
-return get_field_from_reg_u32(reg, 1U << bit, bit);
-}
-
static inline int iommu_has_cap(struct amd_iommu *iommu, uint32_t bit)
{
return !!(iommu->cap.header & (1u << bit));
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c
b/xen/drivers/passthrough/amd/iommu_cmd.c
index 92eaab407b..166f0e7263 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -68,7 +68,7 @@ static void flush_command_buffer(struct amd_iommu *iommu)
int loop_count,
