On 09.06.2021 12:36, Andrew Cooper wrote:
> On 09/06/2021 10:26, Jan Beulich wrote:
>> @@ -49,28 +52,31 @@ static void send_iommu_command(struct am
>> static void flush_command_buffer(struct amd_iommu *iommu,
>> unsigned int timeout_base)
>> {
>> + static DEFINE_PER_CPU(uint64_t, poll_slot);
>> + uint64_t *this_poll_slot = &this_cpu(poll_slot);
>> + paddr_t addr = virt_to_maddr(this_poll_slot);
>> uint32_t cmd[4];
>> s_time_t start, timeout;
>> static unsigned int __read_mostly threshold = 1;
>>
>> - /* RW1C 'ComWaitInt' in status register */
>> - writel(IOMMU_STATUS_COMP_WAIT_INT,
>> - iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> -
>> - /* send an empty COMPLETION_WAIT command to flush command buffer */
>> - cmd[3] = cmd[2] = 0;
>> - set_field_in_reg_u32(IOMMU_CMD_COMPLETION_WAIT, 0,
>> + ACCESS_ONCE(*this_poll_slot) = CMD_COMPLETION_INIT;
>> +
>> + /* send a COMPLETION_WAIT command to flush command buffer */
>> + cmd[0] = addr;
>> + set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, cmd[0],
>> + IOMMU_COMP_WAIT_S_FLAG_MASK,
>> + IOMMU_COMP_WAIT_S_FLAG_SHIFT, &cmd[0]);
>
> set_field_in_reg_u32() is a disaster of a function - both in terms of
> semantics, and code gen - and needs to be purged from the code.
Long ago I had an item on my todo list to get this cleaned up. But
it never really having made it up high enough, I dropped it at
some point, in the hope that we'd manage to get this sorted while
re-writing code step by step.
> It is a shame we don't have a real struct for objects in the command
> buffer, but in lieu of that, this is just
>
> cmd[0] = addr | IOMMU_COMP_WAIT_S_FLAG_MASK;
>
> which is the direction that previous cleanup has gone.
I don't think I can spot a single instance of such. Some work was
done to introduce (mainly bitfield) structs, but this surely goes
too far for the change at hand. I can spot two instances using
MASK_INSR(), so I can see two consistent ways of doing what you
ask for:
cmd[0] = addr | MASK_INSR(IOMMU_CONTROL_ENABLED,
IOMMU_COMP_WAIT_S_FLAG_MASK);
keeping the name as *_MASK (and I'd be open to replace
IOMMU_CONTROL_ENABLED by true) or
cmd[0] = addr | IOMMU_COMP_WAIT_S_FLAG;
i.e. dropping _MASK (but requiring adjustments elsewhere in the
code). Please let me know which one you'd prefer.
> There are no current users of IOMMU_COMP_WAIT_S_FLAG_SHIFT, and ...
>
>> + cmd[1] = addr >> 32;
>> + set_field_in_reg_u32(IOMMU_CMD_COMPLETION_WAIT, cmd[1],
>> IOMMU_CMD_OPCODE_MASK,
>> IOMMU_CMD_OPCODE_SHIFT, &cmd[1]);
>> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
>> - IOMMU_COMP_WAIT_I_FLAG_MASK,
>> - IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]);
>
> ... this drops the final use of IOMMU_COMP_WAIT_I_FLAG_SHIFT, so both
> should be dropped.
Well, I can surely do so, but like this entire request of yours this
feels like scope creep - there was no intention here to do any
unrelated cleanup. And if I remove _S_ and _I_, then surely _F_
wants dropping as well, while IOMMU_COMP_WAIT_ADDR_*_SHIFT have a
use each in iommu_guest.c and hence need to stay for now.
> As for IOMMU_CMD_OPCODE_SHIFT, that can't be dropped yet, but it would
> still be better to use
>
> cmd[1] = (addr >> 32) | MASK_INSR(IOMMU_CMD_COMPLETION_WAIT,
> IOMMU_CMD_COMPLETION_WAIT);
>
> in the short term.
Can do (using IOMMU_CMD_OPCODE_MASK).
Jan
