Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers

[Jan Beulich <jbeulich@xxxxxxxx>]

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 != &iommu->event_log) && (log != 
>>> &iommu->ppr_log)));
>>>  
>>>      run_bit = ( log == &iommu->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 <jbeulich@xxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel