Re: [PATCH] pci-assign: Fix multifunction support

[Marcelo Tosatti]

On Mon, Jan 16, 2012 at 10:11:51AM -0700, Alex Williamson wrote:
 The core PCI code sets the multifunction bit in the header before
 calling the device initfn.  For device assignment, we're blasting
 that value with the actual hardware value, so nobody sees the
 additional functions if the devices isn't physically multifunction.
 Switch the HEADER_TYPE to a fully emulated field (all read-only
 anyway) and add setting and clearing of the multifunction bit to
 match qemu directive.
 
 Signed-off-by: Alex Williamson alex.william...@redhat.com

Applied, thanks.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci-assign: Fix multifunction support

[Jan Kiszka]

On 2012-01-16 18:11, Alex Williamson wrote:
 The core PCI code sets the multifunction bit in the header before
 calling the device initfn.  For device assignment, we're blasting
 that value with the actual hardware value, so nobody sees the
 additional functions if the devices isn't physically multifunction.
 Switch the HEADER_TYPE to a fully emulated field (all read-only
 anyway) and add setting and clearing of the multifunction bit to
 match qemu directive.
 
 Signed-off-by: Alex Williamson alex.william...@redhat.com
 ---
 
  hw/device-assignment.c |8 +++-
  1 files changed, 7 insertions(+), 1 deletions(-)
 
 diff --git a/hw/device-assignment.c b/hw/device-assignment.c
 index 2a9e66d..7f4a5ec 100644
 --- a/hw/device-assignment.c
 +++ b/hw/device-assignment.c
 @@ -540,6 +540,13 @@ again:
  fprintf(stderr, %s: read failed, errno = %d\n, __func__, errno);
  }
  
 +/* Restore or clear multifunction, this is always controlled by qemu */
 +if (pci_dev-dev.cap_present  QEMU_PCI_CAP_MULTIFUNCTION) {
 +pci_dev-dev.config[PCI_HEADER_TYPE] |= 
 PCI_HEADER_TYPE_MULTI_FUNCTION;
 +} else {
 +pci_dev-dev.config[PCI_HEADER_TYPE] = 
 ~PCI_HEADER_TYPE_MULTI_FUNCTION;
 +}
 +

Why have this in get_*real*_device? Why not fix this up at the caller
site, i.e. in assigned_initfn? Just for consistency, not a functional issue.

  /* Clear host resource mapping info.  If we choose not to register a
   * BAR, such as might be the case with the option ROM, we can get
   * confusing, unwritable, residual addresses from the host here. */
 @@ -1575,7 +1582,6 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
  assigned_dev_direct_config_read(dev, PCI_CLASS_PROG, 3);
  assigned_dev_direct_config_read(dev, PCI_CACHE_LINE_SIZE, 1);
  assigned_dev_direct_config_read(dev, PCI_LATENCY_TIMER, 1);
 -assigned_dev_direct_config_read(dev, PCI_HEADER_TYPE, 1);
  assigned_dev_direct_config_read(dev, PCI_BIST, 1);
  assigned_dev_direct_config_read(dev, PCI_CARDBUS_CIS, 4);
  assigned_dev_direct_config_read(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);
 

Looks good otherwise. Is it a regression of the access control refactoring?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pci-assign: Fix multifunction support

[Alex Williamson]

- Original Message -
 On 2012-01-16 18:11, Alex Williamson wrote:
  The core PCI code sets the multifunction bit in the header before
  calling the device initfn.  For device assignment, we're blasting
  that value with the actual hardware value, so nobody sees the
  additional functions if the devices isn't physically multifunction.
  Switch the HEADER_TYPE to a fully emulated field (all read-only
  anyway) and add setting and clearing of the multifunction bit to
  match qemu directive.
  
  Signed-off-by: Alex Williamson alex.william...@redhat.com
  ---
  
   hw/device-assignment.c |8 +++-
   1 files changed, 7 insertions(+), 1 deletions(-)
  
  diff --git a/hw/device-assignment.c b/hw/device-assignment.c
  index 2a9e66d..7f4a5ec 100644
  --- a/hw/device-assignment.c
  +++ b/hw/device-assignment.c
  @@ -540,6 +540,13 @@ again:
   fprintf(stderr, %s: read failed, errno = %d\n, __func__,
   errno);
   }
   
  +/* Restore or clear multifunction, this is always controlled
  by qemu */
  +if (pci_dev-dev.cap_present  QEMU_PCI_CAP_MULTIFUNCTION) {
  +pci_dev-dev.config[PCI_HEADER_TYPE] |=
  PCI_HEADER_TYPE_MULTI_FUNCTION;
  +} else {
  +pci_dev-dev.config[PCI_HEADER_TYPE] =
  ~PCI_HEADER_TYPE_MULTI_FUNCTION;
  +}
  +
 
 Why have this in get_*real*_device? Why not fix this up at the caller
 site, i.e. in assigned_initfn? Just for consistency, not a functional
 issue.

I chose here because we've just overwritten the emulated config space and we 
then proceed to clean out the BAR registers.  As this is close to the point 
where it gets trashed and we're doing other fixup, it seems appropriate.

   /* Clear host resource mapping info.  If we choose not to
   register a
* BAR, such as might be the case with the option ROM, we can
get
* confusing, unwritable, residual addresses from the host
here. */
  @@ -1575,7 +1582,6 @@ static int assigned_initfn(struct PCIDevice
  *pci_dev)
   assigned_dev_direct_config_read(dev, PCI_CLASS_PROG, 3);
   assigned_dev_direct_config_read(dev, PCI_CACHE_LINE_SIZE, 1);
   assigned_dev_direct_config_read(dev, PCI_LATENCY_TIMER, 1);
  -assigned_dev_direct_config_read(dev, PCI_HEADER_TYPE, 1);
   assigned_dev_direct_config_read(dev, PCI_BIST, 1);
   assigned_dev_direct_config_read(dev, PCI_CARDBUS_CIS, 4);
   assigned_dev_direct_config_read(dev, PCI_SUBSYSTEM_VENDOR_ID,
   2);
  
 
 Looks good otherwise. Is it a regression of the access control
 refactoring?

I believe it's been a latent issue since before the refactoring.  Thanks,

Alex
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pci-assign: Fix multifunction support

[Alex Williamson]

The core PCI code sets the multifunction bit in the header before
calling the device initfn.  For device assignment, we're blasting
that value with the actual hardware value, so nobody sees the
additional functions if the devices isn't physically multifunction.
Switch the HEADER_TYPE to a fully emulated field (all read-only
anyway) and add setting and clearing of the multifunction bit to
match qemu directive.

Signed-off-by: Alex Williamson alex.william...@redhat.com
---

 hw/device-assignment.c |8 +++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 2a9e66d..7f4a5ec 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -540,6 +540,13 @@ again:
 fprintf(stderr, %s: read failed, errno = %d\n, __func__, errno);
 }
 
+/* Restore or clear multifunction, this is always controlled by qemu */
+if (pci_dev-dev.cap_present  QEMU_PCI_CAP_MULTIFUNCTION) {
+pci_dev-dev.config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
+} else {
+pci_dev-dev.config[PCI_HEADER_TYPE] = 
~PCI_HEADER_TYPE_MULTI_FUNCTION;
+}
+
 /* Clear host resource mapping info.  If we choose not to register a
  * BAR, such as might be the case with the option ROM, we can get
  * confusing, unwritable, residual addresses from the host here. */
@@ -1575,7 +1582,6 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
 assigned_dev_direct_config_read(dev, PCI_CLASS_PROG, 3);
 assigned_dev_direct_config_read(dev, PCI_CACHE_LINE_SIZE, 1);
 assigned_dev_direct_config_read(dev, PCI_LATENCY_TIMER, 1);
-assigned_dev_direct_config_read(dev, PCI_HEADER_TYPE, 1);
 assigned_dev_direct_config_read(dev, PCI_BIST, 1);
 assigned_dev_direct_config_read(dev, PCI_CARDBUS_CIS, 4);
 assigned_dev_direct_config_read(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html