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

[Cornelia Huck]

On Tue, 21 Nov 2017 19:03:17 +0100
Pierre Morel <address@hidden> wrote:

> On 21/11/2017 15:25, Cornelia Huck wrote:
> > On Tue, 21 Nov 2017 11:38:45 +0100
> > Cornelia Huck <address@hidden> wrote:
> >   
> >> On Thu, 16 Nov 2017 18:51:50 +0100
> >> Pierre Morel <address@hidden> 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.  
> >   
> 
>