All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	"moderated list:XEN HYPERVISOR INTERFACE"
	<xen-devel@lists.xenproject.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled
Date: Fri, 18 Nov 2022 08:36:14 +0100	[thread overview]
Message-ID: <93b6385c-63c8-1b5a-13c0-838f7c03ccce@suse.com> (raw)
In-Reply-To: <20221118023535.1903459-1-marmarek@invisiblethingslab.com>

On 18.11.2022 03:35, Marek Marczykowski-Górecki wrote:
> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
> the table is filled. Then it disables INTx just before clearing MASKALL
> bit. Currently this approach is rejected by xen-pciback.
> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
> enabled.

Similarly the spec doesn't allow using MSI and MSI-X at the same time.
Before your change xen_pcibk_get_interrupt_type() is consistent for all
three forms of interrupt delivery; imo it also wants to be consistent
after your change. This effectively would mean setting only one bit at
a time (or using an enum right away), but then the question is what
order you do the checks in. IOW I think the change to the function is
wrong.

Furthermore it looks to me as if you're making msi_msix_flags_write()
inconsistent with command_write() - you'd now want to also permit
clearing "INTx disable" when MSI or MSI-X are enabled. Which, I think,
would simply mean allowing the domain unconditional control of the bit
(as long as allow_interrupt_control is set of course).

Especially with these further changes I'm afraid at least for now I
view this as moving in the wrong direction. My view might change in
particular if the description made more clear what was wrong with the
original change (476878e4b2be ["xen-pciback: optionally allow interrupt
enable flag writes"]), or perhaps the discussion having led to the form
which was committed in the end.

Jan


  reply	other threads:[~2022-11-18  7:36 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-18  2:35 [PATCH v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is enabled Marek Marczykowski-Górecki
2022-11-18  7:36 ` Jan Beulich [this message]
2022-11-18 12:06   ` Marek Marczykowski-Górecki
2022-11-18 12:27     ` Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=93b6385c-63c8-1b5a-13c0-838f7c03ccce@suse.com \
    --to=jbeulich@suse.com \
    --cc=jgross@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marmarek@invisiblethingslab.com \
    --cc=oleksandr_tyshchenko@epam.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.