[PATCH] spapr: Modify ibm,get-config-addr-info2 to set DEVNUM in PE config address.

[PATCH] spapr: Modify ibm,get-config-addr-info2 to set DEVNUM in PE config address.

[Mahesh Salgaonkar]

With upstream kernel, especially after commit 98ba956f6a389
("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
guest isn't able to enable EEH option for PCI pass-through devices anymore.

[root@atest-guest ~]# dmesg | grep EEH
[    0.032337] EEH: pSeries platform initialized
[    0.298207] EEH: No capable adapters found: recovery disabled.
[root@atest-guest ~]#

So far the linux kernel was assuming pe_config_addr equal to device's
config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
PE config address first using the ibm,get-config-addr-info2 RTAS call, and
then uses the found address as argument to ibm,set-eeh-option RTAS call to
enable EEH on the PCI device. This works on PowerVM lpar but fails in qemu
KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
config address bits 16:20 be populated with device number (DEVNUM).

The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
"BB" represents the bus number of PE's primary bus and with device number
information always set to zero. However until commit 98ba956f6a389 this
return value wasn't used to enable EEH on the PCI device.

Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
with -3 return value indicating that there is no PCI device exist for the
specified pe config address. The rtas_ibm_set_eeh_option call uses
pci_find_device() to get the PC device that matches specific bus and devfn
extracted from PE config address passed as argument. Since the DEVFN part
of PE config always contains zero, pci_find_device() fails to find the
specific PCI device and hence fails to enable the EEH capability.

hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
   case RTAS_EEH_ENABLE: {
        PCIHostState *phb;
        PCIDevice *pdev;

        /*
         * The EEH functionality is enabled on basis of PCI device,
         * instead of PE. We need check the validity of the PCI
         * device address.
         */
        phb = PCI_HOST_BRIDGE(sphb);
        pdev = pci_find_device(phb->bus,
                               (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
        if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
            return RTAS_OUT_PARAM_ERROR;
        }

hw/pci/pci.c:pci_find_device()

PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
{
    bus = pci_find_bus_nr(bus, bus_num);

    if (!bus)
        return NULL;

    return bus->devices[devfn];
}

This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
config address with device number.

After this fix guest is able to find EEH capable devices and enable EEH
recovery on it.

[root@atest-guest ~]# dmesg | grep EEH
[    0.048139] EEH: pSeries platform initialized
[    0.405115] EEH: Capable adapter found: recovery enabled.
[root@atest-guest ~]#

Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
 hw/ppc/spapr_pci.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index feba18cb12..d6b0c380c8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -538,8 +538,9 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
     }
 
     /*
-     * We always have PE address of form "00BB0001". "BB"
-     * represents the bus number of PE's primary bus.
+     * Return PE address of form "00BBD001". "BB" represents the
+     * bus number of PE's primary bus and "D" represents the device number.
+     * Guest uses this PE address to enable EEH on this pci device.
      */
     option = rtas_ld(args, 3);
     switch (option) {
@@ -550,7 +551,8 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
             goto param_error_exit;
         }
 
-        rtas_st(rets, 1, (pci_bus_num(pci_get_bus(pdev)) << 16) + 1);
+        rtas_st(rets, 1, (pci_bus_num(pci_get_bus(pdev)) << 16) |
+                         (PCI_DEVFN(PCI_SLOT(pdev->devfn), 0) << 8) | 1);
         break;
     case RTAS_GET_PE_MODE:
         rtas_st(rets, 1, RTAS_PE_MODE_SHARED);

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Daniel Henrique Barboza]

On 4/27/21 8:56 AM, Mahesh Salgaonkar wrote:
> With upstream kernel, especially after commit 98ba956f6a389
> ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> guest isn't able to enable EEH option for PCI pass-through devices anymore.
> 
> [root@atest-guest ~]# dmesg | grep EEH
> [    0.032337] EEH: pSeries platform initialized
> [    0.298207] EEH: No capable adapters found: recovery disabled.
> [root@atest-guest ~]#
> 
> So far the linux kernel was assuming pe_config_addr equal to device's
> config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
> RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
> commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
> PE config address first using the ibm,get-config-addr-info2 RTAS call, and
> then uses the found address as argument to ibm,set-eeh-option RTAS call to
> enable EEH on the PCI device. This works on PowerVM lpar but fails in qemu
> KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
> config address bits 16:20 be populated with device number (DEVNUM).
> 
> The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
> config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
> "BB" represents the bus number of PE's primary bus and with device number
> information always set to zero. However until commit 98ba956f6a389 this
> return value wasn't used to enable EEH on the PCI device.
> 
> Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
> with -3 return value indicating that there is no PCI device exist for the
> specified pe config address. The rtas_ibm_set_eeh_option call uses
> pci_find_device() to get the PC device that matches specific bus and devfn
> extracted from PE config address passed as argument. Since the DEVFN part
> of PE config always contains zero, pci_find_device() fails to find the
> specific PCI device and hence fails to enable the EEH capability.
> 
> hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
>     case RTAS_EEH_ENABLE: {
>          PCIHostState *phb;
>          PCIDevice *pdev;
> 
>          /*
>           * The EEH functionality is enabled on basis of PCI device,
>           * instead of PE. We need check the validity of the PCI
>           * device address.
>           */
>          phb = PCI_HOST_BRIDGE(sphb);
>          pdev = pci_find_device(phb->bus,
>                                 (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
>          if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>              return RTAS_OUT_PARAM_ERROR;
>          }
> 
> hw/pci/pci.c:pci_find_device()
> 
> PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn)
> {
>      bus = pci_find_bus_nr(bus, bus_num);
> 
>      if (!bus)
>          return NULL;
> 
>      return bus->devices[devfn];
> }
> 
> This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
> config address with device number.
> 
> After this fix guest is able to find EEH capable devices and enable EEH
> recovery on it.
> 
> [root@atest-guest ~]# dmesg | grep EEH
> [    0.048139] EEH: pSeries platform initialized
> [    0.405115] EEH: Capable adapter found: recovery enabled.
> [root@atest-guest ~]#
> 
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   hw/ppc/spapr_pci.c |    8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index feba18cb12..d6b0c380c8 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -538,8 +538,9 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
>       }
>   
>       /*
> -     * We always have PE address of form "00BB0001". "BB"
> -     * represents the bus number of PE's primary bus.
> +     * Return PE address of form "00BBD001". "BB" represents the
> +     * bus number of PE's primary bus and "D" represents the device number.
> +     * Guest uses this PE address to enable EEH on this pci device.
>        */
>       option = rtas_ld(args, 3);
>       switch (option) {
> @@ -550,7 +551,8 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
>               goto param_error_exit;
>           }
>   
> -        rtas_st(rets, 1, (pci_bus_num(pci_get_bus(pdev)) << 16) + 1);
> +        rtas_st(rets, 1, (pci_bus_num(pci_get_bus(pdev)) << 16) |
> +                         (PCI_DEVFN(PCI_SLOT(pdev->devfn), 0) << 8) | 1);
>           break;
>       case RTAS_GET_PE_MODE:
>           rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
> 
> 

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Oliver O'Halloran]

On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>
> With upstream kernel, especially after commit 98ba956f6a389
> ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> guest isn't able to enable EEH option for PCI pass-through devices anymore.

How are you passing the devices through to the guest?

> [root@atest-guest ~]# dmesg | grep EEH
> [    0.032337] EEH: pSeries platform initialized
> [    0.298207] EEH: No capable adapters found: recovery disabled.
> [root@atest-guest ~]#
>
> So far the linux kernel was assuming pe_config_addr equal to device's
> config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
> RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
> commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
> PE config address first using the ibm,get-config-addr-info2 RTAS call, and
> then uses the found address as argument to ibm,set-eeh-option RTAS call to
> enable EEH on the PCI device.

That's not really correct. EEH being enabled or disabled is a per-PE
setting rather than a per-device setting. The fact there's not a 1-1
correspondence between devices and PEs is a large part of why the
get-config-addr-info2 RTAS call exists in the first place.
Unfortunately, the initial implementation of EEH support in linux
conflated the two because in the past there was typically a single
device within a PE. However, that assumption was never really correct
and it has long outlived its usefulness.

> This works on PowerVM lpar but fails in qemu
> KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
> config address bits 16:20 be populated with device number (DEVNUM).
>
> The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
> config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
> "BB" represents the bus number of PE's primary bus and with device number
> information always set to zero. However until commit 98ba956f6a389 this
> return value wasn't used to enable EEH on the PCI device.
>
> Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
> with -3 return value indicating that there is no PCI device exist for the
> specified pe config address. The rtas_ibm_set_eeh_option call uses
> pci_find_device() to get the PC device that matches specific bus and devfn
> extracted from PE config address passed as argument. Since the DEVFN part
> of PE config always contains zero, pci_find_device() fails to find the
> specific PCI device and hence fails to enable the EEH capability.
>
> hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
>    case RTAS_EEH_ENABLE: {
>         PCIHostState *phb;
>         PCIDevice *pdev;
>
>         /*
>          * The EEH functionality is enabled on basis of PCI device,
>          * instead of PE. We need check the validity of the PCI
>          * device address.
>          */
>         phb = PCI_HOST_BRIDGE(sphb);
>         pdev = pci_find_device(phb->bus,
>                                (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
>         if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>             return RTAS_OUT_PARAM_ERROR;
>         }
>
> This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
> config address with device number.

I don't think this is a good idea and I'm fairly sure you're
introducing some subtle breakage here. As an example, say that on the
host side you have two devices on the same bus:

0000:00:00.0 - card 1
0000:00:01.0 - card 2

On PowerNV (i.e. the hypervisor) these would be placed into the same
hardware PE since they're on the same bus. Now, if I take both devices
and pass them through to the guest on the same PHB and bus (use
0005:ff) we'll have:

0005:ff:00.0 - card 1
0005:ff:01.0 - card 2

With this patch applied get-config-addr-info2 returns 00BBD001, so the
PE we get for each device will be:

card 1 - 00ff0001
card 2 - 00ff1001

Which implies the two are in different PEs. As a result, if the guest
requests a reset of card 1's PE then the guest will see an unexpected
reset of card 2 as well. From the hypervisor's point of view the two
are in the same PE so this is a legitimate thing to do, but due to
this patch the guest doesn't know that.

As far as I can remember this is why you're supposed to pass each EEH
capable devices to the guest on a seperate spapr-phb (which matches
what PHYP does). Alexy can probably tell you more.

Oliver

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Alexey Kardashevskiy]

On 4/28/21 22:33, Oliver O'Halloran wrote:
> On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>>
>> With upstream kernel, especially after commit 98ba956f6a389
>> ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
>> guest isn't able to enable EEH option for PCI pass-through devices anymore.
> 
> How are you passing the devices through to the guest?
> 
>> [root@atest-guest ~]# dmesg | grep EEH
>> [    0.032337] EEH: pSeries platform initialized
>> [    0.298207] EEH: No capable adapters found: recovery disabled.
>> [root@atest-guest ~]#
>>
>> So far the linux kernel was assuming pe_config_addr equal to device's
>> config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
>> RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
>> commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
>> PE config address first using the ibm,get-config-addr-info2 RTAS call, and
>> then uses the found address as argument to ibm,set-eeh-option RTAS call to
>> enable EEH on the PCI device.
> 
> That's not really correct. EEH being enabled or disabled is a per-PE
> setting rather than a per-device setting. The fact there's not a 1-1
> correspondence between devices and PEs is a large part of why the
> get-config-addr-info2 RTAS call exists in the first place.
> Unfortunately, the initial implementation of EEH support in linux
> conflated the two because in the past there was typically a single
> device within a PE. However, that assumption was never really correct
> and it has long outlived its usefulness.
> 
>> This works on PowerVM lpar but fails in qemu
>> KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
>> config address bits 16:20 be populated with device number (DEVNUM).
>>
>> The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
>> config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
>> "BB" represents the bus number of PE's primary bus and with device number
>> information always set to zero. However until commit 98ba956f6a389 this
>> return value wasn't used to enable EEH on the PCI device.
>>
>> Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
>> with -3 return value indicating that there is no PCI device exist for the
>> specified pe config address. The rtas_ibm_set_eeh_option call uses
>> pci_find_device() to get the PC device that matches specific bus and devfn
>> extracted from PE config address passed as argument. Since the DEVFN part
>> of PE config always contains zero, pci_find_device() fails to find the
>> specific PCI device and hence fails to enable the EEH capability.
>>
>> hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
>>     case RTAS_EEH_ENABLE: {
>>          PCIHostState *phb;
>>          PCIDevice *pdev;
>>
>>          /*
>>           * The EEH functionality is enabled on basis of PCI device,
>>           * instead of PE. We need check the validity of the PCI
>>           * device address.
>>           */
>>          phb = PCI_HOST_BRIDGE(sphb);
>>          pdev = pci_find_device(phb->bus,
>>                                 (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
>>          if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>>              return RTAS_OUT_PARAM_ERROR;
>>          }
>>
>> This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
>> config address with device number.
> 
> I don't think this is a good idea and I'm fairly sure you're
> introducing some subtle breakage here. As an example, say that on the
> host side you have two devices on the same bus:
> 
> 0000:00:00.0 - card 1
> 0000:00:01.0 - card 2
> 
> On PowerNV (i.e. the hypervisor) these would be placed into the same
> hardware PE since they're on the same bus. Now, if I take both devices
> and pass them through to the guest on the same PHB and bus (use
> 0005:ff) we'll have:
> 
> 0005:ff:00.0 - card 1
> 0005:ff:01.0 - card 2
> 
> With this patch applied get-config-addr-info2 returns 00BBD001, so the
> PE we get for each device will be:
> 
> card 1 - 00ff0001
> card 2 - 00ff1001
> 
> Which implies the two are in different PEs. As a result, if the guest
> requests a reset of card 1's PE then the guest will see an unexpected
> reset of card 2 as well. From the hypervisor's point of view the two
> are in the same PE so this is a legitimate thing to do, but due to
> this patch the guest doesn't know that.


Agree. I guess we should only use vphbid:00:00.0 as a PE config address 
in QEMU as there is really just one per vphb which allows EEH.


> As far as I can remember this is why you're supposed to pass each EEH
> capable devices to the guest on a seperate spapr-phb (which matches
> what PHYP does). Alexy can probably tell you more.


The primary reason was that the EEH subdriver in VFIO did a poor job 
synchronizing states from different PEs so recovery was either tricky or 
broken:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=3153119;hp=f1a6cf3ef734aab142d5f7ce52e219474ababf6b


-- 
Alexey

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Mahesh J Salgaonkar]

On 2021-04-28 22:33:45 Wed, Oliver O'Halloran wrote:
> On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
> >
> > With upstream kernel, especially after commit 98ba956f6a389
> > ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> > guest isn't able to enable EEH option for PCI pass-through devices anymore.
> 
> How are you passing the devices through to the guest?

I am using libvirt with below xml section to add pass-through:

    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0033' bus='0x01' slot='0x00' function='0x0'/>
      </source>
      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0' multifunction='on'/>
    </hostdev>
    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0033' bus='0x01' slot='0x00' function='0x1'/>
      </source>
      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x1' multifunction='on'/>
    </hostdev>

Looks like libvirt does not allow pass through device in slot zero, and
throws following error.

error: XML error: Invalid PCI address 0000:01:00.0. slot must be >= 1
Failed. Try again? [y,n,i,f,?]:

> 
> > [root@atest-guest ~]# dmesg | grep EEH
> > [    0.032337] EEH: pSeries platform initialized
> > [    0.298207] EEH: No capable adapters found: recovery disabled.
> > [root@atest-guest ~]#
> >
> > So far the linux kernel was assuming pe_config_addr equal to device's
> > config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
> > RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
> > commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
> > PE config address first using the ibm,get-config-addr-info2 RTAS call, and
> > then uses the found address as argument to ibm,set-eeh-option RTAS call to
> > enable EEH on the PCI device.
> 
> That's not really correct. EEH being enabled or disabled is a per-PE
> setting rather than a per-device setting. The fact there's not a 1-1
> correspondence between devices and PEs is a large part of why the
> get-config-addr-info2 RTAS call exists in the first place.
> Unfortunately, the initial implementation of EEH support in linux
> conflated the two because in the past there was typically a single
> device within a PE. However, that assumption was never really correct
> and it has long outlived its usefulness.
> 
> > This works on PowerVM lpar but fails in qemu
> > KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
> > config address bits 16:20 be populated with device number (DEVNUM).
> >
> > The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
> > config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
> > "BB" represents the bus number of PE's primary bus and with device number
> > information always set to zero. However until commit 98ba956f6a389 this
> > return value wasn't used to enable EEH on the PCI device.
> >
> > Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
> > with -3 return value indicating that there is no PCI device exist for the
> > specified pe config address. The rtas_ibm_set_eeh_option call uses
> > pci_find_device() to get the PC device that matches specific bus and devfn
> > extracted from PE config address passed as argument. Since the DEVFN part
> > of PE config always contains zero, pci_find_device() fails to find the
> > specific PCI device and hence fails to enable the EEH capability.
> >
> > hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
> >    case RTAS_EEH_ENABLE: {
> >         PCIHostState *phb;
> >         PCIDevice *pdev;
> >
> >         /*
> >          * The EEH functionality is enabled on basis of PCI device,
> >          * instead of PE. We need check the validity of the PCI
> >          * device address.
> >          */
> >         phb = PCI_HOST_BRIDGE(sphb);
> >         pdev = pci_find_device(phb->bus,
> >                                (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
> >         if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> >             return RTAS_OUT_PARAM_ERROR;
> >         }
> >
> > This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
> > config address with device number.
> 
> I don't think this is a good idea and I'm fairly sure you're
> introducing some subtle breakage here. As an example, say that on the
> host side you have two devices on the same bus:
> 
> 0000:00:00.0 - card 1
> 0000:00:01.0 - card 2
> 
> On PowerNV (i.e. the hypervisor) these would be placed into the same
> hardware PE since they're on the same bus. Now, if I take both devices
> and pass them through to the guest on the same PHB and bus (use
> 0005:ff) we'll have:
> 
> 0005:ff:00.0 - card 1
> 0005:ff:01.0 - card 2

It looks like libvirt does not support pass through device in slot zero.
Hence these appears as below in guest:

 0005:ff:01.0 - card 1
 0005:ff:02.0 - card 2

And with get-config-addr-info2 returning 00BB0001, ibm,set-eeh-option
RTAS call tries to check if device is present on the bus of spapr_phb at
bus->devices[devfn] where devfn is 0. And since qemu does not support
pass through device on slot zero bus->devices[0] is always NULL. And
hence it fails to enable EEH.

> 
> With this patch applied get-config-addr-info2 returns 00BBD001, so the
> PE we get for each device will be:
> 
> card 1 - 00ff0001
> card 2 - 00ff1001
> 
> Which implies the two are in different PEs. As a result, if the guest
> requests a reset of card 1's PE then the guest will see an unexpected
> reset of card 2 as well. From the hypervisor's point of view the two
> are in the same PE so this is a legitimate thing to do, but due to
> this patch the guest doesn't know that.

Agree. I realize my fix is not correctly handling this. The current code
under ibm,set-eeh-option is checking for individual PCI device presence.
Better fix should be to check if there is any PCI device (vfio-pci)
present under specified bus and enable the EEH if found. And no change
in return value of get-config-addr-info2. What do you say ?

-- 
Mahesh J Salgaonkar

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Oliver O'Halloran]

On Thu, Apr 29, 2021 at 7:02 PM Mahesh J Salgaonkar
<mahesh@linux.ibm.com> wrote:
>
> On 2021-04-28 22:33:45 Wed, Oliver O'Halloran wrote:
> > On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
> > >
> > > With upstream kernel, especially after commit 98ba956f6a389
> > > ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> > > guest isn't able to enable EEH option for PCI pass-through devices anymore.
> >
> > How are you passing the devices through to the guest?
>
> I am using libvirt with below xml section to add pass-through:
>
>     <hostdev mode='subsystem' type='pci' managed='yes'>
>       <driver name='vfio'/>
>       <source>
>         <address domain='0x0033' bus='0x01' slot='0x00' function='0x0'/>
>       </source>
>       <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0' multifunction='on'/>
>     </hostdev>
>     <hostdev mode='subsystem' type='pci' managed='yes'>
>       <driver name='vfio'/>
>       <source>
>         <address domain='0x0033' bus='0x01' slot='0x00' function='0x1'/>
>       </source>
>       <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x1' multifunction='on'/>
>     </hostdev>
>
> Looks like libvirt does not allow pass through device in slot zero, and
> throws following error.
>
> error: XML error: Invalid PCI address 0000:01:00.0. slot must be >= 1
> Failed. Try again? [y,n,i,f,?]:

That's pretty odd and I have no idea why that's happening. I seem to
remember being able to use slot 0 for vfio devices when doing the
passthru manually with the qemu command line so this might be a
libvirt quirk.

> *snip*
>
> Agree. I realize my fix is not correctly handling this. The current code
> under ibm,set-eeh-option is checking for individual PCI device presence.
> Better fix should be to check if there is any PCI device (vfio-pci)
> present under specified bus and enable the EEH if found. And no change
> in return value of get-config-addr-info2. What do you say ?

That sounds reasonable. You would however need to verify that all the
devices on that bus are within the same PE on the hypervisor side.

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Daniel Henrique Barboza]

On 4/29/21 6:02 AM, Mahesh J Salgaonkar wrote:
> On 2021-04-28 22:33:45 Wed, Oliver O'Halloran wrote:
>> On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
>>>
>>> With upstream kernel, especially after commit 98ba956f6a389
>>> ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
>>> guest isn't able to enable EEH option for PCI pass-through devices anymore.
>>
>> How are you passing the devices through to the guest?
> 
> I am using libvirt with below xml section to add pass-through:
> 
>      <hostdev mode='subsystem' type='pci' managed='yes'>
>        <driver name='vfio'/>
>        <source>
>          <address domain='0x0033' bus='0x01' slot='0x00' function='0x0'/>
>        </source>
>        <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0' multifunction='on'/>
>      </hostdev>
>      <hostdev mode='subsystem' type='pci' managed='yes'>
>        <driver name='vfio'/>
>        <source>
>          <address domain='0x0033' bus='0x01' slot='0x00' function='0x1'/>
>        </source>
>        <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x1' multifunction='on'/>
>      </hostdev>
> 
> Looks like libvirt does not allow pass through device in slot zero, and
> throws following error.

There's no such restriction in Libvirt, at least as far as I remember.

> 
> error: XML error: Invalid PCI address 0000:01:00.0. slot must be >= 1
> Failed. Try again? [y,n,i,f,?]:


This looks odd. The error message is complaining about 0000:01:00.0 but
your XML up there is declaring 0000:01:01.0.

Also, the 'multifunction' bool is usually used only in the function 0
passthrough, indicating that the guest will configure all other functions as
the the multifunction device. You can ignore this bool in the XML for
PCI passthrough if you don't care about the guest seeing this device as
multifunction (i.e. all functions in the same IOMMU group and so on).





Thanks,


DHB

> 
>>
>>> [root@atest-guest ~]# dmesg | grep EEH
>>> [    0.032337] EEH: pSeries platform initialized
>>> [    0.298207] EEH: No capable adapters found: recovery disabled.
>>> [root@atest-guest ~]#
>>>
>>> So far the linux kernel was assuming pe_config_addr equal to device's
>>> config_addr and using it to enable EEH on the PE through ibm,set-eeh-option
>>> RTAS call. Which wasn't the correct way as per PAPR. The linux kernel
>>> commit 98ba956f6a389 fixed this flow. With that fixed, linux now gets the
>>> PE config address first using the ibm,get-config-addr-info2 RTAS call, and
>>> then uses the found address as argument to ibm,set-eeh-option RTAS call to
>>> enable EEH on the PCI device.
>>
>> That's not really correct. EEH being enabled or disabled is a per-PE
>> setting rather than a per-device setting. The fact there's not a 1-1
>> correspondence between devices and PEs is a large part of why the
>> get-config-addr-info2 RTAS call exists in the first place.
>> Unfortunately, the initial implementation of EEH support in linux
>> conflated the two because in the past there was typically a single
>> device within a PE. However, that assumption was never really correct
>> and it has long outlived its usefulness.
>>
>>> This works on PowerVM lpar but fails in qemu
>>> KVM guests. This is because ibm,set-eeh-option RTAS call in qemu expects PE
>>> config address bits 16:20 be populated with device number (DEVNUM).
>>>
>>> The rtas call ibm,get-config-addr-info2 in qemu always returns the PE
>>> config address in form of "00BB0001" (i.e.  <00><BUS><DEVFN><REG>) where
>>> "BB" represents the bus number of PE's primary bus and with device number
>>> information always set to zero. However until commit 98ba956f6a389 this
>>> return value wasn't used to enable EEH on the PCI device.
>>>
>>> Now in qemu guest with recent kernel the ibm,set-eeh-option RTAS call fails
>>> with -3 return value indicating that there is no PCI device exist for the
>>> specified pe config address. The rtas_ibm_set_eeh_option call uses
>>> pci_find_device() to get the PC device that matches specific bus and devfn
>>> extracted from PE config address passed as argument. Since the DEVFN part
>>> of PE config always contains zero, pci_find_device() fails to find the
>>> specific PCI device and hence fails to enable the EEH capability.
>>>
>>> hw/ppc/spapr_pci_vfio.c: spapr_phb_vfio_eeh_set_option()
>>>     case RTAS_EEH_ENABLE: {
>>>          PCIHostState *phb;
>>>          PCIDevice *pdev;
>>>
>>>          /*
>>>           * The EEH functionality is enabled on basis of PCI device,
>>>           * instead of PE. We need check the validity of the PCI
>>>           * device address.
>>>           */
>>>          phb = PCI_HOST_BRIDGE(sphb);
>>>          pdev = pci_find_device(phb->bus,
>>>                                 (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
>>>          if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
>>>              return RTAS_OUT_PARAM_ERROR;
>>>          }
>>>
>>> This patch fixes this issue by populating DEVNUM field (bits 16:20) of PE
>>> config address with device number.
>>
>> I don't think this is a good idea and I'm fairly sure you're
>> introducing some subtle breakage here. As an example, say that on the
>> host side you have two devices on the same bus:
>>
>> 0000:00:00.0 - card 1
>> 0000:00:01.0 - card 2
>>
>> On PowerNV (i.e. the hypervisor) these would be placed into the same
>> hardware PE since they're on the same bus. Now, if I take both devices
>> and pass them through to the guest on the same PHB and bus (use
>> 0005:ff) we'll have:
>>
>> 0005:ff:00.0 - card 1
>> 0005:ff:01.0 - card 2
> 
> It looks like libvirt does not support pass through device in slot zero.
> Hence these appears as below in guest:
> 
>   0005:ff:01.0 - card 1
>   0005:ff:02.0 - card 2
> 
> And with get-config-addr-info2 returning 00BB0001, ibm,set-eeh-option
> RTAS call tries to check if device is present on the bus of spapr_phb at
> bus->devices[devfn] where devfn is 0. And since qemu does not support
> pass through device on slot zero bus->devices[0] is always NULL. And
> hence it fails to enable EEH.
> 
>>
>> With this patch applied get-config-addr-info2 returns 00BBD001, so the
>> PE we get for each device will be:
>>
>> card 1 - 00ff0001
>> card 2 - 00ff1001
>>
>> Which implies the two are in different PEs. As a result, if the guest
>> requests a reset of card 1's PE then the guest will see an unexpected
>> reset of card 2 as well. From the hypervisor's point of view the two
>> are in the same PE so this is a legitimate thing to do, but due to
>> this patch the guest doesn't know that.
> 
> Agree. I realize my fix is not correctly handling this. The current code
> under ibm,set-eeh-option is checking for individual PCI device presence.
> Better fix should be to check if there is any PCI device (vfio-pci)
> present under specified bus and enable the EEH if found. And no change
> in return value of get-config-addr-info2. What do you say ?
> 

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Mahesh J Salgaonkar]

On 2021-04-30 07:52:58 Fri, Daniel Henrique Barboza wrote:
> 
> 
> On 4/29/21 6:02 AM, Mahesh J Salgaonkar wrote:
> > On 2021-04-28 22:33:45 Wed, Oliver O'Halloran wrote:
> > > On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
> > > > 
> > > > With upstream kernel, especially after commit 98ba956f6a389
> > > > ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> > > > guest isn't able to enable EEH option for PCI pass-through devices anymore.
> > > 
> > > How are you passing the devices through to the guest?
> > 
> > I am using libvirt with below xml section to add pass-through:
> > 
> >      <hostdev mode='subsystem' type='pci' managed='yes'>
> >        <driver name='vfio'/>
> >        <source>
> >          <address domain='0x0033' bus='0x01' slot='0x00' function='0x0'/>
> >        </source>
> >        <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0' multifunction='on'/>
> >      </hostdev>
> >      <hostdev mode='subsystem' type='pci' managed='yes'>
> >        <driver name='vfio'/>
> >        <source>
> >          <address domain='0x0033' bus='0x01' slot='0x00' function='0x1'/>
> >        </source>
> >        <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x1' multifunction='on'/>
> >      </hostdev>
> > 
> > Looks like libvirt does not allow pass through device in slot zero, and
> > throws following error.
> 
> There's no such restriction in Libvirt, at least as far as I remember.
> 
> > 
> > error: XML error: Invalid PCI address 0000:01:00.0. slot must be >= 1
> > Failed. Try again? [y,n,i,f,?]:
> 
> 
> This looks odd. The error message is complaining about 0000:01:00.0 but
> your XML up there is declaring 0000:01:01.0.

Above XML snipphet is working one. I see the XML error when I change slot
value to zero.

> 
> Also, the 'multifunction' bool is usually used only in the function 0
> passthrough, indicating that the guest will configure all other functions as
> the the multifunction device. You can ignore this bool in the XML for
> PCI passthrough if you don't care about the guest seeing this device as
> multifunction (i.e. all functions in the same IOMMU group and so on).
> 

Thanks,
-Mahesh.

Re: [PATCH] spapr: Modify ibm, get-config-addr-info2 to set DEVNUM in PE config address.

[Mahesh J Salgaonkar]

On 2021-04-30 11:53:24 Fri, Oliver O'Halloran wrote:
> On Thu, Apr 29, 2021 at 7:02 PM Mahesh J Salgaonkar
> <mahesh@linux.ibm.com> wrote:
> >
> > On 2021-04-28 22:33:45 Wed, Oliver O'Halloran wrote:
> > > On Tue, Apr 27, 2021 at 9:56 PM Mahesh Salgaonkar <mahesh@linux.ibm.com> wrote:
> > > >
> > > > With upstream kernel, especially after commit 98ba956f6a389
> > > > ("powerpc/pseries/eeh: Rework device EEH PE determination") we see that KVM
> > > > guest isn't able to enable EEH option for PCI pass-through devices anymore.
> > >
> > > How are you passing the devices through to the guest?
> >
> > I am using libvirt with below xml section to add pass-through:
> >
> >     <hostdev mode='subsystem' type='pci' managed='yes'>
> >       <driver name='vfio'/>
> >       <source>
> >         <address domain='0x0033' bus='0x01' slot='0x00' function='0x0'/>
> >       </source>
> >       <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0' multifunction='on'/>
> >     </hostdev>
> >     <hostdev mode='subsystem' type='pci' managed='yes'>
> >       <driver name='vfio'/>
> >       <source>
> >         <address domain='0x0033' bus='0x01' slot='0x00' function='0x1'/>
> >       </source>
> >       <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x1' multifunction='on'/>
> >     </hostdev>
> >
> > Looks like libvirt does not allow pass through device in slot zero, and
> > throws following error.
> >
> > error: XML error: Invalid PCI address 0000:01:00.0. slot must be >= 1
> > Failed. Try again? [y,n,i,f,?]:
> 
> That's pretty odd and I have no idea why that's happening. I seem to
> remember being able to use slot 0 for vfio devices when doing the
> passthru manually with the qemu command line so this might be a
> libvirt quirk.
> 
> > *snip*
> >
> > Agree. I realize my fix is not correctly handling this. The current code
> > under ibm,set-eeh-option is checking for individual PCI device presence.
> > Better fix should be to check if there is any PCI device (vfio-pci)
> > present under specified bus and enable the EEH if found. And no change
> > in return value of get-config-addr-info2. What do you say ?
> 
> That sounds reasonable. You would however need to verify that all the
> devices on that bus are within the same PE on the hypervisor side.

I see that the spapr_phb_eeh_available(sphb) check in ibm,set-eeh-option
already makes sure that all the devices on that bus are from same iommu
group (within same PE) before going ahead with enabling EEH.

---------
rtas_ibm_set_eeh_option():
    ...
    if (!spapr_phb_eeh_available(sphb)) {          <---
        goto param_error_exit;
    }

    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);
    rtas_st(rets, 0, ret);
    return;

param_error_exit:
    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
----------

I verified that, if we add devices from two different PHB under same
sphb then spapr_phb_eeh_available(sphb) returns false and we fail to
enable EEH. Hence we are good here, we fail very early if devices on
that bus are not within same PE.

So, I just need to check for presence of any vfio-pci device under the
specified bus and enable the EEH. Let me know your views on this.

Thanks,
-Mahesh.

-- 
Mahesh J Salgaonkar