[PATCH 0/4] s390x/pci: some small fixes

[PATCH 0/4] s390x/pci: some small fixes

[Matthew Rosato]

A collection of small fixes for s390x PCI (not urgent).  The first 3 are        
fixes related to always using the host-provided CLP value when provided         
vs a hard-coded value.  The last patch adds logic for QEMU to report a          
proper DTSM clp response rather than just 0s (guest linux doesn't look          
at this field today).

Matthew Rosato (4):
  s390x/pci: use a reserved ID for the default PCI group
  s390x/pci: don't use hard-coded dma range in reg_ioat
  s390x/pci: use the passthrough measurement update interval
  s390x/pci: add supported DT information to clp response

 hw/s390x/s390-pci-bus.c         |  1 +
 hw/s390x/s390-pci-inst.c        | 14 ++++++++------
 hw/s390x/s390-pci-vfio.c        |  1 +
 include/hw/s390x/s390-pci-bus.h |  3 ++-
 include/hw/s390x/s390-pci-clp.h |  3 ++-
 5 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.27.0

[PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Matthew Rosato]

The current default PCI group being used can technically collide with a
real group ID passed from a hostdev.  Let's instead use a group ID that comes
from a special pool that is architected to be reserved for simulated devices.

Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 include/hw/s390x/s390-pci-bus.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index aa891c178d..2727e7bdef 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -313,7 +313,7 @@ typedef struct ZpciFmb {
 } ZpciFmb;
 QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
 
-#define ZPCI_DEFAULT_FN_GRP 0x20
+#define ZPCI_DEFAULT_FN_GRP 0xFF
 typedef struct S390PCIGroup {
     ClpRspQueryPciGrp zpci_group;
     int id;
-- 
2.27.0

[PATCH 2/4] s390x/pci: don't use hard-coded dma range in reg_ioat

[Matthew Rosato]

Instead use the values from clp info, they will either be the hard-coded
values or what came from the host driver via vfio.

Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 1c8ad91175..11b7f6bfa1 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -916,9 +916,10 @@ int pci_dereg_irqs(S390PCIBusDevice *pbdev)
     return 0;
 }
 
-static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
+static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
                     uintptr_t ra)
 {
+    S390PCIIOMMU *iommu = pbdev->iommu;
     uint64_t pba = ldq_p(&fib.pba);
     uint64_t pal = ldq_p(&fib.pal);
     uint64_t g_iota = ldq_p(&fib.iota);
@@ -927,7 +928,7 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
 
     pba &= ~0xfff;
     pal |= 0xfff;
-    if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
+    if (pba > pal || pba < pbdev->zpci_fn.sdma || pal > pbdev->zpci_fn.edma) {
         s390_program_interrupt(env, PGM_OPERAND, ra);
         return -EINVAL;
     }
@@ -1125,7 +1126,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
         } else if (pbdev->iommu->enabled) {
             cc = ZPCI_PCI_LS_ERR;
             s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
-        } else if (reg_ioat(env, pbdev->iommu, fib, ra)) {
+        } else if (reg_ioat(env, pbdev, fib, ra)) {
             cc = ZPCI_PCI_LS_ERR;
             s390_set_status_code(env, r1, ZPCI_MOD_ST_INSUF_RES);
         }
@@ -1150,7 +1151,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
             s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
         } else {
             pci_dereg_ioat(pbdev->iommu);
-            if (reg_ioat(env, pbdev->iommu, fib, ra)) {
+            if (reg_ioat(env, pbdev, fib, ra)) {
                 cc = ZPCI_PCI_LS_ERR;
                 s390_set_status_code(env, r1, ZPCI_MOD_ST_INSUF_RES);
             }
-- 
2.27.0

[PATCH 3/4] s390x/pci: use the passthrough measurement update interval

[Matthew Rosato]

We may have gotten a measurement update interval from the underlying host
via vfio -- Use it to set the interval via which we update the function
measurement block.

Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 11b7f6bfa1..07bab85ce5 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -1046,7 +1046,7 @@ static void fmb_update(void *opaque)
                       sizeof(pbdev->fmb.last_update))) {
         return;
     }
-    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
+    timer_mod(pbdev->fmb_timer, t + pbdev->pci_group->zpci_group.mui);
 }
 
 int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
@@ -1204,7 +1204,8 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
         }
         pbdev->fmb_addr = fmb_addr;
         timer_mod(pbdev->fmb_timer,
-                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
+                                    pbdev->pci_group->zpci_group.mui);
         break;
     }
     default:
-- 
2.27.0

[PATCH 4/4] s390x/pci: add supported DT information to clp response

[Matthew Rosato]

The DTSM is a mask that specifies which I/O Address Translation designation
types are supported.  A linux guest today does not look at this field but
could in the future; let's advertise what QEMU actually supports.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-bus.c         | 1 +
 hw/s390x/s390-pci-vfio.c        | 1 +
 include/hw/s390x/s390-pci-bus.h | 1 +
 include/hw/s390x/s390-pci-clp.h | 3 ++-
 4 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 1b51a72838..01b58ebc70 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -782,6 +782,7 @@ static void s390_pci_init_default_group(void)
     resgrp->i = 128;
     resgrp->maxstbl = 128;
     resgrp->version = 0;
+    resgrp->dtsm = ZPCI_DTSM;
 }
 
 static void set_pbdev_info(S390PCIBusDevice *pbdev)
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index 2a153fa8c9..6f80a47e29 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -160,6 +160,7 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
         resgrp->i = cap->noi;
         resgrp->maxstbl = cap->maxstbl;
         resgrp->version = cap->version;
+        resgrp->dtsm = ZPCI_DTSM;
     }
 }
 
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 2727e7bdef..da3cde2bb4 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -37,6 +37,7 @@
 #define ZPCI_MAX_UID 0xffff
 #define UID_UNDEFINED 0
 #define UID_CHECKING_ENABLED 0x01
+#define ZPCI_DTSM 0x40
 
 OBJECT_DECLARE_SIMPLE_TYPE(S390pciState, S390_PCI_HOST_BRIDGE)
 OBJECT_DECLARE_SIMPLE_TYPE(S390PCIBus, S390_PCI_BUS)
diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
index 96b8e3f133..cc8c8662b8 100644
--- a/include/hw/s390x/s390-pci-clp.h
+++ b/include/hw/s390x/s390-pci-clp.h
@@ -163,7 +163,8 @@ typedef struct ClpRspQueryPciGrp {
     uint8_t fr;
     uint16_t maxstbl;
     uint16_t mui;
-    uint64_t reserved3;
+    uint8_t dtsm;
+    uint8_t reserved3[7];
     uint64_t dasm; /* dma address space mask */
     uint64_t msia; /* MSI address */
     uint64_t reserved4;
-- 
2.27.0

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[David Hildenbrand]

On 02.12.21 17:41, Matthew Rosato wrote:
> The current default PCI group being used can technically collide with a
> real group ID passed from a hostdev.  Let's instead use a group ID that comes
> from a special pool that is architected to be reserved for simulated devices.
> 
> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  include/hw/s390x/s390-pci-bus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index aa891c178d..2727e7bdef 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>  } ZpciFmb;
>  QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>  
> -#define ZPCI_DEFAULT_FN_GRP 0x20
> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>  typedef struct S390PCIGroup {
>      ClpRspQueryPciGrp zpci_group;
>      int id;
> 

What happens if we migrate a VM from old to new QEMU? Won't the guest be
able to observe the change?

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Matthew Rosato]

On 12/2/21 11:43 AM, David Hildenbrand wrote:
> On 02.12.21 17:41, Matthew Rosato wrote:
>> The current default PCI group being used can technically collide with a
>> real group ID passed from a hostdev.  Let's instead use a group ID that comes
>> from a special pool that is architected to be reserved for simulated devices.
>>
>> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   include/hw/s390x/s390-pci-bus.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
>> index aa891c178d..2727e7bdef 100644
>> --- a/include/hw/s390x/s390-pci-bus.h
>> +++ b/include/hw/s390x/s390-pci-bus.h
>> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>>   } ZpciFmb;
>>   QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>>   
>> -#define ZPCI_DEFAULT_FN_GRP 0x20
>> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>>   typedef struct S390PCIGroup {
>>       ClpRspQueryPciGrp zpci_group;
>>       int id;
>>
> 
> What happens if we migrate a VM from old to new QEMU? Won't the guest be
> able to observe the change?
> 

Yes, technically --  But # itself is not really all that important, it 
is provided from CLP Q PCI FN to be subsequently used as input into Q 
PCI FNGRP -- With the fundamental notion being that all functions that 
share the same group # share the same group CLP info.  Whether the 
number is, say, 1 or 5 doesn't matter so much.

However..  0xF0 and greater are the only values reserved for hypervisor 
use.  By using 0x20 we run the risk of accidentally conflating simulated 
devices and real hardware, hence the desire to change it.

Is your concern about a migrated guest with a virtio device trying to do 
a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could 
modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch 
simulated devices trying to use something other than the default group, 
e.g.:

if ((pbdev->fh & FH_SHM_EMUL) &&
     (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
         /* Simulated device MUST have default group */
	pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
	group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
}

What do you think?

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Eric Farman]

On Thu, 2021-12-02 at 11:41 -0500, Matthew Rosato wrote:
> The current default PCI group being used can technically collide with
> a
> real group ID passed from a hostdev.  Let's instead use a group ID
> that comes
> from a special pool that is architected to be reserved for simulated
> devices.
> 
> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Regardless of the question regarding virtio migration, this is good.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  include/hw/s390x/s390-pci-bus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-
> pci-bus.h
> index aa891c178d..2727e7bdef 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>  } ZpciFmb;
>  QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in
> ZpciFmb");
>  
> -#define ZPCI_DEFAULT_FN_GRP 0x20
> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>  typedef struct S390PCIGroup {
>      ClpRspQueryPciGrp zpci_group;
>      int id;

Re: [PATCH 2/4] s390x/pci: don't use hard-coded dma range in reg_ioat

[Eric Farman]

On Thu, 2021-12-02 at 11:41 -0500, Matthew Rosato wrote:
> Instead use the values from clp info, they will either be the hard-
> coded
> values or what came from the host driver via vfio.
> 
> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  hw/s390x/s390-pci-inst.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 1c8ad91175..11b7f6bfa1 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -916,9 +916,10 @@ int pci_dereg_irqs(S390PCIBusDevice *pbdev)
>      return 0;
>  }
>  
> -static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib
> fib,
> +static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev,
> ZpciFib fib,
>                      uintptr_t ra)
>  {
> +    S390PCIIOMMU *iommu = pbdev->iommu;
>      uint64_t pba = ldq_p(&fib.pba);
>      uint64_t pal = ldq_p(&fib.pal);
>      uint64_t g_iota = ldq_p(&fib.iota);
> @@ -927,7 +928,7 @@ static int reg_ioat(CPUS390XState *env,
> S390PCIIOMMU *iommu, ZpciFib fib,
>  
>      pba &= ~0xfff;
>      pal |= 0xfff;
> -    if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
> +    if (pba > pal || pba < pbdev->zpci_fn.sdma || pal > pbdev-
> >zpci_fn.edma) {
>          s390_program_interrupt(env, PGM_OPERAND, ra);
>          return -EINVAL;
>      }
> @@ -1125,7 +1126,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t
> r1, uint64_t fiba, uint8_t ar,
>          } else if (pbdev->iommu->enabled) {
>              cc = ZPCI_PCI_LS_ERR;
>              s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
> -        } else if (reg_ioat(env, pbdev->iommu, fib, ra)) {
> +        } else if (reg_ioat(env, pbdev, fib, ra)) {
>              cc = ZPCI_PCI_LS_ERR;
>              s390_set_status_code(env, r1, ZPCI_MOD_ST_INSUF_RES);
>          }
> @@ -1150,7 +1151,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t
> r1, uint64_t fiba, uint8_t ar,
>              s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
>          } else {
>              pci_dereg_ioat(pbdev->iommu);
> -            if (reg_ioat(env, pbdev->iommu, fib, ra)) {
> +            if (reg_ioat(env, pbdev, fib, ra)) {
>                  cc = ZPCI_PCI_LS_ERR;
>                  s390_set_status_code(env, r1,
> ZPCI_MOD_ST_INSUF_RES);
>              }

Re: [PATCH 3/4] s390x/pci: use the passthrough measurement update interval

[Eric Farman]

On Thu, 2021-12-02 at 11:41 -0500, Matthew Rosato wrote:
> We may have gotten a measurement update interval from the underlying
> host
> via vfio -- Use it to set the interval via which we update the
> function
> measurement block.
> 
> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  hw/s390x/s390-pci-inst.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 11b7f6bfa1..07bab85ce5 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -1046,7 +1046,7 @@ static void fmb_update(void *opaque)
>                        sizeof(pbdev->fmb.last_update))) {
>          return;
>      }
> -    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
> +    timer_mod(pbdev->fmb_timer, t + pbdev->pci_group-
> >zpci_group.mui);
>  }
>  
>  int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba,
> uint8_t ar,
> @@ -1204,7 +1204,8 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t
> r1, uint64_t fiba, uint8_t ar,
>          }
>          pbdev->fmb_addr = fmb_addr;
>          timer_mod(pbdev->fmb_timer,
> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> DEFAULT_MUI);
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> +                                    pbdev->pci_group-
> >zpci_group.mui);
>          break;
>      }
>      default:

Re: [PATCH 4/4] s390x/pci: add supported DT information to clp response

[Eric Farman]

On Thu, 2021-12-02 at 11:41 -0500, Matthew Rosato wrote:
> The DTSM is a mask that specifies which I/O Address Translation
> designation
> types are supported.  A linux guest today does not look at this field
> but
> could in the future; let's advertise what QEMU actually supports.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Also good.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  hw/s390x/s390-pci-bus.c         | 1 +
>  hw/s390x/s390-pci-vfio.c        | 1 +
>  include/hw/s390x/s390-pci-bus.h | 1 +
>  include/hw/s390x/s390-pci-clp.h | 3 ++-
>  4 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1b51a72838..01b58ebc70 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -782,6 +782,7 @@ static void s390_pci_init_default_group(void)
>      resgrp->i = 128;
>      resgrp->maxstbl = 128;
>      resgrp->version = 0;
> +    resgrp->dtsm = ZPCI_DTSM;
>  }
>  
>  static void set_pbdev_info(S390PCIBusDevice *pbdev)
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index 2a153fa8c9..6f80a47e29 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -160,6 +160,7 @@ static void s390_pci_read_group(S390PCIBusDevice
> *pbdev,
>          resgrp->i = cap->noi;
>          resgrp->maxstbl = cap->maxstbl;
>          resgrp->version = cap->version;
> +        resgrp->dtsm = ZPCI_DTSM;
>      }
>  }
>  
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-
> pci-bus.h
> index 2727e7bdef..da3cde2bb4 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -37,6 +37,7 @@
>  #define ZPCI_MAX_UID 0xffff
>  #define UID_UNDEFINED 0
>  #define UID_CHECKING_ENABLED 0x01
> +#define ZPCI_DTSM 0x40
>  
>  OBJECT_DECLARE_SIMPLE_TYPE(S390pciState, S390_PCI_HOST_BRIDGE)
>  OBJECT_DECLARE_SIMPLE_TYPE(S390PCIBus, S390_PCI_BUS)
> diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-
> pci-clp.h
> index 96b8e3f133..cc8c8662b8 100644
> --- a/include/hw/s390x/s390-pci-clp.h
> +++ b/include/hw/s390x/s390-pci-clp.h
> @@ -163,7 +163,8 @@ typedef struct ClpRspQueryPciGrp {
>      uint8_t fr;
>      uint16_t maxstbl;
>      uint16_t mui;
> -    uint64_t reserved3;
> +    uint8_t dtsm;
> +    uint8_t reserved3[7];
>      uint64_t dasm; /* dma address space mask */
>      uint64_t msia; /* MSI address */
>      uint64_t reserved4;

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[David Hildenbrand]

On 02.12.21 18:11, Matthew Rosato wrote:
> On 12/2/21 11:43 AM, David Hildenbrand wrote:
>> On 02.12.21 17:41, Matthew Rosato wrote:
>>> The current default PCI group being used can technically collide with a
>>> real group ID passed from a hostdev.  Let's instead use a group ID that comes
>>> from a special pool that is architected to be reserved for simulated devices.
>>>
>>> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> ---
>>>   include/hw/s390x/s390-pci-bus.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
>>> index aa891c178d..2727e7bdef 100644
>>> --- a/include/hw/s390x/s390-pci-bus.h
>>> +++ b/include/hw/s390x/s390-pci-bus.h
>>> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>>>   } ZpciFmb;
>>>   QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>>>   
>>> -#define ZPCI_DEFAULT_FN_GRP 0x20
>>> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>>>   typedef struct S390PCIGroup {
>>>       ClpRspQueryPciGrp zpci_group;
>>>       int id;
>>>
>>
>> What happens if we migrate a VM from old to new QEMU? Won't the guest be
>> able to observe the change?
>>
> 
> Yes, technically --  But # itself is not really all that important, it 
> is provided from CLP Q PCI FN to be subsequently used as input into Q 
> PCI FNGRP -- With the fundamental notion being that all functions that 
> share the same group # share the same group CLP info.  Whether the 
> number is, say, 1 or 5 doesn't matter so much.
> 
> However..  0xF0 and greater are the only values reserved for hypervisor 
> use.  By using 0x20 we run the risk of accidentally conflating simulated 
> devices and real hardware, hence the desire to change it.
> 
> Is your concern about a migrated guest with a virtio device trying to do 
> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could 
> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch 
> simulated devices trying to use something other than the default group, 
> e.g.:
> 
> if ((pbdev->fh & FH_SHM_EMUL) &&
>      (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>          /* Simulated device MUST have default group */
> 	pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
> 	group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
> }
> 
> What do you think?
> 

Good question, I'm certainly not a zPCI expert. However if you think
that we cannot really break migration in a sane use case, I'm fine with
it as well :)

The question about breaking migration just popped up in my head when I
stumbled over this patch.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Halil Pasic]

On Thu, 2 Dec 2021 12:11:38 -0500
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> > 
> > What happens if we migrate a VM from old to new QEMU? Won't the guest be
> > able to observe the change?
> >   
> 
> Yes, technically --  But # itself is not really all that important, it 
> is provided from CLP Q PCI FN to be subsequently used as input into Q 
> PCI FNGRP -- With the fundamental notion being that all functions that 
> share the same group # share the same group CLP info.  Whether the 
> number is, say, 1 or 5 doesn't matter so much.
> 
> However..  0xF0 and greater are the only values reserved for hypervisor 
> use.  By using 0x20 we run the risk of accidentally conflating simulated 
> devices and real hardware, hence the desire to change it.
> 
> Is your concern about a migrated guest with a virtio device trying to do 
> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could 
> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch 
> simulated devices trying to use something other than the default group, 
> e.g.:
> 
> if ((pbdev->fh & FH_SHM_EMUL) &&
>      (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>          /* Simulated device MUST have default group */
> 	pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
> 	group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
> }
> 
> What do you think?

Another option, and in my opinion the cleaner one would be to tie this
change to a new machine version. That is if a post-change qemu is used
in compatibility mode, we would still have the old behavior.

What do you think?

Regards,
Halil

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Matthew Rosato]

On 12/2/21 6:06 PM, Halil Pasic wrote:
> On Thu, 2 Dec 2021 12:11:38 -0500
> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
> 
>>>
>>> What happens if we migrate a VM from old to new QEMU? Won't the guest be
>>> able to observe the change?
>>>    
>>
>> Yes, technically --  But # itself is not really all that important, it
>> is provided from CLP Q PCI FN to be subsequently used as input into Q
>> PCI FNGRP -- With the fundamental notion being that all functions that
>> share the same group # share the same group CLP info.  Whether the
>> number is, say, 1 or 5 doesn't matter so much.
>>
>> However..  0xF0 and greater are the only values reserved for hypervisor
>> use.  By using 0x20 we run the risk of accidentally conflating simulated
>> devices and real hardware, hence the desire to change it.
>>
>> Is your concern about a migrated guest with a virtio device trying to do
>> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could
>> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch
>> simulated devices trying to use something other than the default group,
>> e.g.:
>>
>> if ((pbdev->fh & FH_SHM_EMUL) &&
>>       (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>>           /* Simulated device MUST have default group */
>> 	pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
>> 	group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
>> }
>>
>> What do you think?
> 
> Another option, and in my opinion the cleaner one would be to tie this
> change to a new machine version. That is if a post-change qemu is used
> in compatibility mode, we would still have the old behavior.
> 
> What do you think?
> 

The problem there is that the old behavior goes against the architecture 
(group 0x20 could belong to real hardware) and AFAIU assigning this new 
behavior only to a new machine version means we can't fix old stable 
QEMU versions.

Also, wait a minute -- migration isn't even an option right now, it's 
blocked for zpci devices, both passthrough and simulated (see 
aede5d5dfc5f 's390x/pci: mark zpci devices as unmigratable') so I say 
let's just move to a proper default group now before we potentially 
allow migration later.

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Pierre Morel]

On 12/3/21 03:25, Matthew Rosato wrote:
> On 12/2/21 6:06 PM, Halil Pasic wrote:
>> On Thu, 2 Dec 2021 12:11:38 -0500
>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>>>>
>>>> What happens if we migrate a VM from old to new QEMU? Won't the 
>>>> guest be
>>>> able to observe the change?
>>>
>>> Yes, technically --  But # itself is not really all that important, it
>>> is provided from CLP Q PCI FN to be subsequently used as input into Q
>>> PCI FNGRP -- With the fundamental notion being that all functions that
>>> share the same group # share the same group CLP info.  Whether the
>>> number is, say, 1 or 5 doesn't matter so much.
>>>
>>> However..  0xF0 and greater are the only values reserved for hypervisor
>>> use.  By using 0x20 we run the risk of accidentally conflating simulated
>>> devices and real hardware, hence the desire to change it.
>>>
>>> Is your concern about a migrated guest with a virtio device trying to do
>>> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could
>>> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch
>>> simulated devices trying to use something other than the default group,
>>> e.g.:
>>>
>>> if ((pbdev->fh & FH_SHM_EMUL) &&
>>>       (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>>>           /* Simulated device MUST have default group */
>>>     pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
>>>     group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
>>> }
>>>
>>> What do you think?
>>
>> Another option, and in my opinion the cleaner one would be to tie this
>> change to a new machine version. That is if a post-change qemu is used
>> in compatibility mode, we would still have the old behavior.
>>
>> What do you think?
>>
> 
> The problem there is that the old behavior goes against the architecture 
> (group 0x20 could belong to real hardware) and AFAIU assigning this new 
> behavior only to a new machine version means we can't fix old stable 
> QEMU versions.
> 
> Also, wait a minute -- migration isn't even an option right now, it's 
> blocked for zpci devices, both passthrough and simulated (see 
> aede5d5dfc5f 's390x/pci: mark zpci devices as unmigratable') so I say 
> let's just move to a proper default group now before we potentially 
> allow migration later.

Looks right to me.

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 2/4] s390x/pci: don't use hard-coded dma range in reg_ioat

[Pierre Morel]

On 12/2/21 17:41, Matthew Rosato wrote:
> Instead use the values from clp info, they will either be the hard-coded
> values or what came from the host driver via vfio.
> 
> Fixes: 9670ee752727 ("s390x/pci: use a PCI Function structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-inst.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 1c8ad91175..11b7f6bfa1 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -916,9 +916,10 @@ int pci_dereg_irqs(S390PCIBusDevice *pbdev)
>       return 0;
>   }
>   
> -static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
> +static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
>                       uintptr_t ra)
>   {
> +    S390PCIIOMMU *iommu = pbdev->iommu;
>       uint64_t pba = ldq_p(&fib.pba);
>       uint64_t pal = ldq_p(&fib.pal);
>       uint64_t g_iota = ldq_p(&fib.iota);
> @@ -927,7 +928,7 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib,
>   
>       pba &= ~0xfff;
>       pal |= 0xfff;
> -    if (pba > pal || pba < ZPCI_SDMA_ADDR || pal > ZPCI_EDMA_ADDR) {
> +    if (pba > pal || pba < pbdev->zpci_fn.sdma || pal > pbdev->zpci_fn.edma) {
>           s390_program_interrupt(env, PGM_OPERAND, ra);
>           return -EINVAL;
>       }
> @@ -1125,7 +1126,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>           } else if (pbdev->iommu->enabled) {
>               cc = ZPCI_PCI_LS_ERR;
>               s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
> -        } else if (reg_ioat(env, pbdev->iommu, fib, ra)) {
> +        } else if (reg_ioat(env, pbdev, fib, ra)) {
>               cc = ZPCI_PCI_LS_ERR;
>               s390_set_status_code(env, r1, ZPCI_MOD_ST_INSUF_RES);
>           }
> @@ -1150,7 +1151,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>               s390_set_status_code(env, r1, ZPCI_MOD_ST_SEQUENCE);
>           } else {
>               pci_dereg_ioat(pbdev->iommu);
> -            if (reg_ioat(env, pbdev->iommu, fib, ra)) {
> +            if (reg_ioat(env, pbdev, fib, ra)) {
>                   cc = ZPCI_PCI_LS_ERR;
>                   s390_set_status_code(env, r1, ZPCI_MOD_ST_INSUF_RES);
>               }
> 
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 3/4] s390x/pci: use the passthrough measurement update interval

[Pierre Morel]

On 12/2/21 17:41, Matthew Rosato wrote:
> We may have gotten a measurement update interval from the underlying host
> via vfio -- Use it to set the interval via which we update the function
> measurement block.
> 
> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-inst.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 11b7f6bfa1..07bab85ce5 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -1046,7 +1046,7 @@ static void fmb_update(void *opaque)
>                         sizeof(pbdev->fmb.last_update))) {
>           return;
>       }
> -    timer_mod(pbdev->fmb_timer, t + DEFAULT_MUI);
> +    timer_mod(pbdev->fmb_timer, t + pbdev->pci_group->zpci_group.mui);
>   }
>   
>   int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
> @@ -1204,7 +1204,8 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar,
>           }
>           pbdev->fmb_addr = fmb_addr;
>           timer_mod(pbdev->fmb_timer,
> -                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + DEFAULT_MUI);
> +                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> +                                    pbdev->pci_group->zpci_group.mui);
>           break;
>       }
>       default:
> 

Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[Pierre Morel]

On 12/2/21 17:41, Matthew Rosato wrote:
> The current default PCI group being used can technically collide with a
> real group ID passed from a hostdev.  Let's instead use a group ID that comes
> from a special pool that is architected to be reserved for simulated devices.

NIT: May be add that PCIFG between 0xF0 and 0xFF is specified for this 
reserved pool.


> 
> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   include/hw/s390x/s390-pci-bus.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index aa891c178d..2727e7bdef 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>   } ZpciFmb;
>   QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>   
> -#define ZPCI_DEFAULT_FN_GRP 0x20
> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>   typedef struct S390PCIGroup {
>       ClpRspQueryPciGrp zpci_group;
>       int id;
> 

Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>


-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 4/4] s390x/pci: add supported DT information to clp response

[Pierre Morel]

On 12/2/21 17:41, Matthew Rosato wrote:
> The DTSM is a mask that specifies which I/O Address Translation designation
> types are supported.  A linux guest today does not look at this field but

Even Linux is the most used guest it is not the only one so may be not 
mention Linux here.

> could in the future; let's advertise what QEMU actually supports.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c         | 1 +
>   hw/s390x/s390-pci-vfio.c        | 1 +
>   include/hw/s390x/s390-pci-bus.h | 1 +
>   include/hw/s390x/s390-pci-clp.h | 3 ++-
>   4 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1b51a72838..01b58ebc70 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -782,6 +782,7 @@ static void s390_pci_init_default_group(void)
>       resgrp->i = 128;
>       resgrp->maxstbl = 128;
>       resgrp->version = 0;
> +    resgrp->dtsm = ZPCI_DTSM;

OK

>   }
>   
>   static void set_pbdev_info(S390PCIBusDevice *pbdev)
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index 2a153fa8c9..6f80a47e29 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -160,6 +160,7 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
>           resgrp->i = cap->noi;
>           resgrp->maxstbl = cap->maxstbl;
>           resgrp->version = cap->version;
> +        resgrp->dtsm = ZPCI_DTSM;

Is it safe for VFIO whith interpretation?
Shouldn't we extend the capability and use the host DTSM in this case?

...snip...

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH 4/4] s390x/pci: add supported DT information to clp response

[Matthew Rosato]

On 12/3/21 4:33 AM, Pierre Morel wrote:
> 
> 
> On 12/2/21 17:41, Matthew Rosato wrote:
>> The DTSM is a mask that specifies which I/O Address Translation 
>> designation
>> types are supported.  A linux guest today does not look at this field but
> 
> Even Linux is the most used guest it is not the only one so may be not 
> mention Linux here.
> 

OK

>> could in the future; let's advertise what QEMU actually supports.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.c         | 1 +
>>   hw/s390x/s390-pci-vfio.c        | 1 +
>>   include/hw/s390x/s390-pci-bus.h | 1 +
>>   include/hw/s390x/s390-pci-clp.h | 3 ++-
>>   4 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>> index 1b51a72838..01b58ebc70 100644
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -782,6 +782,7 @@ static void s390_pci_init_default_group(void)
>>       resgrp->i = 128;
>>       resgrp->maxstbl = 128;
>>       resgrp->version = 0;
>> +    resgrp->dtsm = ZPCI_DTSM;
> 
> OK
> 
>>   }
>>   static void set_pbdev_info(S390PCIBusDevice *pbdev)
>> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
>> index 2a153fa8c9..6f80a47e29 100644
>> --- a/hw/s390x/s390-pci-vfio.c
>> +++ b/hw/s390x/s390-pci-vfio.c
>> @@ -160,6 +160,7 @@ static void s390_pci_read_group(S390PCIBusDevice 
>> *pbdev,
>>           resgrp->i = cap->noi;
>>           resgrp->maxstbl = cap->maxstbl;
>>           resgrp->version = cap->version;
>> +        resgrp->dtsm = ZPCI_DTSM;
> 
> Is it safe for VFIO whith interpretation?
> Shouldn't we extend the capability and use the host DTSM in this case?

We will do exactly this when there is an interpretation series.

For the current intercept-based code, QEMU only supports DT 1 for 
passthrough regardless of what the host is supporting and rejects all 
else (see check in reg_ioat)

Re: [PATCH 4/4] s390x/pci: add supported DT information to clp response

[Matthew Rosato]

On 12/2/21 11:41 AM, Matthew Rosato wrote:
> The DTSM is a mask that specifies which I/O Address Translation designation
> types are supported.  A linux guest today does not look at this field but
> could in the future; let's advertise what QEMU actually supports.

Will send a v2, this patch is missing a line in clp_service_call to copy 
the byte into the guest payload (forgot it's not a memcpy anymore)

> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c         | 1 +
>   hw/s390x/s390-pci-vfio.c        | 1 +
>   include/hw/s390x/s390-pci-bus.h | 1 +
>   include/hw/s390x/s390-pci-clp.h | 3 ++-
>   4 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 1b51a72838..01b58ebc70 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -782,6 +782,7 @@ static void s390_pci_init_default_group(void)
>       resgrp->i = 128;
>       resgrp->maxstbl = 128;
>       resgrp->version = 0;
> +    resgrp->dtsm = ZPCI_DTSM;
>   }
>   
>   static void set_pbdev_info(S390PCIBusDevice *pbdev)
> diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
> index 2a153fa8c9..6f80a47e29 100644
> --- a/hw/s390x/s390-pci-vfio.c
> +++ b/hw/s390x/s390-pci-vfio.c
> @@ -160,6 +160,7 @@ static void s390_pci_read_group(S390PCIBusDevice *pbdev,
>           resgrp->i = cap->noi;
>           resgrp->maxstbl = cap->maxstbl;
>           resgrp->version = cap->version;
> +        resgrp->dtsm = ZPCI_DTSM;
>       }
>   }
>   
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index 2727e7bdef..da3cde2bb4 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -37,6 +37,7 @@
>   #define ZPCI_MAX_UID 0xffff
>   #define UID_UNDEFINED 0
>   #define UID_CHECKING_ENABLED 0x01
> +#define ZPCI_DTSM 0x40
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(S390pciState, S390_PCI_HOST_BRIDGE)
>   OBJECT_DECLARE_SIMPLE_TYPE(S390PCIBus, S390_PCI_BUS)
> diff --git a/include/hw/s390x/s390-pci-clp.h b/include/hw/s390x/s390-pci-clp.h
> index 96b8e3f133..cc8c8662b8 100644
> --- a/include/hw/s390x/s390-pci-clp.h
> +++ b/include/hw/s390x/s390-pci-clp.h
> @@ -163,7 +163,8 @@ typedef struct ClpRspQueryPciGrp {
>       uint8_t fr;
>       uint16_t maxstbl;
>       uint16_t mui;
> -    uint64_t reserved3;
> +    uint8_t dtsm;
> +    uint8_t reserved3[7];
>       uint64_t dasm; /* dma address space mask */
>       uint64_t msia; /* MSI address */
>       uint64_t reserved4;
> 

Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[David Hildenbrand]

On 03.12.21 03:25, Matthew Rosato wrote:
> On 12/2/21 6:06 PM, Halil Pasic wrote:
>> On Thu, 2 Dec 2021 12:11:38 -0500
>> Matthew Rosato <mjrosato@linux.ibm.com> wrote:
>>
>>>>
>>>> What happens if we migrate a VM from old to new QEMU? Won't the guest be
>>>> able to observe the change?
>>>>    
>>>
>>> Yes, technically --  But # itself is not really all that important, it
>>> is provided from CLP Q PCI FN to be subsequently used as input into Q
>>> PCI FNGRP -- With the fundamental notion being that all functions that
>>> share the same group # share the same group CLP info.  Whether the
>>> number is, say, 1 or 5 doesn't matter so much.
>>>
>>> However..  0xF0 and greater are the only values reserved for hypervisor
>>> use.  By using 0x20 we run the risk of accidentally conflating simulated
>>> devices and real hardware, hence the desire to change it.
>>>
>>> Is your concern about a migrated guest with a virtio device trying to do
>>> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could
>>> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch
>>> simulated devices trying to use something other than the default group,
>>> e.g.:
>>>
>>> if ((pbdev->fh & FH_SHM_EMUL) &&
>>>       (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>>>           /* Simulated device MUST have default group */
>>> 	pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
>>> 	group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
>>> }
>>>
>>> What do you think?
>>
>> Another option, and in my opinion the cleaner one would be to tie this
>> change to a new machine version. That is if a post-change qemu is used
>> in compatibility mode, we would still have the old behavior.
>>
>> What do you think?
>>
> 
> The problem there is that the old behavior goes against the architecture 
> (group 0x20 could belong to real hardware) and AFAIU assigning this new 
> behavior only to a new machine version means we can't fix old stable 
> QEMU versions.
> 
> Also, wait a minute -- migration isn't even an option right now, it's 
> blocked for zpci devices, both passthrough and simulated (see 
> aede5d5dfc5f 's390x/pci: mark zpci devices as unmigratable') so I say 
> let's just move to a proper default group now before we potentially 
> allow migration later.

Perfect, thanks for confirming!


-- 
Thanks,

David / dhildenb