On Tue, 21 Nov 2017 19:03:17 +0100 Pierre Morel <pmo...@linux.vnet.ibm.com> wrote:
> On 21/11/2017 15:25, Cornelia Huck wrote: > > On Tue, 21 Nov 2017 11:38:45 +0100 > > Cornelia Huck <coh...@redhat.com> wrote: > > > >> On Thu, 16 Nov 2017 18:51:50 +0100 > >> Pierre Morel <pmo...@linux.vnet.ibm.com> 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(&data, 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(&data, 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. > > > >