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

[Cornelia Huck]

On Wed, 22 Nov 2017 13:15:21 +0800
Yi Min Zhao <address@hidden> wrote:

> 在 2017/11/22 上午2:07, Pierre Morel 写道:
> > On 21/11/2017 11:42, Cornelia Huck wrote:  
> >> On Thu, 16 Nov 2017 18:51:52 +0100
> >> Pierre Morel <address@hidden> wrote:
> >>  
> >>> Enhance the fault detection.
> >>>
> >>> Fixup the precedence to check the destination path existance
> >>> before checking for the source accessibility.
> >>>
> >>> Add the maxstbl entry to both the Query PCI Function Group
> >>> response and the PCIBusDevice structure.
> >>>
> >>> Initialize the maxstbl to 128 per default until we get
> >>> the actual data from the hardware.
> >>>
> >>> Signed-off-by: Pierre Morel <address@hidden>
> >>> Reviewed-by: Yi Min Zhao <address@hidden>
> >>> ---
> >>>   hw/s390x/s390-pci-bus.h  |  1 +
> >>>   hw/s390x/s390-pci-inst.c | 62 
> >>> +++++++++++++++++++++++++++++-------------------
> >>>   hw/s390x/s390-pci-inst.h |  2 +-
> >>>   3 files changed, 40 insertions(+), 25 deletions(-)  
> >>  
> >>> @@ -700,8 +685,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t 
> >>> r1, uint8_t r3, uint64_t gaddr,
> >>>           break;
> >>>       }
> >>>   +    if (pcias > 5) {
> >>> +        DPRINTF("pcistb invalid space\n");
> >>> +        setcc(cpu, ZPCI_PCI_LS_ERR);
> >>> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> >>> +        return 0;
> >>> +    }
> >>> +
> >>> +    /* Verify the address, offset and length */
> >>> +    /* offset must be a multiple of 8 */
> >>> +    if (offset % 8) {
> >>> +        goto addressing_error;
> >>> +    }
> >>> +    /* Length must be greater than 8, a multiple of 8, not greater 
> >>> maxstbl */  
> >>
> >> "not greater than maxstlb"  
> >
> > Better I know but greater that 80 characters, this is why I preferred 
> > broken English.
> > What do I do ? break the line or English ?  
> less than?

It's less or equal, no?

In any case, I prefer a multi-line comment over awkward language.

> >  
> >>  
> >>> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> >>> +        goto addressing_error;
> >>> +    }
> >>> +    /* Do not cross a 4K-byte boundary */
> >>> +    if (((offset & 0xfff) + len) > 0x1000) {
> >>> +        goto addressing_error;
> >>> +    }
> >>> +    /* Guest address must be double word aligned */
> >>> +    if (gaddr & 0x07UL) {
> >>> +        goto addressing_error;
> >>> +    }
> >>> +
> >>>       mr = pbdev->pdev->io_regions[pcias].memory;
> >>> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
> >>> +    if (!memory_region_access_valid(mr, offset, len, true)) {
> >>>           program_interrupt(env, PGM_OPERAND, 6);
> >>>           return 0;
> >>>       }  
> >>
> >> Looks good.
> >>  
> >
> >  
>