From: "Michael S. Tsirkin" <mst@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, jan.kiszka@siemens.com, shashidhar.patil@gmail.com
Subject: Re: [PATCH 4/9] pci-assign: Use struct for MSI-X table
Date: Tue, 31 Jan 2012 23:24:16 +0200 [thread overview]
Message-ID: <20120131212414.GD6843@redhat.com> (raw)
In-Reply-To: <1328044650.6937.155.camel@bling.home>
On Tue, Jan 31, 2012 at 02:17:30PM -0700, Alex Williamson wrote:
> On Tue, 2012-01-31 at 22:00 +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 31, 2012 at 12:05:49PM -0700, Alex Williamson wrote:
> > > On Tue, 2012-01-31 at 19:40 +0200, Michael S. Tsirkin wrote:
> > > > On Sat, Jan 28, 2012 at 07:21:58AM -0700, Alex Williamson wrote:
> > > > > This makes access much easier.
> > > > >
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > >
> > > > Yes but this also makes it easier to forget
> > > > to handle endian-ness.
> > > > How about using pci_get/pci_set instead?
> > >
> > > Do we really think existing device assignment is ever going to work on
> > > anything but x86/x86?
> > >
> > > Alex
> >
> > It's a question of whether anyone bothers to
> > fix all portability bugs. Besides endian-ness,
> > and msix vector decoding, there's not much that is
> > x86 specific there.
>
> The way interrupt routing is updated from the chipset is disgustingly
> x86 specific.
Right, that's what I mean - the decoding of msix vector to destination.
> The entire kernel side implementation is only enabled for
> x86 (well, not counting ia64). I expect the kernel side interrupt
> injection is very x86, or at least ioapic, specific. Beyond that, yeah,
> it may be trivial ;) Thanks,
>
> Alex
>
> > > > > ---
> > > > >
> > > > > hw/device-assignment.c | 55 ++++++++++++++++++++++--------------------------
> > > > > hw/device-assignment.h | 9 +++++++-
> > > > > 2 files changed, 33 insertions(+), 31 deletions(-)
> > > > >
> > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > > > > index 1fc7ffa..422ee00 100644
> > > > > --- a/hw/device-assignment.c
> > > > > +++ b/hw/device-assignment.c
> > > > > @@ -973,10 +973,9 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > > AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
> > > > > uint16_t entries_nr = 0, entries_max_nr;
> > > > > int pos = 0, i, r = 0;
> > > > > - uint32_t msg_addr, msg_upper_addr, msg_data, msg_ctrl;
> > > > > struct kvm_assigned_msix_nr msix_nr;
> > > > > struct kvm_assigned_msix_entry msix_entry;
> > > > > - void *va = adev->msix_table_page;
> > > > > + MSIXTableEntry *entry = adev->msix_table;
> > > > >
> > > > > pos = pci_find_capability(pci_dev, PCI_CAP_ID_MSIX);
> > > > >
> > > > > @@ -985,12 +984,11 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > > entries_max_nr += 1;
> > > > >
> > > > > /* Get the usable entry number for allocating */
> > > > > - for (i = 0; i < entries_max_nr; i++) {
> > > > > - memcpy(&msg_ctrl, va + i * 16 + 12, 4);
> > > > > - memcpy(&msg_data, va + i * 16 + 8, 4);
> > > > > + for (i = 0; i < entries_max_nr; i++, entry++) {
> > > > > /* Ignore unused entry even it's unmasked */
> > > > > - if (msg_data == 0)
> > > > > + if (entry->data == 0) {
> > > > > continue;
> > > > > + }
> > > > > entries_nr ++;
> > > > > }
> > > > >
> > > > > @@ -1013,16 +1011,13 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > >
> > > > > msix_entry.assigned_dev_id = msix_nr.assigned_dev_id;
> > > > > entries_nr = 0;
> > > > > - for (i = 0; i < entries_max_nr; i++) {
> > > > > + entry = adev->msix_table;
> > > > > + for (i = 0; i < entries_max_nr; i++, entry++) {
> > > > > if (entries_nr >= msix_nr.entry_nr)
> > > > > break;
> > > > > - memcpy(&msg_ctrl, va + i * 16 + 12, 4);
> > > > > - memcpy(&msg_data, va + i * 16 + 8, 4);
> > > > > - if (msg_data == 0)
> > > > > + if (entry->data == 0) {
> > > > > continue;
> > > > > -
> > > > > - memcpy(&msg_addr, va + i * 16, 4);
> > > > > - memcpy(&msg_upper_addr, va + i * 16 + 4, 4);
> > > > > + }
> > > > >
> > > > > r = kvm_get_irq_route_gsi();
> > > > > if (r < 0)
> > > > > @@ -1031,10 +1026,11 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev)
> > > > > adev->entry[entries_nr].gsi = r;
> > > > > adev->entry[entries_nr].type = KVM_IRQ_ROUTING_MSI;
> > > > > adev->entry[entries_nr].flags = 0;
> > > > > - adev->entry[entries_nr].u.msi.address_lo = msg_addr;
> > > > > - adev->entry[entries_nr].u.msi.address_hi = msg_upper_addr;
> > > > > - adev->entry[entries_nr].u.msi.data = msg_data;
> > > > > - DEBUG("MSI-X data 0x%x, MSI-X addr_lo 0x%x\n!", msg_data, msg_addr);
> > > > > + adev->entry[entries_nr].u.msi.address_lo = entry->addr_lo;
> > > > > + adev->entry[entries_nr].u.msi.address_hi = entry->addr_hi;
> > > > > + adev->entry[entries_nr].u.msi.data = entry->data;
> > > > > + DEBUG("MSI-X data 0x%x, MSI-X addr_lo 0x%x\n!",
> > > > > + entry->data, entry->addr_lo);
> > > > > kvm_add_routing_entry(&adev->entry[entries_nr]);
> > > > >
> > > > > msix_entry.gsi = adev->entry[entries_nr].gsi;
> > > > > @@ -1443,7 +1439,7 @@ static uint64_t msix_mmio_read(void *opaque, target_phys_addr_t addr,
> > > > > AssignedDevice *adev = opaque;
> > > > > uint64_t val;
> > > > >
> > > > > - memcpy(&val, (void *)((uint8_t *)adev->msix_table_page + addr), size);
> > > > > + memcpy(&val, (void *)((uint8_t *)adev->msix_table + addr), size);
> > > > >
> > > > > return val;
> > > > > }
> > > > > @@ -1456,7 +1452,7 @@ static void msix_mmio_write(void *opaque, target_phys_addr_t addr,
> > > > > DEBUG("write to MSI-X entry table mmio offset 0x%lx, val 0x%lx\n",
> > > > > addr, val);
> > > > >
> > > > > - memcpy((void *)((uint8_t *)adev->msix_table_page + addr), &val, size);
> > > > > + memcpy((void *)((uint8_t *)adev->msix_table + addr), &val, size);
> > > > > }
> > > > >
> > > > > static const MemoryRegionOps msix_mmio_ops = {
> > > > > @@ -1471,15 +1467,13 @@ static const MemoryRegionOps msix_mmio_ops = {
> > > > >
> > > > > static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
> > > > > {
> > > > > - dev->msix_table_page = mmap(NULL, 0x1000,
> > > > > - PROT_READ|PROT_WRITE,
> > > > > - MAP_ANONYMOUS|MAP_PRIVATE, 0, 0);
> > > > > - if (dev->msix_table_page == MAP_FAILED) {
> > > > > - fprintf(stderr, "fail allocate msix_table_page! %s\n",
> > > > > - strerror(errno));
> > > > > + dev->msix_table = mmap(NULL, 0x1000, PROT_READ|PROT_WRITE,
> > > > > + MAP_ANONYMOUS|MAP_PRIVATE, 0, 0);
> > > > > + if (dev->msix_table == MAP_FAILED) {
> > > > > + fprintf(stderr, "fail allocate msix_table! %s\n", strerror(errno));
> > > > > return -EFAULT;
> > > > > }
> > > > > - memset(dev->msix_table_page, 0, 0x1000);
> > > > > + memset(dev->msix_table, 0, 0x1000);
> > > > > memory_region_init_io(&dev->mmio, &msix_mmio_ops, dev,
> > > > > "assigned-dev-msix", MSIX_PAGE_SIZE);
> > > > > return 0;
> > > > > @@ -1487,16 +1481,17 @@ static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
> > > > >
> > > > > static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
> > > > > {
> > > > > - if (!dev->msix_table_page)
> > > > > + if (!dev->msix_table) {
> > > > > return;
> > > > > + }
> > > > >
> > > > > memory_region_destroy(&dev->mmio);
> > > > >
> > > > > - if (munmap(dev->msix_table_page, 0x1000) == -1) {
> > > > > - fprintf(stderr, "error unmapping msix_table_page! %s\n",
> > > > > + if (munmap(dev->msix_table, 0x1000) == -1) {
> > > > > + fprintf(stderr, "error unmapping msix_table! %s\n",
> > > > > strerror(errno));
> > > > > }
> > > > > - dev->msix_table_page = NULL;
> > > > > + dev->msix_table = NULL;
> > > > > }
> > > > >
> > > > > static const VMStateDescription vmstate_assigned_device = {
> > > > > diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> > > > > index 8ee2274..e229303 100644
> > > > > --- a/hw/device-assignment.h
> > > > > +++ b/hw/device-assignment.h
> > > > > @@ -82,6 +82,13 @@ typedef struct {
> > > > > #define ASSIGNED_DEVICE_PREFER_MSI_MASK (1 << ASSIGNED_DEVICE_PREFER_MSI_BIT)
> > > > > #define ASSIGNED_DEVICE_USE_MEM64_MASK (1 << ASSIGNED_DEVICE_USE_MEM64_BIT)
> > > > >
> > > > > +typedef struct {
> > > > > + uint32_t addr_lo;
> > > > > + uint32_t addr_hi;
> > > > > + uint32_t data;
> > > > > + uint32_t ctrl;
> > > > > +} MSIXTableEntry;
> > > > > +
> > > > > typedef struct AssignedDevice {
> > > > > PCIDevice dev;
> > > > > PCIHostDevice host;
> > > > > @@ -110,7 +117,7 @@ typedef struct AssignedDevice {
> > > > > uint8_t emulate_config_write[PCI_CONFIG_SPACE_SIZE];
> > > > > int irq_entries_nr;
> > > > > struct kvm_irq_routing_entry *entry;
> > > > > - void *msix_table_page;
> > > > > + MSIXTableEntry *msix_table;
> > > > > target_phys_addr_t msix_table_addr;
> > > > > MemoryRegion mmio;
> > > > > char *configfd_name;
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> > >
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
next prev parent reply other threads:[~2012-01-31 21:24 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-01-28 14:21 [PATCH 0/9] pci-assign: 64bit MMIO + better MSI-X table support Alex Williamson
2012-01-28 14:21 ` [PATCH 1/9] pci-assign: Optionally enable 64bit BARs in guest Alex Williamson
2012-01-31 12:40 ` Avi Kivity
2012-01-31 12:45 ` Jan Kiszka
2012-01-31 12:51 ` Avi Kivity
2012-01-31 12:57 ` Jan Kiszka
2012-01-31 13:10 ` Avi Kivity
2012-01-31 13:21 ` Jan Kiszka
2012-01-31 13:33 ` Avi Kivity
2012-01-31 21:08 ` Alex Williamson
2012-01-31 21:14 ` Michael S. Tsirkin
2012-02-01 9:03 ` Avi Kivity
2012-02-01 10:03 ` Michael S. Tsirkin
2012-02-01 13:55 ` Alex Williamson
2012-02-01 15:18 ` Michael S. Tsirkin
2012-02-01 15:24 ` Alex Williamson
2012-01-28 14:21 ` [PATCH 2/9] pci-assign: Fix warnings with DEBUG enabled Alex Williamson
2012-01-28 14:21 ` [PATCH 3/9] pci-assign: Update MSI-X MMIO to Memory API Alex Williamson
2012-01-31 12:45 ` Avi Kivity
2012-01-31 21:13 ` Alex Williamson
2012-02-01 4:22 ` Alex Williamson
2012-02-01 9:04 ` Avi Kivity
2012-02-01 13:56 ` Alex Williamson
2012-01-28 14:21 ` [PATCH 4/9] pci-assign: Use struct for MSI-X table Alex Williamson
2012-01-31 17:40 ` Michael S. Tsirkin
2012-01-31 19:05 ` Alex Williamson
2012-01-31 20:00 ` Michael S. Tsirkin
2012-01-31 21:17 ` Alex Williamson
2012-01-31 21:24 ` Michael S. Tsirkin [this message]
2012-01-31 21:30 ` Alex Williamson
2012-01-28 14:22 ` [PATCH 5/9] pci-assign: Only calculate maximum MSI-X vector entries once Alex Williamson
2012-01-31 20:18 ` Michael S. Tsirkin
2012-01-31 20:31 ` Alex Williamson
2012-01-31 20:56 ` Michael S. Tsirkin
2012-01-28 14:22 ` [PATCH 6/9] pci-assign: Proper initialization for MSI-X table Alex Williamson
2012-01-31 17:40 ` Michael S. Tsirkin
2012-01-31 19:07 ` Alex Williamson
2012-01-31 19:12 ` Michael S. Tsirkin
2012-01-31 19:16 ` Jan Kiszka
2012-01-31 20:19 ` Michael S. Tsirkin
2012-01-31 21:06 ` Alex Williamson
2012-01-28 14:22 ` [PATCH 7/9] pci-assign: Allocate entries for all MSI-X vectors Alex Williamson
2012-01-28 14:22 ` [PATCH 8/9] pci-assign: Use MSIX_PAGE_SIZE Alex Williamson
2012-01-28 14:22 ` [PATCH 9/9] pci-assign: Update MSI-X config based on table writes Alex Williamson
2012-01-31 12:50 ` Avi Kivity
2012-01-30 10:11 ` [PATCH 0/9] pci-assign: 64bit MMIO + better MSI-X table support Jan Kiszka
2012-01-30 13:44 ` Alex Williamson
2012-01-31 12:52 ` Avi Kivity
2012-01-31 12:56 ` Jan Kiszka
2012-02-06 15:55 ` Shashidhar Patil
2012-02-06 17:29 ` Alex Williamson
2012-02-09 16:23 ` Shashidhar Patil
2012-02-09 17:23 ` Alex Williamson
2012-02-12 16:30 ` Shashidhar Patil
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=20120131212414.GD6843@redhat.com \
--to=mst@redhat.com \
--cc=alex.williamson@redhat.com \
--cc=jan.kiszka@siemens.com \
--cc=kvm@vger.kernel.org \
--cc=shashidhar.patil@gmail.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.