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

2017-11-22 Thread Pierre Morel 

On 22/11/2017 14:25, Cornelia Huck wrote:

On Tue, 21 Nov 2017 19:03:17 +0100
Pierre Morel  wrote:


On 21/11/2017 15:25, Cornelia Huck wrote:

On Tue, 21 Nov 2017 11:38:45 +0100
Cornelia Huck  wrote:
   

On Thu, 16 Nov 2017 18:51:50 +0100
Pierre Morel  wrote:
   

@@ -493,9 +494,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 valid and do the appropriates operations */
+switch (pcias) {
+case 0 ... 5:


Make this
case 0...5:
?


...only that it confuses the compiler when using numbers. We can either


I did not see this as I replied to the previous email, sorry.


keep the slightly ugly version, or introduce #defines.
ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?



I agree something is missing.
But I am not sure that a #define brings clarity.
I would prefer to add a comment.
/* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */

?


It's more a speaking value vs. magic numbers thing. A comment does not
hurt either, though :)

(And we get rid of the spacing as well.)


OK, thanks.







+ * 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;
   }
@@ -513,21 +518,21 @@ 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 lengths 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;
+default:
   DPRINTF("pcistg invalid space\n");
   setcc(cpu, ZPCI_PCI_LS_ERR);
   s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);


Other than the nits, looks good.
   








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

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

2017-11-22 Thread Cornelia Huck 
On Tue, 21 Nov 2017 19:03:17 +0100
Pierre Morel  wrote:

> On 21/11/2017 15:25, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 11:38:45 +0100
> > Cornelia Huck  wrote:
> >   
> >> On Thu, 16 Nov 2017 18:51:50 +0100
> >> Pierre Morel  wrote:  
> >   
> >>> @@ -493,9 +494,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 valid and do the appropriates operations */
> >>> +switch (pcias) {
> >>> +case 0 ... 5:  
> >>
> >> Make this
> >> case 0...5:
> >> ?  
> > 
> > ...only that it confuses the compiler when using numbers. We can either  
> 
> I did not see this as I replied to the previous email, sorry.
> 
> > keep the slightly ugly version, or introduce #defines.
> > ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?  
> 
> 
> I agree something is missing.
> But I am not sure that a #define brings clarity.
> I would prefer to add a comment.
>   /* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */
> 
> ?

It's more a speaking value vs. magic numbers thing. A comment does not
hurt either, though :)

(And we get rid of the spacing as well.)

> 
> 
> >>> + * 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;
> >>>   }
> >>> @@ -513,21 +518,21 @@ 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 lengths 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;
> >>> +default:
> >>>   DPRINTF("pcistg invalid space\n");
> >>>   setcc(cpu, ZPCI_PCI_LS_ERR);
> >>>   s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);  
> >>
> >> Other than the nits, looks good.  
> >   
> 
>

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

2017-11-21 Thread Pierre Morel 

On 21/11/2017 15:25, Cornelia Huck wrote:

On Tue, 21 Nov 2017 11:38:45 +0100
Cornelia Huck  wrote:


On Thu, 16 Nov 2017 18:51:50 +0100
Pierre Morel  wrote:



@@ -493,9 +494,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 valid and do the appropriates operations */
+switch (pcias) {
+case 0 ... 5:


Make this
case 0...5:
?


...only that it confuses the compiler when using numbers. We can either


I did not see this as I replied to the previous email, sorry.


keep the slightly ugly version, or introduce #defines.
ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?



I agree something is missing.
But I am not sure that a #define brings clarity.
I would prefer to add a comment.
/* A ZPCI PCI card may use any BAR from BAR 0 to BAR 5 */

?



+ * 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;
  }
@@ -513,21 +518,21 @@ 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 lengths 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;
+default:
  DPRINTF("pcistg invalid space\n");
  setcc(cpu, ZPCI_PCI_LS_ERR);
  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);


Other than the nits, looks good.





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

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

2017-11-21 Thread Pierre Morel 

On 21/11/2017 11:38, Cornelia Huck wrote:

On Thu, 16 Nov 2017 18:51:50 +0100
Pierre Morel  wrote:


Enhance the fault detection, correction of the fault reporting.

Signed-off-by: Pierre Morel 
Reviewed-by: Yi Min Zhao 
Signed-off-by: Pierre Morel 


Same double s-o-b.


removed.
Thanks




---
  hw/s390x/s390-pci-inst.c | 39 ++-
  1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index ded1556..df0c8f0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -470,6 +470,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) {
@@ -479,12 +485,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);
@@ -493,9 +494,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 valid and do the appropriates operations */
+switch (pcias) {
+case 0 ... 5:


Make this
case 0...5:
?


Compiler complains.
a blanc around the three points seems right.





+/* 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;
  }
@@ -513,21 +518,21 @@ 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 lengths 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;
+default:
  DPRINTF("pcistg invalid space\n");
  setcc(cpu, ZPCI_PCI_LS_ERR);
  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);


Other than the nits, looks good.




Thanks for reviewing, I remove the double signature

Regards,

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

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

2017-11-21 Thread Cornelia Huck 
On Tue, 21 Nov 2017 11:38:45 +0100
Cornelia Huck  wrote:

> On Thu, 16 Nov 2017 18:51:50 +0100
> Pierre Morel  wrote:

> > @@ -493,9 +494,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 valid and do the appropriates operations */
> > +switch (pcias) {
> > +case 0 ... 5:  
> 
> Make this
> case 0...5:
> ?

...only that it confuses the compiler when using numbers. We can either
keep the slightly ugly version, or introduce #defines.
ZPCI_PCIAS_IOREGION_{MIN,MAX} (and maybe ZPCI_PCIAS_CONFIG for 15)?

> 
> > +/* 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;
> >  }
> > @@ -513,21 +518,21 @@ 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 lengths 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;
> > +default:
> >  DPRINTF("pcistg invalid space\n");
> >  setcc(cpu, ZPCI_PCI_LS_ERR);
> >  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);  
> 
> Other than the nits, looks good.

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

2017-11-21 Thread Cornelia Huck 
On Thu, 16 Nov 2017 18:51:50 +0100
Pierre Morel  wrote:

> Enhance the fault detection, correction of the fault reporting.
> 
> Signed-off-by: Pierre Morel 
> Reviewed-by: Yi Min Zhao 
> Signed-off-by: Pierre Morel 

Same double s-o-b.

> ---
>  hw/s390x/s390-pci-inst.c | 39 ++-
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index ded1556..df0c8f0 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -470,6 +470,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) {
> @@ -479,12 +485,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);
> @@ -493,9 +494,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 valid and do the appropriates operations */
> +switch (pcias) {
> +case 0 ... 5:

Make this
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;
>  }
> @@ -513,21 +518,21 @@ 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 lengths 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;
> +default:
>  DPRINTF("pcistg invalid space\n");
>  setcc(cpu, ZPCI_PCI_LS_ERR);
>  s390_set_status_code(env, r2, ZPCI_PCI_ST_INVAL_AS);

Other than the nits, looks good.

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

2017-11-16 Thread Pierre Morel 
Enhance the fault detection, correction of the fault reporting.

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

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index ded1556..df0c8f0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -470,6 +470,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) {
@@ -479,12 +485,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);
@@ -493,9 +494,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 valid and do the appropriates 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;
 }
@@ -513,21 +518,21 @@ 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 lengths 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;
+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