Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control

From: Jan Beulich <JBeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	AndrewCooper <andrew.cooper3@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	TimDeegan <tim@xen.org>, "Julien Grall" <julien.grall@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Daniel De Graaf" <dgdegra@tycho.nsa.gov>
Subject: Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
Date: Fri, 19 Jul 2019 09:43:26 +0000	[thread overview]
Message-ID: <b7d4e591-a5e3-94f1-6870-f0b6e7e9daf7@suse.com> (raw)
In-Reply-To: <20190719090202.vzeernrydawwnzan@Air-de-Roger.citrite.net>

On 19.07.2019 11:02, Roger Pau Monné  wrote:
> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
>> On 18.07.2019 18:52, Roger Pau Monné  wrote:
>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
>>>> On 18.07.2019 15:46, Roger Pau Monné  wrote:
>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
>>>>> QEMU already traps writes to the command register, and it will manage
>>>>> INTx enabling/disabling by itself. I think the only check required is
>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
>>>>> way both MSI caspabilities cannot be enabled simultaneously. The
>>>>> function should not explicitly disable any of the other capabilities,
>>>>> and just return -EBUSY if the caller attempts for example to enable
>>>>> MSI while INTx or MSI-X is enabled.
>>>>
>>>> You do realize that pci_intx() only ever gets called for Xen
>>>> internally used interrupts, i.e. mainly the serial console one?
>>>
>>> You will have to bear with me because I'm not sure I understand why
>>> it does matter. Do you mean to point out that dom0 is the one in full
>>> control of INTx, and thus Xen shouldn't care of whether INTx and
>>> MSI(-X) are enabled at the same time?
>>>
>>> I still think that at least a warning should be printed if a caller
>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
>>> a quirk for some hardware issue?) it shouldn't be allowed on this new
>>> interface.
>>
>> I don't mind improvements to the current situation (i.e. such a
>> warning may indeed make sense); I merely stated how things currently
>> are. INTx treatment was completely left aside when MSI support was
>> introduced into Xen.
> 
> In order to give Marek a more concise reply, would you agree to return
> -EBUSY (or some error code) and print a warning message if the caller
> attempts to enable MSI(-X) while INTx is also enabled?

As to returning an error - I think so, yes. I'm less sure about logging
a message.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel