Re: [Xen-devel] [PATCH v3] xen-pciback: optionally allow interrupt enable flag writes
On Mon, Jan 13, 2020 at 04:25:02PM -0500, Boris Ostrovsky wrote:
>
>
> On 1/10/20 10:43 PM, Marek Marczykowski-Górecki wrote:
> > @@ -117,6 +117,24 @@ static int command_write(struct pci_dev *dev, int
> > offset, u16 value, void *data)
> > pci_clear_mwi(dev);
> > }
> > + if (dev_data && dev_data->allow_interrupt_control) {
> > + if ((cmd->val ^ val) & PCI_COMMAND_INTX_DISABLE) {
> > + if (value & PCI_COMMAND_INTX_DISABLE) {
> > + pci_intx(dev, 0);
> > + } else {
> > + /* Do not allow enabling INTx together with MSI
> > or MSI-X. */
> > + switch (xen_pcibk_get_interrupt_type(dev)) {
> > + case INTERRUPT_TYPE_NONE:
> > + case INTERRUPT_TYPE_INTX:
> > + pci_intx(dev, 1);
>
> If INTERRUPT_TYPE_INTX , why call pci_intx(1)?
Not needed indeed.
>
> (I think I asked this last time as well).
>
>
> -boris
>
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] xen-pciback: optionally allow interrupt enable flag writes
On 1/10/20 10:43 PM, Marek Marczykowski-Górecki wrote:
@@ -117,6 +117,24 @@ static int command_write(struct pci_dev *dev, int offset,
u16 value, void *data)
pci_clear_mwi(dev);
}
+ if (dev_data && dev_data->allow_interrupt_control) {
+ if ((cmd->val ^ val) & PCI_COMMAND_INTX_DISABLE) {
+ if (value & PCI_COMMAND_INTX_DISABLE) {
+ pci_intx(dev, 0);
+ } else {
+ /* Do not allow enabling INTx together with MSI
or MSI-X. */
+ switch (xen_pcibk_get_interrupt_type(dev)) {
+ case INTERRUPT_TYPE_NONE:
+ case INTERRUPT_TYPE_INTX:
+ pci_intx(dev, 1);
If INTERRUPT_TYPE_INTX , why call pci_intx(1)?
(I think I asked this last time as well).
-boris
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3] xen-pciback: optionally allow interrupt enable flag writes
Hi Marek,
Below is a report from 0day bot build w/ Clang. The warning looks
legit, can you please take a look? Apologies if this has already been
reported.
On Sat, Jan 11, 2020 at 7:48 AM kbuild test robot wrote:
>
> CC: kbuild-...@lists.01.org
> In-Reply-To: <20200111034347.5270-1-marma...@invisiblethingslab.com>
> References: <20200111034347.5270-1-marma...@invisiblethingslab.com>
> TO: "Marek Marczykowski-Górecki"
> CC: xen-devel@lists.xenproject.org, "Marek Marczykowski-Górecki"
> , Jan Beulich , Simon
> Gaiser , Boris Ostrovsky
> , Juergen Gross , Stefano
> Stabellini , YueHaibing , open
> list , "Marek Marczykowski-Górecki"
> , Jan Beulich , Simon
> Gaiser , Boris Ostrovsky
> , Juergen Gross , Stefano
> Stabellini , YueHaibing , open
> list
> CC: "Marek Marczykowski-Górecki" , Jan
> Beulich , Simon Gaiser ,
> Boris Ostrovsky , Juergen Gross
> , Stefano Stabellini , YueHaibing
> , open list
>
> Hi "Marek,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on xen-tip/linux-next]
> [also build test WARNING on linux/master linus/master v5.5-rc5 next-20200110]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see
> https://stackoverflow.com/a/37406982]
>
> url:
> https://github.com/0day-ci/linux/commits/Marek-Marczykowski-G-recki/xen-pciback-optionally-allow-interrupt-enable-flag-writes/20200111-162243
> base: https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git linux-next
> config: x86_64-allyesconfig (attached as .config)
> compiler: clang version 10.0.0 (git://gitmirror/llvm_project
> 016bf03ef6fcd9dce43b0c17971f76323f07a684)
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/xen/xen-pciback/conf_space_header.c:121:19: warning: variable
> >> 'val' is uninitialized when used here [-Wuninitialized]
>if ((cmd->val ^ val) & PCI_COMMAND_INTX_DISABLE) {
>^~~
>drivers/xen/xen-pciback/conf_space_header.c:65:9: note: initialize the
> variable 'val' to silence this warning
>u16 val;
> ^
>= 0
>1 warning generated.
>
> vim +/val +121 drivers/xen/xen-pciback/conf_space_header.c
>
> 60
> 61 static int command_write(struct pci_dev *dev, int offset, u16 value,
> void *data)
> 62 {
> 63 struct xen_pcibk_dev_data *dev_data;
> 64 int err;
> 65 u16 val;
> 66 struct pci_cmd_info *cmd = data;
> 67
> 68 dev_data = pci_get_drvdata(dev);
> 69 if (!pci_is_enabled(dev) && is_enable_cmd(value)) {
> 70 if (unlikely(verbose_request))
> 71 printk(KERN_DEBUG DRV_NAME ": %s: enable\n",
> 72 pci_name(dev));
> 73 err = pci_enable_device(dev);
> 74 if (err)
> 75 return err;
> 76 if (dev_data)
> 77 dev_data->enable_intx = 1;
> 78 } else if (pci_is_enabled(dev) && !is_enable_cmd(value)) {
> 79 if (unlikely(verbose_request))
> 80 printk(KERN_DEBUG DRV_NAME ": %s: disable\n",
> 81 pci_name(dev));
> 82 pci_disable_device(dev);
> 83 if (dev_data)
> 84 dev_data->enable_intx = 0;
> 85 }
> 86
> 87 if (!dev->is_busmaster && is_master_cmd(value)) {
> 88 if (unlikely(verbose_request))
> 89 printk(KERN_DEBUG DRV_NAME ": %s: set bus
> master\n",
> 90 pci_name(dev));
> 91 pci_set_master(dev);
> 92 } else if (dev->is_busmaster && !is_master_cmd(value)) {
> 93 if (unlikely(verbose_request))
> 94 printk(KERN_DEBUG DRV_NAME ": %s: clear bus
> master\n",
> 95 pci_name(dev));
> 96 pci_clear_master(dev);
> 97 }
> 98
> 99 if (!(cmd->val & PCI_COMMAND_INVALIDATE) &&
>100 (value & PCI_COMMAND_INVALIDATE)) {
>101 if (unlikely(verbose_request))
>102 printk(KERN_DEBUG
>103 DRV_NAME ": %s: enable
> memory-write-invalidate\n",
>104 pci_name(dev));
>105 err = pci_set_mwi(dev);
>106 if (err) {
>107
[Xen-devel] [PATCH v3] xen-pciback: optionally allow interrupt enable flag writes
QEMU running in a stubdom needs to be able to set INTX_DISABLE, and the
MSI(-X) enable flags in the PCI config space. This adds an attribute
'allow_interrupt_control' which when set for a PCI device allows writes
to this flag(s). The toolstack will need to set this for stubdoms.
When enabled, guest (stubdomain) will be allowed to set relevant enable
flags, but only one at a time - i.e. it refuses to enable more than one
of INTx, MSI, MSI-X at a time.
This functionality is needed only for config space access done by device
model (stubdomain) serving a HVM with the actual PCI device. It is not
necessary and unsafe to enable direct access to those bits for PV domain
with the device attached. For PV domains, there are separate protocol
messages (XEN_PCI_OP_{enable,disable}_{msi,msix}) for this purpose.
Those ops in addition to setting enable bits, also configure MSI(-X) in
dom0 kernel - which is undesirable for PCI passthrough to HVM guests.
This should not introduce any new security issues since a malicious
guest (or stubdom) can already generate MSIs through other ways, see
[1] page 8. Additionally, when qemu runs in dom0, it already have direct
access to those bits.
This is the second iteration of this feature. First was proposed as a
direct Xen interface through a new hypercall, but ultimately it was
rejected by the maintainer, because of mixing pciback and hypercalls for
PCI config space access isn't a good design. Full discussion at [2].
[1]:
https://invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
[2]: https://xen.markmail.org/thread/smpgpws4umdzizze
[part of the commit message and sysfs handling]
Signed-off-by: Simon Gaiser
[the rest]
Signed-off-by: Marek Marczykowski-Górecki
---
Changes in v3:
- return bitmap (or negative error) from
xen_pcibk_get_interrupt_type(), to implicitly handle cases when
multiple interrupt types are already enabled - disallow enabling in
that case
- add documentation
Changes in v2:
- introduce xen_pcibk_get_interrupt_type() to deduplicate current
INTx/MSI/MSI-X state check
- fix checking MSI/MSI-X state on devices not supporting it
---
.../ABI/testing/sysfs-driver-pciback | 13 +++
drivers/xen/xen-pciback/conf_space.c | 36
drivers/xen/xen-pciback/conf_space.h | 7 ++
.../xen/xen-pciback/conf_space_capability.c | 88 +++
drivers/xen/xen-pciback/conf_space_header.c | 18
drivers/xen/xen-pciback/pci_stub.c| 66 ++
drivers/xen/xen-pciback/pciback.h | 1 +
7 files changed, 229 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-driver-pciback
b/Documentation/ABI/testing/sysfs-driver-pciback
index 6a733bfa37e6..566a11f2c12f 100644
--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,16 @@ Description:
#echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
will allow the guest to read and write to the configuration
register 0x0E.
+
+What: /sys/bus/pci/drivers/pciback/allow_interrupt_control
+Date: Jan 2020
+KernelVersion: 5.5
+Contact:xen-devel@lists.xenproject.org
+Description:
+List of devices which can have interrupt control flag (INTx,
+MSI, MSI-X) set by a connected guest. It is meant to be set
+only when the guest is a stubdomain hosting device model (qemu)
+and the actual device is assigned to a HVM. It is not safe
+(similar to permissive attribute) to set for a devices assigned
+to a PV guest. The device is automatically removed from this
+list when the connected pcifront terminates.
diff --git a/drivers/xen/xen-pciback/conf_space.c
b/drivers/xen/xen-pciback/conf_space.c
index 60111719b01f..7697001e8ffc 100644
--- a/drivers/xen/xen-pciback/conf_space.c
+++ b/drivers/xen/xen-pciback/conf_space.c
@@ -286,6 +286,42 @@ int xen_pcibk_config_write(struct pci_dev *dev, int
offset, int size, u32 value)
return xen_pcibios_err_to_errno(err);
}
+int xen_pcibk_get_interrupt_type(struct pci_dev *dev)
+{
+ int err;
+ u16 val;
+ int ret = 0;
+
+ err = pci_read_config_word(dev, PCI_COMMAND, );
+ if (err)
+ return err;
+ if (!(val & PCI_COMMAND_INTX_DISABLE))
+ ret |= INTERRUPT_TYPE_INTX;
+
+ /* Do not trust dev->msi(x)_enabled here, as enabling could be done
+* bypassing the pci_*msi* functions, by the qemu.
+*/
+ if (dev->msi_cap) {
+ err = pci_read_config_word(dev,
+ dev->msi_cap + PCI_MSI_FLAGS,
+ );
+ if (err)
+ return err;
+ if (val & PCI_MSI_FLAGS_ENABLE)
+ ret |= INTERRUPT_TYPE_MSI;
+ }
+ if
