Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: dS+vjrFfPlS4PVqHTSC91cOXFGLlNb3F4KTNnNBmWAuyW/uIPpUDIaB2wP+uEw7Rqdtkt8I6wA
 ET23v6hjpwxw==
IronPort-SDR: pY1rn2+yFwurYEEZusyAcMIbYAL5gNGeSAySVygwhPQlEZa/LtSF5sLmquVg15OsP7JQTpe5GH
 hRvS6lLleLKg==
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Alex Williamson <alex.williamson@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>
CC:     Jason Gunthorpe <jgg@nvidia.com>,
        "Dey, Megha" <megha.dey@intel.com>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "Pan, Jacob jun" <jacob.jun.pan@intel.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "Liu, Yi L" <yi.l.liu@intel.com>, "Lu, Baolu" <baolu.lu@intel.com>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        "Luck, Tony" <tony.luck@intel.com>,
        "Kumar, Sanjay K" <sanjay.k.kumar@intel.com>,
        LKML <linux-kernel@vger.kernel.org>, KVM <kvm@vger.kernel.org>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        "Peter Zijlstra" <peterz@infradead.org>,
        Marc Zyngier <maz@kernel.org>,
        "Bjorn Helgaas" <helgaas@kernel.org>
Subject: RE: Virtualizing MSI-X on IMS via VFIO
Thread-Topic: Virtualizing MSI-X on IMS via VFIO
Thread-Index: AddnMs7+4GfLhTceT8q8tdV8716lmQAZ7UiAAAoHBgAACsXtAAAX4LwAAA2wIPAABLctgAAACLXQAB0q6IAADZ4hAAAOY4tQ
Date:   Fri, 25 Jun 2021 05:21:11 +0000
Message-ID: <BN9PR11MB5433063F826F5CEC93BCE0E38C069@BN9PR11MB5433.namprd11.prod.outlook.com>
References: <MWHPR11MB1886E14C57689A253D9B40C08C079@MWHPR11MB1886.namprd11.prod.outlook.com>
        <8735t7wazk.ffs@nanos.tec.linutronix.de>
 <20210624154434.11809b8f.alex.williamson@redhat.com>
In-Reply-To: <20210624154434.11809b8f.alex.williamson@redhat.com>
Accept-Language: en-US
Content-Language: en-US
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SymMrEmzYyhclrL2oKYjasxO1vwW2riu1BEH1KE/evqU+I4fF75ah3lqrr8p?=
 =?us-ascii?Q?DT0XeFO0Fns0fun5i97VxjQq1yyOR/+GUOX789BOCKsXd3XPOFPa9Bazs6if?=
 =?us-ascii?Q?nbPyQICmkEQqM8IlMIQGmQVHLuR2nZsxzq7dacU4COxs/Ltede84/3VCMUCb?=
 =?us-ascii?Q?YToccPewPNJdwxI8i5qba+KvzO3RtWcJLwcsucstS6NYkbL+EA+FLu2rJj3U?=
 =?us-ascii?Q?0j/M1oGOSloHcfPXE6SFpb43MSIrMlvjPo1Al8o+e5GonDZguGg6FBGUlW4P?=
 =?us-ascii?Q?mpHezcUazvhmqA/Oa/mMZNCSifUOJmEpFIJ1VigTmjGSFO1+hCFnXM8zNgzD?=
 =?us-ascii?Q?OJdysLXt4DrxZ4njX1kEceFZlrJRg1R521CFUGU0dH0iNhygei7XFSwmuuCK?=
 =?us-ascii?Q?ytKILJiecg8SM1q66t3oYEFtovPMFRYlYB0/AXQjc8TWJsDZb86BU113TMzS?=
 =?us-ascii?Q?sOWSaQ601auCstFBPiuzNo4w01HH9TdId2I/1Fkq6nFJHIwm9rHrRjoNMPCF?=
 =?us-ascii?Q?a5/Ac0DsDOe0lIaNkMppEEu5nWdBVpjHGNlUDzK/Q8QS0K//tS6aGVZOb68j?=
 =?us-ascii?Q?NbMj+FcOCxKiu1j17b2J8lu0yciblxqvNUm5urbvMtFY+LuMq5ae5YccQTg8?=
 =?us-ascii?Q?Zis9hQuBrHC8V/swk5roc1NpJMN4s/Rec+4NSBZkgBs2jfrSpSMuqKBqdyQg?=
 =?us-ascii?Q?Rnxmy2WrK0blRxnddrcQh6jg1/5PqIep+JzcIz4LdbIMdWuOsiCV7CagcMQK?=
 =?us-ascii?Q?ktzgodC9zpy67N0Pp7YkPpvfXJvub4QqneHPR3OcLwDjMc0+HV9RuXJcZboH?=
 =?us-ascii?Q?FRkLmPQMUd9lIaYOabBpApc1EzYcIOqWxehJoxZk2IMpT2MrJhwvDbQIAWLf?=
 =?us-ascii?Q?UdzmAaphdBxy6D+SqTBahT+jneBcA0qCqatfQCENZf7bPk8xzrDEq1Ze9Uc7?=
 =?us-ascii?Q?HUJkFtPLpfOCrIy+zG94LMG7zIoZwjsXZeW/YeGSJFXoDm7LmuNjDO90bSkA?=
 =?us-ascii?Q?mYDrUYNQ+b0gvB+Y7vErBSPNEfZp46QOYVBhwkMU7Fgu7EePHhwrR2E4byV9?=
 =?us-ascii?Q?A6C3WgeU0cnD5/u/QC2KiGawzvuozlixFcMY8Asg9pIicAw6oAF2duLrAG7K?=
 =?us-ascii?Q?+w4H4Knxb1xvMWQQYcVgdTaSqygpwgUS6RDIZzjnovq4A3fOetLUXBOB9N+/?=
 =?us-ascii?Q?IhD1STMLaW75ZZY8+RDDptR7XCPhu23wRf5hjN/ZRnhse6OulbzRjdHUXd8a?=
 =?us-ascii?Q?ichUzefSmkkSQKvIbjsZ38DmS2CEoNYyXJ1Rrj8S307xUjBNhOSCO8+mrfET?=
 =?us-ascii?Q?TWJVXiWUhxiufPWUmFZKI9Xm?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5433.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f8f5ad9d-169a-4cfa-1267-08d937990bc9
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Jun 2021 05:21:11.5781
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: NjM6nhVyPK7n2Q40RaF1Iww3KTQByxARddLyJViLIcYrBw1VuDiNVIsYEMNLzr3m5hn/LsJH+E93GnWV0LgvLQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7PR11MB2596
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, June 25, 2021 5:45 AM
>=20
> On Thu, 24 Jun 2021 17:14:39 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
>=20
> > After studying the MSI-X specification again, I think there is another
> > option to solve this for MSI-X, i.e. the dynamic sizing part:
> >
> > MSI requires to disable MSI in order to update the number of enabled
> > vectors in the control word.
>=20
> Exactly what part of the spec requires this?  This is generally the
> convention I expect too, and there are complications around contiguous
> vectors and data field alignment, but I'm not actually able to find a
> requirement in the spec that MSI Enable must be 0 when modifying other
> writable fields or that writable fields are latched when MSI Enable is
> set.
>=20
> > MSI-X does not have that requirement as there is no 'number of used
> > vectors' control field. MSI-X provides a fixed sized vector table and
> > enabling MSI-X "activates" the full table.
> >
> > System software has to set proper messages in the table and eventually
> > associate the table entries to device (sub)functions if that's not
> > hardwired in the device and controlled by queue enablement etc.
> >
> > According to the specification there is no requirement for masked table
> > entries to contain a valid message:
> >
> >  "Mask Bit: ... When this bit is set, the function is prohibited from
> >                 sending a message using this MSI-X Table entry."
> >
> > which means that the function must reread the table entry when the mask
> > bit in the vector control word is cleared.
>=20
> What is a "valid" message as far as the device is concerned?  "Valid"
> is meaningful to system software and hardware, the device doesn't care.
>=20
> Like MSI above, I think the real question is when is the data latched
> by the hardware.  For MSI-X this seems to be addressed in (PCIe 5.0
> spec) 6.1.4.2 MSI-X Configuration:
>=20
>   Software must not modify the Address, Data, or Steering Tag fields of
>   an entry while it is unmasked.
>=20
> Followed by 6.1.4.5 Per-vector Masking and Function Masking:
>=20
>   For MSI-X, a Function is permitted to cache Address and Data values
>   from unmasked MSI-X Table entries. However, anytime software unmasks
>   a currently masked MSI-X Table entry either by Clearing its Mask bit
>   or by Clearing the Function Mask bit, the Function must update any
>   Address or Data values that it cached from that entry. If software
>   changes the Address or Data value of an entry while the entry is
>   unmasked, the result is undefined.
>=20
> So caching/latching occurs on unmask for MSI-X, but I can't find
> similar statements for MSI.  If you have, please note them.  It's
> possible MSI is per interrupt.

I checked PCI Local Bus Specification rev3.0. At that time MSI and
MSI-X were described/compared together in almost every paragraph=20
in 6.8.3.4 (Per-vector Masking and Function Masking). The paragraph
that you cited is the last one in that section. It's a pity that MSI is
not clarified in this paragraph but it gives me the impression that=20
MSI function is not permitted to cache address and data values.=20
Later after MSI and MSI-X descriptions were split into separate=20
sections in PCIe spec, this impression is definitely weakened a lot.

If true, this even implies that software is free to change data/addr
when MSI is unmasked, which is sort of counter-intuitive to most
people.=20

Then I further found below thread:

https://lore.kernel.org/lkml/1468426713-31431-1-git-send-email-marc.zyngier=
@arm.com/

It identified a device which does latch the message content in a
MSI-capable device, forcing the kernel to startup irq early before
enabling MSI capability.

So, no answer and let's see whether Thomas can help identify
a better proof.

>=20
> Anyway, at least MSI-X if not also MSI could have a !NORESIZE
> implementation, which is why this flag exists in vfio.  Thanks,
>=20

For MSI we can still mitigate the lost interrupt issue by having
Qemu to allocate all possible MSI vectors in the start when guest=20
enables MSI capability and never freeing them before disable.
Anyway there are just up to 32 vectors per device, and total
vectors of all MSI devices in a platform should be limited. This
won't be a big problem after CPU vector exhaustion is relaxed.

p.s. one question to Thomas. As Alex cited above, software must=20
not modify the Address, Data, or Steering Tag fields of an MSI-X
entry while it is unmasked. However this rule might be violated
today in below flow:

request_irq()
    __setup_irq()
        irq_startup()
            __irq_startup()
                irq_enable()
                    unmask_irq() <<<<<<<<<<<<<
        irq_setup_affinity()
            irq_do_set_affinity()
                msi_set_affinity() // when IR is disabled
                    irq_msi_update_msg()
                        pci_msi_domain_write_msg() <<<<<<<<<<<<<<

Isn't above have msi-x entry updated after it's unmasked?=20

I may overlook something though since there are many branches
in the middle which may make above flow invalid...

Thanks
Kevin