Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 1 Feb 2018 12:56:01 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 01/02/2018 12:28, Yi Min Zhao wrote:
> >
> >
> > 在 2018/1/31 下午6:58, Cornelia Huck 写道:  
> >> On Tue, 30 Jan 2018 10:47:13 +0100
> >> Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> >>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >>> index be449210d9..63fa06fb97 100644
> >>> --- a/hw/s390x/s390-pci-inst.c
> >>> +++ b/hw/s390x/s390-pci-inst.c
> >>> @@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t 
> >>> r1, uint8_t r2, uintptr_t ra)
> >>>         while (start < end) {
> >>>           entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
> >>> -
> >>> -        if (!entry.translated_addr) {
> >>> -            pbdev->state = ZPCI_FS_ERROR;
> >>> -            setcc(cpu, ZPCI_PCI_LS_ERR);
> >>> -            s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
> >>> -            s390_pci_generate_error_event(ERR_EVENT_SERR, 
> >>> pbdev->fh, pbdev->fid,
> >>> -                                          start, ERR_EVENT_Q_BIT);
> >>> -            goto out;
> >>> -        }
> >>> -
> >>>           memory_region_notify_iommu(iommu_mr, entry);
> >>>           start += entry.addr_mask + 1;  
> >> You're now progressing even though you might have generated an error
> >> event. Is that what's intended?  
> > Yes, this is wrong. The right thing is only delete the code generating 
> > error event,
> > and keep the if check here in this patch.  
> 
> Hum, I do not see any problem with having a translated address being 0.
> 
> I would say it is one of the fixup ;) .

Isn't this in response to an instruction that is supposed to
register/... something? IOW, the caller of the instruction tried to
make something happen, but something did not work out as intended (and
doesn't the code stumble over this later? Or do we get an error then?)
Shouldn't the caller get an indication of that? Of course, this again
depends on what the architecture says :)