Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

On 13/11/2017 12:48, Cornelia Huck wrote:

On Mon, 13 Nov 2017 10:03:37 +0100
Pierre Morel  wrote:


On 09/11/2017 17:50, Cornelia Huck wrote:

On Tue,  7 Nov 2017 18:24:34 +0100
Pierre Morel  wrote:



+case PCI_ROM_SLOT:


So, will this be filled in a later patch? (Reading from the top.)


No it will not it is only a reminder that we explicitly do not support it.
Since I do not know if we ever will support it do you think I better let
it fall?


Either add a comment, or drop it. Otherwise I will ask again in the
future :)



OK Ill do the easier, just suppress it and its brother in PCI LOAD. :)

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Cornelia Huck]

On Mon, 13 Nov 2017 10:03:37 +0100
Pierre Morel  wrote:

> On 09/11/2017 17:50, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:34 +0100
> > Pierre Morel  wrote:

> >> +case PCI_ROM_SLOT:  
> > 
> > So, will this be filled in a later patch? (Reading from the top.)  
> 
> No it will not it is only a reminder that we explicitly do not support it.
> Since I do not know if we ever will support it do you think I better let 
> it fall?

Either add a comment, or drop it. Otherwise I will ask again in the
future :)

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

On 09/11/2017 17:50, Cornelia Huck wrote:

On Tue,  7 Nov 2017 18:24:34 +0100
Pierre Morel  wrote:


Enhance the fault detection, correction of the fault reporting.

Signed-off-by: Pierre Morel 
Reviewed-by: Yi Min Zhao 
---
  hw/s390x/s390-pci-inst.c | 41 -
  1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8fcb02d..4a2f996 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
  pcias = (env->regs[r2] >> 16) & 0xf;
  len = env->regs[r2] & 0xf;
  offset = env->regs[r2 + 1];
+data = env->regs[r1];
+
+if (!(fh & FH_MASK_ENABLE)) {


This covers the reserved/standby/disabled states, right?


yes it does, all these states have the FH_MASK_ENABLE set to 0




+setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
+return 0;
+}
  
  pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);

  if (!pbdev) {
@@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
  }
  
  switch (pbdev->state) {

-case ZPCI_FS_RESERVED:
-case ZPCI_FS_STANDBY:
-case ZPCI_FS_DISABLED:
  case ZPCI_FS_PERMANENT_ERROR:
-setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-return 0;
  case ZPCI_FS_ERROR:
  setcc(cpu, ZPCI_PCI_LS_ERR);
  s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
@@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
  break;
  }
  
-data = env->regs[r1];

-if (pcias < 6) {
-if ((8 - (offset & 0x7)) < len) {
+/* Test that the pcias is a valid pcias and do the according operations */


s/according/appropriate/ ?


yes, better, thanks




+switch (pcias) {
+case 0 ... 5:
+/* Check length:
+ * A length of 0 is invalid and length should not cross a double word
+ */
+if (!len || (len > (8 - (offset & 0x7 {
  program_interrupt(env, PGM_OPERAND, 4);
  return 0;
  }
@@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
  program_interrupt(env, PGM_OPERAND, 4);
  return 0;
  }
-} else if (pcias == 15) {
-if ((4 - (offset & 0x3)) < len) {
-program_interrupt(env, PGM_OPERAND, 4);
-return 0;
-}
-
-if (zpci_endian_swap(, len)) {
+break;
+case 15:
+/* ZPCI uses the pseudo BAR number 15 as configuration space */
+/* possible access length are 1,2,4 and must not cross a word */


s/length/lengths/

yes, thanks




+if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
  program_interrupt(env, PGM_OPERAND, 4);
  return 0;
  }
-
+/* len = 1,2,4 so we do not need to test */
+zpci_endian_swap(, len);
  pci_host_config_write_common(pbdev->pdev, offset,
   pci_config_size(pbdev->pdev),
   data, len);
-} else {
+break;
+case PCI_ROM_SLOT:


So, will this be filled in a later patch? (Reading from the top.)


No it will not it is only a reminder that we explicitly do not support it.
Since I do not know if we ever will support it do you think I better let 
it fall?





+/* Fall through */
+default:
  DPRINTF("pcistg invalid space\n");
  setcc(cpu, ZPCI_PCI_LS_ERR);
  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);


On the whole, I think this improves readability, especially for folks
that do not have access to the spec.




--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Yi Min Zhao]

在 2017/11/10 上午12:50, Cornelia Huck 写道:

On Tue,  7 Nov 2017 18:24:34 +0100
Pierre Morel  wrote:


Enhance the fault detection, correction of the fault reporting.

Signed-off-by: Pierre Morel 
Reviewed-by: Yi Min Zhao 
---
  hw/s390x/s390-pci-inst.c | 41 -
  1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8fcb02d..4a2f996 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
  pcias = (env->regs[r2] >> 16) & 0xf;
  len = env->regs[r2] & 0xf;
  offset = env->regs[r2 + 1];
+data = env->regs[r1];
+
+if (!(fh & FH_MASK_ENABLE)) {

This covers the reserved/standby/disabled states, right?

yes

[...]

Re: [Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Cornelia Huck]

On Tue,  7 Nov 2017 18:24:34 +0100
Pierre Morel  wrote:

> Enhance the fault detection, correction of the fault reporting.
> 
> Signed-off-by: Pierre Morel 
> Reviewed-by: Yi Min Zhao 
> ---
>  hw/s390x/s390-pci-inst.c | 41 -
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8fcb02d..4a2f996 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, 
> uint8_t r2)
>  pcias = (env->regs[r2] >> 16) & 0xf;
>  len = env->regs[r2] & 0xf;
>  offset = env->regs[r2 + 1];
> +data = env->regs[r1];
> +
> +if (!(fh & FH_MASK_ENABLE)) {

This covers the reserved/standby/disabled states, right?

> +setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> +return 0;
> +}
>  
>  pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
>  if (!pbdev) {
> @@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, 
> uint8_t r2)
>  }
>  
>  switch (pbdev->state) {
> -case ZPCI_FS_RESERVED:
> -case ZPCI_FS_STANDBY:
> -case ZPCI_FS_DISABLED:
>  case ZPCI_FS_PERMANENT_ERROR:
> -setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> -return 0;
>  case ZPCI_FS_ERROR:
>  setcc(cpu, ZPCI_PCI_LS_ERR);
>  s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
> @@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, 
> uint8_t r2)
>  break;
>  }
>  
> -data = env->regs[r1];
> -if (pcias < 6) {
> -if ((8 - (offset & 0x7)) < len) {
> +/* Test that the pcias is a valid pcias and do the according operations 
> */

s/according/appropriate/ ?

> +switch (pcias) {
> +case 0 ... 5:
> +/* Check length:
> + * A length of 0 is invalid and length should not cross a double word
> + */
> +if (!len || (len > (8 - (offset & 0x7 {
>  program_interrupt(env, PGM_OPERAND, 4);
>  return 0;
>  }
> @@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, 
> uint8_t r2)
>  program_interrupt(env, PGM_OPERAND, 4);
>  return 0;
>  }
> -} else if (pcias == 15) {
> -if ((4 - (offset & 0x3)) < len) {
> -program_interrupt(env, PGM_OPERAND, 4);
> -return 0;
> -}
> -
> -if (zpci_endian_swap(, len)) {
> +break;
> +case 15:
> +/* ZPCI uses the pseudo BAR number 15 as configuration space */
> +/* possible access length are 1,2,4 and must not cross a word */

s/length/lengths/

> +if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
>  program_interrupt(env, PGM_OPERAND, 4);
>  return 0;
>  }
> -
> +/* len = 1,2,4 so we do not need to test */
> +zpci_endian_swap(, len);
>  pci_host_config_write_common(pbdev->pdev, offset,
>   pci_config_size(pbdev->pdev),
>   data, len);
> -} else {
> +break;
> +case PCI_ROM_SLOT:

So, will this be filled in a later patch? (Reading from the top.)

> +/* Fall through */
> +default:
>  DPRINTF("pcistg invalid space\n");
>  setcc(cpu, ZPCI_PCI_LS_ERR);
>  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);

On the whole, I think this improves readability, especially for folks
that do not have access to the spec.

[Qemu-devel] [PATCH 2/7] s390x/pci: rework PCI STORE

[Pierre Morel]

Enhance the fault detection, correction of the fault reporting.

Signed-off-by: Pierre Morel 
Reviewed-by: Yi Min Zhao 
---
 hw/s390x/s390-pci-inst.c | 41 -
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8fcb02d..4a2f996 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -469,6 +469,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
 pcias = (env->regs[r2] >> 16) & 0xf;
 len = env->regs[r2] & 0xf;
 offset = env->regs[r2 + 1];
+data = env->regs[r1];
+
+if (!(fh & FH_MASK_ENABLE)) {
+setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
+return 0;
+}
 
 pbdev = s390_pci_find_dev_by_fh(s390_get_phb(), fh);
 if (!pbdev) {
@@ -478,12 +484,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
 }
 
 switch (pbdev->state) {
-case ZPCI_FS_RESERVED:
-case ZPCI_FS_STANDBY:
-case ZPCI_FS_DISABLED:
 case ZPCI_FS_PERMANENT_ERROR:
-setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-return 0;
 case ZPCI_FS_ERROR:
 setcc(cpu, ZPCI_PCI_LS_ERR);
 s390_set_status_code(env, r2, ZPCI_PCI_ST_BLOCKED);
@@ -492,9 +493,13 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
 break;
 }
 
-data = env->regs[r1];
-if (pcias < 6) {
-if ((8 - (offset & 0x7)) < len) {
+/* Test that the pcias is a valid pcias and do the according operations */
+switch (pcias) {
+case 0 ... 5:
+/* Check length:
+ * A length of 0 is invalid and length should not cross a double word
+ */
+if (!len || (len > (8 - (offset & 0x7 {
 program_interrupt(env, PGM_OPERAND, 4);
 return 0;
 }
@@ -512,21 +517,23 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t 
r2)
 program_interrupt(env, PGM_OPERAND, 4);
 return 0;
 }
-} else if (pcias == 15) {
-if ((4 - (offset & 0x3)) < len) {
-program_interrupt(env, PGM_OPERAND, 4);
-return 0;
-}
-
-if (zpci_endian_swap(, len)) {
+break;
+case 15:
+/* ZPCI uses the pseudo BAR number 15 as configuration space */
+/* possible access length are 1,2,4 and must not cross a word */
+if (!len || (len > (4 - (offset & 0x3))) || len == 3) {
 program_interrupt(env, PGM_OPERAND, 4);
 return 0;
 }
-
+/* len = 1,2,4 so we do not need to test */
+zpci_endian_swap(, len);
 pci_host_config_write_common(pbdev->pdev, offset,
  pci_config_size(pbdev->pdev),
  data, len);
-} else {
+break;
+case PCI_ROM_SLOT:
+/* Fall through */
+default:
 DPRINTF("pcistg invalid space\n");
 setcc(cpu, ZPCI_PCI_LS_ERR);
 s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);
-- 
2.7.4