From: Alex Williamson <alex.williamson@redhat.com>
To: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Avi Kivity <avi@redhat.com>,
Marcelo Tosatti <mtosatti@redhat.com>,
"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH v3 4/6] pci-assign: Properly handle more overlapping accesses
Date: Mon, 02 May 2011 08:09:54 -0600 [thread overview]
Message-ID: <1304345394.3255.14.camel@x201> (raw)
In-Reply-To: <4DBE8793.3090505@siemens.com>
On Mon, 2011-05-02 at 12:29 +0200, Jan Kiszka wrote:
> On 2011-04-29 22:41, Alex Williamson wrote:
> > On Fri, 2011-04-29 at 11:05 +0200, Jan Kiszka wrote:
> >> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
> >> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
> >> virtualize. Again, we do not optimize these checks and accesses a lot,
> >> they are considered to be slow paths.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >> hw/device-assignment.c | 34 +++++++++++++++++++++++++++++-----
> >> 1 files changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> >> index cea072e..37c77e3 100644
> >> --- a/hw/device-assignment.c
> >> +++ b/hw/device-assignment.c
> >> @@ -442,7 +442,29 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
> >> ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> >> /* used for update-mappings (BAR emulation) */
> >> pci_default_write_config(d, address, val, len);
> >> - return;
> >> +
> >> + /* Ensure that writes to overlapping areas we don't virtualize still
> >> + * hit the device. */
> >> + switch (address) {
> >> + case PCI_CAPABILITY_LIST:
> >> + if (len > 1) {
> >> + len -= 1;
> >> + address += 1;
> >> + val >>= 8;
> >> + break; /* continue writing to the device */
> >> + }
> >> + return;
> >> + case PCI_INTERRUPT_LINE:
> >> + if (len > 2) {
> >> + len -= 2;
> >> + address += 2;
> >> + val >>= 16;
> >> + break; /* continue writing to the device */
> >> + }
> >> + return;
> >> + default:
> >> + return;
> >> + }
> >> }
> >
> > It seems like we could be more symmetric with the below read. Maybe
> > something like:
> >
> > if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> > ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
> > pci_default_write_config(d, address, val, len);
> > return;
> > } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
> > ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> > uint32_t real_val = assigned_dev_pci_read(d, address, len);
> > pci_default_write_config(d, address, val, len);
> > val = merge_bits(val, real_val, address, len, PCI_CAPABILITY_LIST, 0xff);
> > val = merge_bits(val, real_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> > }
> >
> > We might write out to real hardware when we could avoid it, but I don't
> > think it matters. Thanks,
> >
>
> Yep, makes sense. Find such a version below.
>
> Thanks,
> Jan
>
>
> ------8<-------
>
> Ensure that accesses exceeding PCI_CAPABILITY_LIST and
> PCI_INTERRUPT_LINE+PIN hit the real device in areas we do not
> virtualize. Again, we do not optimize these checks and accesses a lot,
> they are considered to be slow paths.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> hw/device-assignment.c | 34 ++++++++++++++++++++++++++--------
> 1 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index cea072e..8e95730 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -438,11 +438,22 @@ static void assigned_dev_pci_write_config(PCIDevice *d, uint32_t address,
> * - interrupt line & pin
> */
> if (ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
> - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> - /* used for update-mappings (BAR emulation) */
> + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
> pci_default_write_config(d, address, val, len);
> return;
> + } else if (ranges_overlap(address, len, PCI_CAPABILITY_LIST, 1) ||
> + ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> + uint32_t real_val;
> +
> + pci_default_write_config(d, address, val, len);
> +
> + /* Ensure that writes to overlapping areas we don't virtualize still
> + * hit the device. */
> + real_val = assigned_dev_pci_read(d, address, len);
> + val = merge_bits(val, real_val, address, len,
> + PCI_CAPABILITY_LIST, 0xff);
> + val = merge_bits(val, real_val, address, len,
> + PCI_INTERRUPT_LINE, 0xffff);
> }
>
> DEBUG("NON BAR (%x.%x): address=%04x val=0x%08x len=%d\n",
> @@ -467,7 +478,7 @@ again:
> static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
> int len)
> {
> - uint32_t val = 0;
> + uint32_t val = 0, virt_val;
> int fd;
> ssize_t ret;
> AssignedDevice *pci_dev = DO_UPCAST(AssignedDevice, dev, d);
> @@ -483,13 +494,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
> * Catch access to
> * - vendor & device ID
> * - base address registers
> - * - ROM base address & capability pointer
> - * - interrupt line & pin
> + * - ROM base address
> */
> if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
> ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
> - ranges_overlap(address, len, PCI_ROM_ADDRESS, 5) ||
> - ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
> + ranges_overlap(address, len, PCI_ROM_ADDRESS, 4)) {
> val = pci_default_read_config(d, address, len);
> DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
> (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
> @@ -523,6 +532,15 @@ do_log:
> address, len, PCI_COMMAND, 0xffff);
> }
>
> + /*
> + * Merge bits from virtualized
> + * - capability pointer
> + * - interrupt line & pin
> + */
> + virt_val = pci_default_read_config(d, address, len);
> + val = merge_bits(val, virt_val, address, len, PCI_CAPABILITY_LIST, 0xff);
> + val = merge_bits(val, virt_val, address, len, PCI_INTERRUPT_LINE, 0xffff);
> +
> if (!pci_dev->cap.available) {
> /* kill the special capabilities */
> if (address == PCI_COMMAND && len == 4) {
next prev parent reply other threads:[~2011-05-02 14:09 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-29 9:05 [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 1/6] pci-assign: Clean up assigned_dev_pci_read/write_config Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 2/6] pci-assign: Move merge_bits Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 3/6] pci-assign: Fix dword read at PCI_COMMAND Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 4/6] pci-assign: Properly handle more overlapping accesses Jan Kiszka
2011-04-29 20:41 ` Alex Williamson
2011-05-02 10:29 ` [PATCH v3 " Jan Kiszka
2011-05-02 14:09 ` Alex Williamson [this message]
2011-04-29 9:05 ` [PATCH v2 5/6] pci-assign: Remove suspicious hunk from assigned_dev_pci_read_config Jan Kiszka
2011-04-29 9:05 ` [PATCH v2 6/6] pci-assign: Convert need_emulate_cmd into a bitmask Jan Kiszka
2011-05-02 14:11 ` [PATCH v2 0/6] qemu-kvm: pci-assign: Some more cleanups Alex Williamson
2011-05-03 19:17 ` Marcelo Tosatti
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=1304345394.3255.14.camel@x201 \
--to=alex.williamson@redhat.com \
--cc=avi@redhat.com \
--cc=jan.kiszka@siemens.com \
--cc=kvm@vger.kernel.org \
--cc=mtosatti@redhat.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.