[Qemu-devel] Fwd: QEMU: AMD IOMMU implementation bugs

[Qemu-devel] Fwd: QEMU: AMD IOMMU implementation bugs

[David Kiarie]

---------- Forwarded message ---------
From: Alexandro Sánchez Bach <alexaltea123@gmail.com>
Date: Wed, Jan 31, 2018 at 2:29 AM
Subject: QEMU: AMD IOMMU implementation bugs
To: <davidkiarie4@gmail.com>


Hey David,

hello Mr Alexandro Sanchez Bach,

I'm working with your AMD IOMMU implementation since I'm writing a
PlayStation 4 emulator. I found few bugs, that I wanted to report to you.

1. https://github.com/qemu/qemu/blob/master/hw/i386/amd_iommu.c#L380
Did you mean amdvi_assign_orq instead of  amdvi_test_mask? Otherwise my
guest OS will get stuck in an endless loop waiting for a flag that never
comes.

it looks like you're correct to me.

2. The arguments `addr` and `val` are swapped in these two lines:
> static void amdvi_writeq_raw(AMDVIState *s, uint64_t val, hwaddr addr);
> amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) | val);

same here.

3. And this might be something specific to my AMD IOMMU (1022:1437, Family
16h), but the PS4 OS is checks the entire word stored in these registers
and computes the expression as:
config[AMDVI_CAPAB_BAR_LOW] | cap[AMDVI_CAPAB_BAR_HIGH] << 32
So just storing half words should be wrong.

not sure about this.

since it looks to me like you're working with emulation and virtualization,
i have cc'd people who i know/knew to be actively involved. it doubt it
would take any effort for these people to respond to bugs like above in the
future.

Cheers,
Alex

Re: [Qemu-devel] QEMU: AMD IOMMU implementation bugs

[David Kiarie]

On Sat, Feb 16, 2019 at 10:11 PM David Kiarie <davidkiarie4@gmail.com>
wrote:

>
>
> ---------- Forwarded message ---------
> From: Alexandro Sánchez Bach <alexaltea123@gmail.com>
> Date: Wed, Jan 31, 2018 at 2:29 AM
> Subject: QEMU: AMD IOMMU implementation bugs
> To: <davidkiarie4@gmail.com>
>
>
> Hey David,
>
> hello Mr Alexandro Sanchez Bach,
>
> I'm working with your AMD IOMMU implementation since I'm writing a
> PlayStation 4 emulator. I found few bugs, that I wanted to report to you.
>
> 1. https://github.com/qemu/qemu/blob/master/hw/i386/amd_iommu.c#L380
> Did you mean amdvi_assign_orq instead of  amdvi_test_mask? Otherwise my
> guest OS will get stuck in an endless loop waiting for a flag that never
> comes.
>
> it looks like you're correct to me.
>
> 2. The arguments `addr` and `val` are swapped in these two lines:
> > static void amdvi_writeq_raw(AMDVIState *s, uint64_t val, hwaddr addr);
> > amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) | val);
>
> same here.
>
> 3. And this might be something specific to my AMD IOMMU (1022:1437, Family
> 16h), but the PS4 OS is checks the entire word stored in these registers
> and computes the expression as:
> config[AMDVI_CAPAB_BAR_LOW] | cap[AMDVI_CAPAB_BAR_HIGH] << 32
> So just storing half words should be wrong.
>
> not sure about this.
>
> since it looks to me like you're working with emulation and
> virtualization, i have cc'd people who i know/knew to be actively involved.
> it doubt it would take any effort for these people to respond to bugs like
> above in the future.
>

i believe these people even know enough about emulation and virtualization
even respond to "research-ish" questions, as PlayStation emulation could
come off a research project.

you probably just need to frame it in the right way.


> Cheers,
> Alex
>
>

Re: [Qemu-devel] Fwd: QEMU: AMD IOMMU implementation bugs

[Peter Xu]

On Sat, Feb 16, 2019 at 10:11:28PM +0300, David Kiarie wrote:
> ---------- Forwarded message ---------
> From: Alexandro Sánchez Bach <alexaltea123@gmail.com>
> Date: Wed, Jan 31, 2018 at 2:29 AM
> Subject: QEMU: AMD IOMMU implementation bugs
> To: <davidkiarie4@gmail.com>
> 
> 
> Hey David,
> 
> hello Mr Alexandro Sanchez Bach,
> 
> I'm working with your AMD IOMMU implementation since I'm writing a
> PlayStation 4 emulator. I found few bugs, that I wanted to report to you.
> 
> 1. https://github.com/qemu/qemu/blob/master/hw/i386/amd_iommu.c#L380
> Did you mean amdvi_assign_orq instead of  amdvi_test_mask? Otherwise my
> guest OS will get stuck in an endless loop waiting for a flag that never
> comes.
> 
> it looks like you're correct to me.
> 
> 2. The arguments `addr` and `val` are swapped in these two lines:
> > static void amdvi_writeq_raw(AMDVIState *s, uint64_t val, hwaddr addr);
> > amdvi_writeq_raw(s, addr, amdvi_readq(s, addr) | val);
> 
> same here.
> 
> 3. And this might be something specific to my AMD IOMMU (1022:1437, Family
> 16h), but the PS4 OS is checks the entire word stored in these registers
> and computes the expression as:
> config[AMDVI_CAPAB_BAR_LOW] | cap[AMDVI_CAPAB_BAR_HIGH] << 32
> So just storing half words should be wrong.
> 
> not sure about this.
> 
> since it looks to me like you're working with emulation and virtualization,
> i have cc'd people who i know/knew to be actively involved. it doubt it
> would take any effort for these people to respond to bugs like above in the
> future.

Hi, Alex,

The sentences are a bit messed up above.  IMHO you can simply post
patches directly if you found any bugs in the code.  You can prefix
the subject with "RFC" if you are uncertain about the changes.  People
on the list can directly comment on the patches.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] Fwd: QEMU: AMD IOMMU implementation bugs

[David Kiarie]

>
> >
> > 3. And this might be something specific to my AMD IOMMU (1022:1437,
> Family
> > 16h), but the PS4 OS is checks the entire word stored in these registers
> > and computes the expression as:
> > config[AMDVI_CAPAB_BAR_LOW] | cap[AMDVI_CAPAB_BAR_HIGH] << 32
> > So just storing half words should be wrong.
> >
>

also,  as much as try, i cannot exactly figure out what you're talking
about here and i'm under the impression i could not be the only person.

would you mind elaborating? :-)

Re: [Qemu-devel] Fwd: QEMU: AMD IOMMU implementation bugs

[David Kiarie]

On Mon, Feb 18, 2019 at 9:59 PM David Kiarie <davidkiarie4@gmail.com> wrote:

>
>
>> >
>> > 3. And this might be something specific to my AMD IOMMU (1022:1437,
>> Family
>> > 16h), but the PS4 OS is checks the entire word stored in these registers
>> > and computes the expression as:
>> > config[AMDVI_CAPAB_BAR_LOW] | cap[AMDVI_CAPAB_BAR_HIGH] << 32
>> > So just storing half words should be wrong.
>> >
>>
>
> also,  as much as try, i cannot exactly figure out what you're talking
> about here and i'm under the impression i could not be the only person.
>
> would you mind elaborating? :-)
>

i can't express how grateful i am about your report.

having being the author of the culprit code, it's a fair bit of
irresponsibility on my side for me to leave issue unresolved, though it
does look easy to fix. it was probably not as easy to "dig" out the bugs.

on the other hand, Mr Alexandro, it does look like Peter(who most probably
still actively works on the Qemu IOMMUs, both Intel and AMD)will be
positively considering any IOMMU issues from you in the future.

4 days has been a wait long enough and i had better be out.