From: Alex Williamson <alex.williamson@redhat.com>
To: Tom Lyon <pugs@cisco.com>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
Subject: Re: [PATCH] vfio: fix config virtualization, esp command byte
Date: Tue, 16 Nov 2010 10:54:21 -0700 [thread overview]
Message-ID: <1289930061.3069.5.camel@x201> (raw)
In-Reply-To: <4cd9f0d6.jnHBr6PBphKwDwhM%pugs@cisco.com>
On Tue, 2010-11-09 at 17:09 -0800, Tom Lyon wrote:
> Cleans up config space virtualization, especialy handling of bytes
> which have some virtual and some real bits, like PCI_COMMAND.
>
> Alex, I hope you can test this with your setups.
Sorry for the delay. FWIW, I'm not having much luck with this, I'll try
to debug the problem. Thanks,
Alex
> Signed-off-by: Tom Lyon <pugs@cisco.com>
> ---
> drivers/vfio/vfio_pci_config.c | 166 +++++++++++++---------------------------
> 1 files changed, 53 insertions(+), 113 deletions(-)
>
> diff --git a/drivers/vfio/vfio_pci_config.c b/drivers/vfio/vfio_pci_config.c
> index 8304316..7132ac4 100644
> --- a/drivers/vfio/vfio_pci_config.c
> +++ b/drivers/vfio/vfio_pci_config.c
> @@ -745,6 +745,8 @@ static int vfio_virt_init(struct vfio_dev *vdev)
> */
> static void vfio_bar_restore(struct vfio_dev *vdev)
> {
> + if (vdev->pdev->is_virtfn)
> + return;
> printk(KERN_WARNING "%s: reset recovery - restoring bars\n", __func__);
>
> #define do_bar(off, which) \
> @@ -815,26 +817,15 @@ static inline int vfio_read_config_byte(struct vfio_dev *vdev,
> static inline int vfio_write_config_byte(struct vfio_dev *vdev,
> int pos, u8 val)
> {
> - vdev->vconfig[pos] = val;
> return pci_user_write_config_byte(vdev->pdev, pos, val);
> }
>
> /* handle virtualized fields in the basic config space */
> -static u8 vfio_virt_basic(struct vfio_dev *vdev, int write,
> - u16 pos, u16 off, u8 val, u8 newval)
> +static void vfio_virt_basic(struct vfio_dev *vdev, int write, u16 pos, u8 *rbp)
> {
> - switch (off) {
> - /*
> - * vendor and device are virt because they don't
> - * show up otherwise for sr-iov vfs
> - */
> - case PCI_VENDOR_ID:
> - case PCI_VENDOR_ID + 1:
> - case PCI_DEVICE_ID:
> - case PCI_DEVICE_ID + 1:
> - /* read only */
> - val = vdev->vconfig[pos];
> - break;
> + u8 val;
> +
> + switch (pos) {
> case PCI_COMMAND:
> /*
> * If the real mem or IO enable bits are zero
> @@ -842,100 +833,58 @@ static u8 vfio_virt_basic(struct vfio_dev *vdev, int write,
> * Restore the real BARs before allowing those
> * bits to re-enable
> */
> + val = vdev->vconfig[pos];
> if (vdev->pdev->is_virtfn)
> val |= PCI_COMMAND_MEMORY;
> if (write) {
> - int upd = 0;
> -
> - upd = (newval & PCI_COMMAND_MEMORY) >
> - (val & PCI_COMMAND_MEMORY);
> - upd += (newval & PCI_COMMAND_IO) >
> - (val & PCI_COMMAND_IO);
> - if (upd)
> - vfio_bar_restore(vdev);
> - vfio_write_config_byte(vdev, pos, newval);
> - }
> - break;
> - case PCI_BASE_ADDRESS_0:
> - case PCI_BASE_ADDRESS_0+1:
> - case PCI_BASE_ADDRESS_0+2:
> - case PCI_BASE_ADDRESS_0+3:
> - case PCI_BASE_ADDRESS_1:
> - case PCI_BASE_ADDRESS_1+1:
> - case PCI_BASE_ADDRESS_1+2:
> - case PCI_BASE_ADDRESS_1+3:
> - case PCI_BASE_ADDRESS_2:
> - case PCI_BASE_ADDRESS_2+1:
> - case PCI_BASE_ADDRESS_2+2:
> - case PCI_BASE_ADDRESS_2+3:
> - case PCI_BASE_ADDRESS_3:
> - case PCI_BASE_ADDRESS_3+1:
> - case PCI_BASE_ADDRESS_3+2:
> - case PCI_BASE_ADDRESS_3+3:
> - case PCI_BASE_ADDRESS_4:
> - case PCI_BASE_ADDRESS_4+1:
> - case PCI_BASE_ADDRESS_4+2:
> - case PCI_BASE_ADDRESS_4+3:
> - case PCI_BASE_ADDRESS_5:
> - case PCI_BASE_ADDRESS_5+1:
> - case PCI_BASE_ADDRESS_5+2:
> - case PCI_BASE_ADDRESS_5+3:
> - case PCI_ROM_ADDRESS:
> - case PCI_ROM_ADDRESS+1:
> - case PCI_ROM_ADDRESS+2:
> - case PCI_ROM_ADDRESS+3:
> - if (write) {
> - vdev->vconfig[pos] = newval;
> - vdev->bardirty = 1;
> - } else {
> - if (vdev->bardirty)
> - vfio_bar_fixup(vdev);
> - val = vdev->vconfig[pos];
> +
> + if (((val & PCI_COMMAND_MEMORY) >
> + (*rbp & PCI_COMMAND_MEMORY)) ||
> + ((val & PCI_COMMAND_IO) >
> + (*rbp & PCI_COMMAND_IO)))
> + vfio_bar_restore(vdev);
> + *rbp &= ~(PCI_COMMAND_MEMORY + PCI_COMMAND_IO);
> + *rbp |= val & (PCI_COMMAND_MEMORY + PCI_COMMAND_IO);
> }
> + vdev->vconfig[pos] = val;
> break;
> - default:
> + case PCI_BASE_ADDRESS_0 ... PCI_BASE_ADDRESS_5 + 3:
> + case PCI_ROM_ADDRESS ... PCI_ROM_ADDRESS + 3:
> if (write)
> - vdev->vconfig[pos] = newval;
> - else
> - val = vdev->vconfig[pos];
> + vdev->bardirty = 1;
> + else if (vdev->bardirty)
> + vfio_bar_fixup(vdev);
> break;
> }
> - return val;
> }
>
> /*
> * handle virtualized fields in msi capability
> * easy, except for multiple-msi fields in flags byte
> */
> -static u8 vfio_virt_msi(struct vfio_dev *vdev, int write,
> - u16 pos, u16 off, u8 val, u8 newval)
> +static void vfio_virt_msi(struct vfio_dev *vdev, int write,
> + u16 pos, u16 off, u8 *rbp)
> {
> - if (off == PCI_MSI_FLAGS) {
> - u8 num;
> + u8 val;
> + u8 num;
>
> + val = vdev->vconfig[pos];
> + if (off == PCI_MSI_FLAGS) {
> if (write) {
> if (!vdev->ev_msi)
> - newval &= ~PCI_MSI_FLAGS_ENABLE;
> - num = (newval & PCI_MSI_FLAGS_QSIZE) >> 4;
> + val &= ~PCI_MSI_FLAGS_ENABLE;
> + num = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
> if (num > vdev->msi_qmax)
> num = vdev->msi_qmax;
> - newval &= ~PCI_MSI_FLAGS_QSIZE;
> - newval |= num << 4;
> - vfio_write_config_byte(vdev, pos, newval);
> + val &= ~PCI_MSI_FLAGS_QSIZE;
> + val |= num << 4;
> + *rbp = val;
> } else {
> - if (vfio_read_config_byte(vdev, pos, &val) < 0)
> - return 0;
> val &= ~PCI_MSI_FLAGS_QMASK;
> val |= vdev->msi_qmax << 1;
> }
> - return val;
> }
> -
> - if (write)
> - vdev->vconfig[pos] = newval;
> - else
> - val = vdev->vconfig[pos];
> - return val;
> + vdev->vconfig[pos] = val;
> }
>
> static int vfio_config_rwbyte(int write,
> @@ -950,6 +899,7 @@ static int vfio_config_rwbyte(int write,
> struct perm_bits *perm;
> u8 wr, virt;
> int ret;
> + u8 realbits = 0;
>
> cap = map[pos];
> if (cap == 0xFF) { /* unknown region */
> @@ -989,7 +939,7 @@ static int vfio_config_rwbyte(int write,
> }
> if (write && !wr) /* no writeable bits */
> return 0;
> - if (!virt) {
> + if (!virt) { /* no virtual bits */
> if (write) {
> if (copy_from_user(&val, buf, 1))
> return -EFAULT;
> @@ -1018,54 +968,44 @@ static int vfio_config_rwbyte(int write,
> if (copy_from_user(&newval, buf, 1))
> return -EFAULT;
> }
> - /*
> - * We get here if there are some virt bits
> - * handle remaining real bits, if any
> - */
> - if (~virt) {
> - u8 rbits = (~virt) & wr;
>
> - ret = vfio_read_config_byte(vdev, pos, &val);
> + if (~virt) { /* mix of real and virt bits */
> + /* update vconfig with latest hw bits */
> + ret = vfio_read_config_byte(vdev, pos, &realbits);
> if (ret < 0)
> return ret;
> - if (write && rbits) {
> - val &= ~rbits;
> - val |= (newval & rbits);
> - vfio_write_config_byte(vdev, pos, val);
> - }
> + vdev->vconfig[pos] =
> + (vdev->vconfig[pos] & virt) | (realbits & ~virt);
> }
> +
> + /* update vconfig with writeable bits */
> + vdev->vconfig[pos] =
> + (vdev->vconfig[pos] & ~wr) | (newval & wr);
> +
> /*
> - * Now handle entirely virtual fields
> + * Now massage virtual fields
> */
> if (pos < PCI_CFG_SPACE_SIZE) {
> switch (cap) {
> case PCI_CAP_ID_BASIC: /* virtualize BARs */
> - val = vfio_virt_basic(vdev, write,
> - pos, off, val, newval);
> + vfio_virt_basic(vdev, write, pos, &realbits);
> break;
> case PCI_CAP_ID_MSI: /* virtualize (parts of) MSI */
> - val = vfio_virt_msi(vdev, write,
> - pos, off, val, newval);
> - break;
> - default:
> - if (write)
> - vdev->vconfig[pos] = newval;
> - else
> - val = vdev->vconfig[pos];
> + vfio_virt_msi(vdev, write, pos, off, &realbits);
> break;
> }
> } else {
> /* no virt fields yet in ecaps */
> switch (cap) { /* extended capabilities */
> default:
> - if (write)
> - vdev->vconfig[pos] = newval;
> - else
> - val = vdev->vconfig[pos];
> break;
> }
> }
> - if (!write && copy_to_user(buf, &val, 1))
> + if (write && ~virt) {
> + realbits = (realbits & virt) | (vdev->vconfig[pos] & ~virt);
> + vfio_write_config_byte(vdev, pos, realbits);
> + }
> + if (!write && copy_to_user(buf, &vdev->vconfig[pos], 1))
> return -EFAULT;
> return 0;
> }
next prev parent reply other threads:[~2010-11-16 18:32 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-10 1:09 [PATCH] vfio: fix config virtualization, esp command byte Tom Lyon
2010-11-16 17:54 ` Alex Williamson [this message]
2010-11-16 18:57 ` Alex Williamson
2010-11-17 5:14 ` Tom Lyon
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=1289930061.3069.5.camel@x201 \
--to=alex.williamson@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=pugs@cisco.com \
/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.