[PATCH] pci-assign: Fix multifunction support

[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.williamson@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);

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.williamson@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

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.williamson@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

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

[Jan Kiszka]

On 2012-01-17 14:23, Alex Williamson wrote:
> 
> 
> ----- 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.williamson@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.

OK.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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.williamson@redhat.com>

Applied, thanks.