[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/8] s390: Add channel I/O instructions.
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [PATCH 4/8] s390: Add channel I/O instructions. |
Date: |
Tue, 11 Dec 2012 13:53:51 +0100 |
On Tue, 11 Dec 2012 11:18:44 +0100
Alexander Graf <address@hidden> wrote:
>
> On 10.12.2012, at 10:18, Cornelia Huck wrote:
>
> > On Mon, 10 Dec 2012 10:00:16 +0100
> > Alexander Graf <address@hidden> wrote:
> >
> >>
> >>
> >> On 07.12.2012, at 13:50, Cornelia Huck <address@hidden> wrote:
> >>> +/* Special handling for the prefix page. */
> >>> +static void *s390_get_address(CPUS390XState *env, ram_addr_t guest_addr)
> >>> +{
> >>> + if (guest_addr < 8192) {
> >>> + guest_addr += env->psa;
> >>> + } else if ((env->psa <= guest_addr) && (guest_addr < env->psa +
> >>> 8192)) {
> >>> + guest_addr -= env->psa;
> >>> + }
> >>> +
> >>> + return qemu_get_ram_ptr(guest_addr);
> >>
> >> Do we actually need this?
> >
> > Yes. I've seen failures for I/O instructions using the lowcore (which
> > the Linux kernel likes to do).
>
> Then we want an s390 generic function that does this, not an io specific one
> though, right? Also qemu_get_ram_ptr is a no-go, as it doesn't do boundary
> checks.
Oh, wasn't aware of that.
>
> So what we really want is something like s390_cpu_physical_memory_map(env,
> ...) with a special case on the lowcore.
Let's see how this works out.
> >>> + addr = ipb >> 28;
> >>> + if (addr > 0) {
> >>> + addr = env->regs[addr];
> >>> + }
> >>> + addr += (ipb & 0xfff0000) >> 16;
> >>
> >> This adds the upper bits twice. Are you sire that's correct?
> >
> > If addr was 0, it doesn't. If addr was > 0 before, we grabbed the
> > address from the corresponding register and want to add to it.
>
> This is a very confusing way of writing what you're trying to express then
> :). How about
>
> hwaddr addr = 0;
>
> reg = ipb >> 28;
> if (reg) {
> addr = env->regs[reg];
> }
> addr += (ipb >> 16) & 0xfff0;
I've moved this to a helper function anyway - but this looks a bit more
readable, yes.
- [Qemu-devel] [PATCH 1/8] Update linux headers., (continued)
[Qemu-devel] [PATCH 7/8] s390-virtio: Factor out some initialization code., Cornelia Huck, 2012/12/07
[Qemu-devel] [PATCH 8/8] s390: Add new channel I/O based virtio transport., Cornelia Huck, 2012/12/07
[Qemu-devel] [PATCH 5/8] s390: Virtual channel subsystem support., Cornelia Huck, 2012/12/07
[Qemu-devel] [PATCH 3/8] s390: I/O interrupt and machine check injection., Cornelia Huck, 2012/12/07