Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Apr 21, 2021 at 11:23:13AM +0200, Jan Beulich wrote:
>On 20.04.2021 18:17, Roger Pau Monné wrote:
>> On Tue, Apr 20, 2021 at 05:38:51PM +0200, Jan Beulich wrote:
>>> On 20.04.2021 17:08, Roger Pau Monné wrote:
>>>> On Thu, Apr 02, 2020 at 04:06:06AM +0800, Chao Gao wrote:
>>>>> --- a/xen/drivers/passthrough/vtd/qinval.c
>>>>> +++ b/xen/drivers/passthrough/vtd/qinval.c
>>>>> @@ -442,6 +442,23 @@ int enable_qinval(struct vtd_iommu *iommu)
>>>>>      return 0;
>>>>>  }
>>>>>  
>>>>> +static int vtd_flush_context_noop(struct vtd_iommu *iommu, uint16_t did,
>>>>> +                                  uint16_t source_id, uint8_t 
>>>>> function_mask,
>>>>> +                                  uint64_t type, bool 
>>>>> flush_non_present_entry)
>>>>> +{
>>>>> +    dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush CONTEXT.\n");
>>>>> +    return -EIO;
>>>>> +}
>>>>> +
>>>>> +static int vtd_flush_iotlb_noop(struct vtd_iommu *iommu, uint16_t did,
>>>>> +                                uint64_t addr, unsigned int size_order,
>>>>> +                                uint64_t type, bool 
>>>>> flush_non_present_entry,
>>>>> +                                bool flush_dev_iotlb)
>>>>> +{
>>>>> +    dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: Cannot flush IOTLB.\n");
>>>>> +    return -EIO;
>>>>> +}
>>>>
>>>> I think I would add an ASSERT_UNREACHABLE() to both noop handlers
>>>> above, as I would expect trying to use them without the proper mode
>>>> being configured would point to an error elsewhere?
>>>
>>> If such an assertion triggered e.g. during S3 suspend/resume, it may
>>> lead to the box simply not doing anything useful, without there being
>>> any way to know what went wrong. If instead the system at least
>>> managed to resume, the log message could be observed.
>> 
>> Oh, OK then. I'm simply worried that people might ignore such one line
>> messages, maybe add a WARN?
>
>Hmm, yes, perhaps - would allow seeing right away where the call
>came from. Chao, I'd again be fine to flip the dprintk()-s to
>WARN()-s while committing. But of course only provided you and
>Kevin (as the maintainer) agree.

Sure, please go ahead.

Thanks
Chao