Re: [PATCH 3/7] AMD IOMMU emulation

[Blue Swirl <blauwirbel@gmail.com>]

On Sat, Aug 28, 2010 at 9:53 PM, Eduard - Gabriel Munteanu
<eduard.munteanu@linux360.ro> wrote:
> On Sat, Aug 28, 2010 at 03:58:23PM +0000, Blue Swirl wrote:
>> On Sat, Aug 28, 2010 at 2:54 PM, Eduard - Gabriel Munteanu
>> <eduard.munteanu@linux360.ro> wrote:
>> > This introduces emulation for the AMD IOMMU, described in "AMD I/O
>> > Virtualization Technology (IOMMU) Specification".
>> >
>> > Signed-off-by: Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>
>> > ---
>
> [snip]
>
>> > diff --git a/hw/amd_iommu.c b/hw/amd_iommu.c
>> > new file mode 100644
>> > index 0000000..43e0426
>> > --- /dev/null
>> > +++ b/hw/amd_iommu.c
>
> [snip]
>
>> > +static void amd_iommu_update_mmio(AMDIOMMUState *st,
>> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ?? ??target_phys_addr_t addr)
>> > +{
>> > + ?? ??size_t reg = addr & ~0x07;
>> > + ?? ??uint64_t *base = (uint64_t *) &st->mmio_buf[reg];
>>
>> This is still buggy.
>>
>> > + ?? ??uint64_t val = le64_to_cpu(*base);
>
> mmio_buf is always LE, so a BE host will have *base in reversed
> byteorder. But look at the next line, where I did the le64_to_cpu().
> That should swap the bytes on a BE host, yielding the correct byteorder.

Sorry, I  missed that one when comparing the patch to previous version.

> On a LE host, *base is right the first time and le64_to_cpu() is a nop.
>
> In any case, I only use 'val', not '*base' directly. I suppose it could
> be rewritten for clarity (i.e. ditch 'base').

Yes, someone could add more code later which accidentally uses 'base' directly.

> Do you still think it's wrong? Or is it for another reason?

I think it's OK for now. The rewrite can happen with a small patch later.