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

[Pierre Morel]

On 01/02/2018 13:15, Cornelia Huck wrote:
> On Thu, 1 Feb 2018 12:56:01 +0100
> Pierre Morel <address@hidden> 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 <address@hidden> 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 :)
I don't think so, I think it was really an implementation error in the
previous code because we have no way to return an error from the
translate() function, we just return the TLB entry.

If we have an R or W or RW entry we return the entry in all other cases,
including a RW protected, invalid or on internal error entry we return 
IOMMU_NONE

IMHO it would have been clearer if this function have been void.

Regards,

Pierre

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