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

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>,
	"Chao Gao" <chao.gao@intel.com>
Cc: xen-devel@lists.xenproject.org,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Kevin Tian <kevin.tian@intel.com>
Subject: Re: [PATCH v2] VT-d: Don't assume register-based invalidation is always supported
Date: Wed, 21 Apr 2021 11:23:13 +0200	[thread overview]
Message-ID: <9276b757-a340-b0f3-3427-13b7bc5dcb9e@suse.com> (raw)
In-Reply-To: <YH7+i/JVnmZGTHOF@Air-de-Roger>

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.

> Would it make sense to mark as tainted which could help identify the
> issue on production builds? Maybe that's too much.

Yeah, for something we expect shouldn't ever happen ...

Jan