Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Jordan Hargrave]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Referencing this old thread:
https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00606.html

I've run into an issue recently with testing q35 DMAR/intel iommu with ahci
driver.  My ahci driver writes the upper-32 bits (PORT_FIS_ADDR_HI) first
then the lower 32-bits (PORT_FIS_ADDR).

The contents of PORT_FIS_ADDR therefore are stale when the PORT_FIS_ADDR_HI
write calls map_page().  DMAR translation fails at this point as the old
stale address (from SEABIOS initialization) is not in the DMAR page table.

[-- Attachment #2: Type: text/html, Size: 671 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Wed, Feb 25, 2015 at 11:13:09PM -0600, Jordan Hargrave wrote:
> Referencing this old thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00606.html
> 
> I've run into an issue recently with testing q35 DMAR/intel iommu with ahci
> driver.  My ahci driver writes the upper-32 bits (PORT_FIS_ADDR_HI) first
> then the lower 32-bits (PORT_FIS_ADDR).
> 
> The contents of PORT_FIS_ADDR therefore are stale when the PORT_FIS_ADDR_HI
> write calls map_page().  DMAR translation fails at this point as the old
> stale address (from SEABIOS initialization) is not in the DMAR page table.

The AHCI device tries to map on register writes to both the base and
upper 32-bit registers.  So it should work for a driver that writes
PORT_FIS_ADDR_HI before PORT_FIS_ADDR.

Does the iommu failure pose a problem?

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Jordan Hargrave]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

The problem is the FIS registers have stale data.

SeaBIOS initialization leaves the registers:
PORT_FIS_ADDR = 0x7fae0000
PORT_FIS_ADDR_HI = 0x0

My OS initializes DMAR page tables and then enables the IOMMU translation.
Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS registers.
eg. FIS DMA address is 0x10000 (maps to some hardware physical address via
iommu)

The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00 << 32) |
0x7fae0000... 0x7fae0000 is stale, and is not in the IOMMU page map.
Causes a non-recoverable IOMMU fault.



On Thu, Feb 26, 2015 at 8:45 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Feb 25, 2015 at 11:13:09PM -0600, Jordan Hargrave wrote:
> > Referencing this old thread:
> > https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00606.html
> >
> > I've run into an issue recently with testing q35 DMAR/intel iommu with
> ahci
> > driver.  My ahci driver writes the upper-32 bits (PORT_FIS_ADDR_HI) first
> > then the lower 32-bits (PORT_FIS_ADDR).
> >
> > The contents of PORT_FIS_ADDR therefore are stale when the
> PORT_FIS_ADDR_HI
> > write calls map_page().  DMAR translation fails at this point as the old
> > stale address (from SEABIOS initialization) is not in the DMAR page
> table.
>
> The AHCI device tries to map on register writes to both the base and
> upper 32-bit registers.  So it should work for a driver that writes
> PORT_FIS_ADDR_HI before PORT_FIS_ADDR.
>
> Does the iommu failure pose a problem?
>
> Stefan
>

[-- Attachment #2: Type: text/html, Size: 2214 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Paolo Bonzini]

On 26/02/2015 22:31, Jordan Hargrave wrote:
> 
> My OS initializes DMAR page tables and then enables the IOMMU translation. 
> Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS registers.
> eg. FIS DMA address is 0x10000 (maps to some hardware physical address
> via iommu)
> 
> The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00 << 32)
> | 0x7fae0000... 0x7fae0000 is stale, and is not in the IOMMU page map. 
> Causes a non-recoverable IOMMU fault.

That's a bug in QEMU.  map_page must be skipped unless PORT_CMD_FIS_ON
is set in pr->cmd (also, QEMU is never resetting PORT_CMD_FIS_ON when
PORT_CMD_FIS_RX goes down).

Paolo

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[John Snow]

(Please don't top-post on qemu-devel: gmail is kind of awful about this, 
but if you expand the conversation while in-reply, you can edit beneath 
the quote instead of above.)

On 02/26/2015 04:31 PM, Jordan Hargrave wrote:
> The problem is the FIS registers have stale data.
>
> SeaBIOS initialization leaves the registers:
> PORT_FIS_ADDR = 0x7fae0000
> PORT_FIS_ADDR_HI = 0x0
>
> My OS initializes DMAR page tables and then enables the IOMMU translation.
> Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS registers.
> eg. FIS DMA address is 0x10000 (maps to some hardware physical address
> via iommu)
>
> The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00 << 32)
> | 0x7fae0000... 0x7fae0000 is stale, and is not in the IOMMU page map.
> Causes a non-recoverable IOMMU fault.
>
>

OK, I see.

We can probably fix this by delaying the map and having it map on-demand 
before first access, setting a dirty flag if the registers have changed 
since last use.

It might be an AHCI spec violation to change this register once the FIS 
Receive Engine is active, too, so it might not be too hard of a change; 
perhaps we can just map the FIS Receive Buffer once the FRE is started.

Did you want to send a patch, or should I?

--js

>
> On Thu, Feb 26, 2015 at 8:45 AM, Stefan Hajnoczi <stefanha@gmail.com
> <mailto:stefanha@gmail.com>> wrote:
>
>     On Wed, Feb 25, 2015 at 11:13:09PM -0600, Jordan Hargrave wrote:
>      > Referencing this old thread:
>      >
>     https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00606.html
>      >
>      > I've run into an issue recently with testing q35 DMAR/intel iommu
>     with ahci
>      > driver.  My ahci driver writes the upper-32 bits
>     (PORT_FIS_ADDR_HI) first
>      > then the lower 32-bits (PORT_FIS_ADDR).
>      >
>      > The contents of PORT_FIS_ADDR therefore are stale when the
>     PORT_FIS_ADDR_HI
>      > write calls map_page().  DMAR translation fails at this point as
>     the old
>      > stale address (from SEABIOS initialization) is not in the DMAR
>     page table.
>
>     The AHCI device tries to map on register writes to both the base and
>     upper 32-bit registers.  So it should work for a driver that writes
>     PORT_FIS_ADDR_HI before PORT_FIS_ADDR.
>
>     Does the iommu failure pose a problem?
>
>     Stefan
>
>

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[John Snow]

On 03/02/2015 11:03 AM, Jordan Hargrave wrote:
>
>
> On Thu, Feb 26, 2015 at 4:31 PM, John Snow <jsnow@redhat.com
> <mailto:jsnow@redhat.com>> wrote:
>
>     (Please don't top-post on qemu-devel: gmail is kind of awful about
>     this, but if you expand the conversation while in-reply, you can
>     edit beneath the quote instead of above.)
>
>     On 02/26/2015 04:31 PM, Jordan Hargrave wrote:
>
>         The problem is the FIS registers have stale data.
>
>         SeaBIOS initialization leaves the registers:
>         PORT_FIS_ADDR = 0x7fae0000
>         PORT_FIS_ADDR_HI = 0x0
>
>         My OS initializes DMAR page tables and then enables the IOMMU
>         translation.
>         Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS
>         registers.
>         eg. FIS DMA address is 0x10000 (maps to some hardware physical
>         address
>         via iommu)
>
>         The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00
>         << 32)
>         | 0x7fae0000... 0x7fae0000 is stale, and is not in the IOMMU
>         page map.
>         Causes a non-recoverable IOMMU fault.
>
>
>
>     OK, I see.
>
>     We can probably fix this by delaying the map and having it map
>     on-demand before first access, setting a dirty flag if the registers
>     have changed since last use.
>
>     It might be an AHCI spec violation to change this register once the
>     FIS Receive Engine is active, too, so it might not be too hard of a
>     change; perhaps we can just map the FIS Receive Buffer once the FRE
>     is started.
>
>     Did you want to send a patch, or should I?
>
>     --js
>
>
> I'm not as familiar with qemu internals yet to figure out how to map the
> memory on demand.   I switched order of FIS initialization in my OS
> driver for the time being.  The same problem was happening with the
> PORT_ADDR_LST/PORT_ADDR_LST_HI register initialization pwas well.

Okay, I'll pick this up, then. Thanks!

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[John Snow]

On 02/26/2015 05:02 PM, Paolo Bonzini wrote:
>
>
> On 26/02/2015 22:31, Jordan Hargrave wrote:
>>
>> My OS initializes DMAR page tables and then enables the IOMMU translation.
>> Then OS initializes AHCI driver.  Writes VIRTUAL DMA to FIS registers.
>> eg. FIS DMA address is 0x10000 (maps to some hardware physical address
>> via iommu)
>>
>> The OS writes 0x00 PORT_FIS_ADDR_HI -> qemu calls map_page (0x00 << 32)
>> | 0x7fae0000... 0x7fae0000 is stale, and is not in the IOMMU page map.
>> Causes a non-recoverable IOMMU fault.
>
> That's a bug in QEMU.  map_page must be skipped unless PORT_CMD_FIS_ON
> is set in pr->cmd (also, QEMU is never resetting PORT_CMD_FIS_ON when
> PORT_CMD_FIS_RX goes down).
>
> Paolo
>

map_page must be skipped *if* PORT_CMD_FIS_ON is set -- or rather, that 
produces undefined behavior in the AHCI spec. We can only set these 
fields when the FIS Receive Engine is off. We can only modify the CLB if 
PxCMD.ST is off.

We also never actually reset PORT_CMD_FIS_RX or PORT_CMD_FIS_ON 
ourselves, so the status bit will get reset when the user writes zeroes 
to those bits when writing PxCMD. (We always clear PORT_CMD_FIS_ON and 
PORT_CMD_LIST_ON both upon any write to PxCMD, then re-add these flags 
if we see the user has written PORT_CMD_START or PORT_CMD_FIS_RX.)

So I think I can fix this just by delaying the CLB and FB mappings to 
the PORT_CMD_LIST_ON and PORT_CMD_FIS_ON events. I'll send a patch.

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[John Snow]

On 02/26/2015 12:13 AM, Jordan Hargrave wrote:
> Referencing this old thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2014-07/msg00606.html
>
> I've run into an issue recently with testing q35 DMAR/intel iommu with
> ahci driver.  My ahci driver writes the upper-32 bits (PORT_FIS_ADDR_HI)
> first then the lower 32-bits (PORT_FIS_ADDR).
>
> The contents of PORT_FIS_ADDR therefore are stale when the
> PORT_FIS_ADDR_HI write calls map_page().  DMAR translation fails at this
> point as the old stale address (from SEABIOS initialization) is not in
> the DMAR page table.
>

Hi: I posted a fix, but do you mind elaborating on what the failure 
actually looked like?

Did QEMU abort, did the guest crash, etc? What exact behavior did you 
observe?

Do you have a command line to share with us?

--js

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On Thu, Jul 03, 2014 at 04:26:27PM +0800, Le Tan wrote:
> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> because ahci devices should not access memory directly but via their address
> space. Add an AddressSpace parameter to map_page(). In order to call
> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> 
> Signed-off-by: Le Tan <tamlokveer@gmail.com>
> ---
>  hw/ide/ahci.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Fri, Jul 04, 2014 at 07:26:14AM +0200, Jan Kiszka wrote:
> On 2014-07-03 22:30, Michael S. Tsirkin wrote:
> > On Thu, Jul 03, 2014 at 06:45:03PM +0200, Jan Kiszka wrote:
> >> On 2014-07-03 12:02, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
> >>>> On 2014-07-03 10:26, Le Tan wrote:
> >>>>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> >>>>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> >>>>> because ahci devices should not access memory directly but via their address
> >>>>> space. Add an AddressSpace parameter to map_page(). In order to call
> >>>>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> >>>>
> >>>> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> >>>> more cases that should be checked (for x86). I suppose vhost is
> >>>> incompatible with an IOMMU,
> >>>
> >>> vhost can be made to work: you just need to
> >>> update its memory tables as appropriate.
> >>> But see below
> >>>
> >>>> but plain virtio should work,
> >>>
> >>> It doesn't: all guests pass in physical addresses at the moment.
> >>
> >> You mean they do not put virtio devices into IOMMU domains, or they do
> >> put them but ignore any translation rules that are not 1:1?
> > 
> > Look at the code. We just pass in physical addresses
> > ignoring which iommu domain device ended up with.
> 
> Should probably be fixed, at least in Linux. I suppose there is always a
> 1:1 domain where virtio devices can be put in so that they are not
> involved in any translation if this is not desired (PPC?).

This will need some experiments.

> > 
> >>> We discussed requiring this for virtio 1.0, but in the end,
> >>> most people thought that passing through virtio devices
> >>> isn't worthwhile.
> >>
> >> It should be consistent at least. If virtio is not translated, we have
> >> to exclude such devices via ACPI tables from the scope of our IOMMUs.
> > 
> > I didn't know this is possible. How does one do this?
> 
> Both the DMAR (Intel) and the IVRS (AMD) ACPI tables allow to report the
> scope of an IOMMU unit. There you list sets of devices on specific buses
> or a complete segment that the unit virtualizes. Le currently reports
> that there is a single DMAR unit for q35, and that one controls the
> complete segment 0. The structure could be adapted if a virtio device is
> present so that this device is not included. That should prevent that
> the guest tries to control virtio devices via IOMMUs or even assign them
> to some L2 guests.

Might be tricky to do with hotplugged devices.

> We could then provide a machine property that controls virtualization of
> virtio devices, default off, but you can enable it when running properly
> adjusted guest drivers.
> 
> > 
> >>> We can certainly add that as an option, with a feature bit.
> >>>
> >>> If you feel otherwise, you can comment on the latest spec draft.
> >>
> >> Does the spec at least state that "virtio devices are not subject to any
> >> guest configured IOMMU translation"? Is is this left undefined?
> >>
> >> Jan
> >>
> >>
> > 
> > I don't think we have anything like this.
> > 
> 
> And I'm not even sure it necessarily belongs to the spec. I think it's
> just a guest driver deficit to ignore IOMMU settings that the guest OS
> may apply.
> 
> Jan
> 

I guess we might want to at least document the historical driver
behaviour of skipping the IOMMU. But I think I agree this can wait:
we will have time to figure it out before we release QEMU 2.2.

-- 
MST

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3156 bytes --]

On 2014-07-03 22:30, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 06:45:03PM +0200, Jan Kiszka wrote:
>> On 2014-07-03 12:02, Michael S. Tsirkin wrote:
>>> On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
>>>> On 2014-07-03 10:26, Le Tan wrote:
>>>>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
>>>>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
>>>>> because ahci devices should not access memory directly but via their address
>>>>> space. Add an AddressSpace parameter to map_page(). In order to call
>>>>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
>>>>
>>>> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
>>>> more cases that should be checked (for x86). I suppose vhost is
>>>> incompatible with an IOMMU,
>>>
>>> vhost can be made to work: you just need to
>>> update its memory tables as appropriate.
>>> But see below
>>>
>>>> but plain virtio should work,
>>>
>>> It doesn't: all guests pass in physical addresses at the moment.
>>
>> You mean they do not put virtio devices into IOMMU domains, or they do
>> put them but ignore any translation rules that are not 1:1?
> 
> Look at the code. We just pass in physical addresses
> ignoring which iommu domain device ended up with.

Should probably be fixed, at least in Linux. I suppose there is always a
1:1 domain where virtio devices can be put in so that they are not
involved in any translation if this is not desired (PPC?).

> 
>>> We discussed requiring this for virtio 1.0, but in the end,
>>> most people thought that passing through virtio devices
>>> isn't worthwhile.
>>
>> It should be consistent at least. If virtio is not translated, we have
>> to exclude such devices via ACPI tables from the scope of our IOMMUs.
> 
> I didn't know this is possible. How does one do this?

Both the DMAR (Intel) and the IVRS (AMD) ACPI tables allow to report the
scope of an IOMMU unit. There you list sets of devices on specific buses
or a complete segment that the unit virtualizes. Le currently reports
that there is a single DMAR unit for q35, and that one controls the
complete segment 0. The structure could be adapted if a virtio device is
present so that this device is not included. That should prevent that
the guest tries to control virtio devices via IOMMUs or even assign them
to some L2 guests.

We could then provide a machine property that controls virtualization of
virtio devices, default off, but you can enable it when running properly
adjusted guest drivers.

> 
>>> We can certainly add that as an option, with a feature bit.
>>>
>>> If you feel otherwise, you can comment on the latest spec draft.
>>
>> Does the spec at least state that "virtio devices are not subject to any
>> guest configured IOMMU translation"? Is is this left undefined?
>>
>> Jan
>>
>>
> 
> I don't think we have anything like this.
> 

And I'm not even sure it necessarily belongs to the spec. I think it's
just a guest driver deficit to ignore IOMMU settings that the guest OS
may apply.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 06:45:03PM +0200, Jan Kiszka wrote:
> On 2014-07-03 12:02, Michael S. Tsirkin wrote:
> > On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
> >> On 2014-07-03 10:26, Le Tan wrote:
> >>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> >>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> >>> because ahci devices should not access memory directly but via their address
> >>> space. Add an AddressSpace parameter to map_page(). In order to call
> >>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> >>
> >> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> >> more cases that should be checked (for x86). I suppose vhost is
> >> incompatible with an IOMMU,
> > 
> > vhost can be made to work: you just need to
> > update its memory tables as appropriate.
> > But see below
> > 
> >> but plain virtio should work,
> > 
> > It doesn't: all guests pass in physical addresses at the moment.
> 
> You mean they do not put virtio devices into IOMMU domains, or they do
> put them but ignore any translation rules that are not 1:1?

Look at the code. We just pass in physical addresses
ignoring which iommu domain device ended up with.

> > We discussed requiring this for virtio 1.0, but in the end,
> > most people thought that passing through virtio devices
> > isn't worthwhile.
> 
> It should be consistent at least. If virtio is not translated, we have
> to exclude such devices via ACPI tables from the scope of our IOMMUs.

I didn't know this is possible. How does one do this?

> > We can certainly add that as an option, with a feature bit.
> > 
> > If you feel otherwise, you can comment on the latest spec draft.
> 
> Does the spec at least state that "virtio devices are not subject to any
> guest configured IOMMU translation"? Is is this left undefined?
> 
> Jan
> 
> 

I don't think we have anything like this.

-- 
MST

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On 2014-07-03 12:02, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
>> On 2014-07-03 10:26, Le Tan wrote:
>>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
>>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
>>> because ahci devices should not access memory directly but via their address
>>> space. Add an AddressSpace parameter to map_page(). In order to call
>>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
>>
>> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
>> more cases that should be checked (for x86). I suppose vhost is
>> incompatible with an IOMMU,
> 
> vhost can be made to work: you just need to
> update its memory tables as appropriate.
> But see below
> 
>> but plain virtio should work,
> 
> It doesn't: all guests pass in physical addresses at the moment.

You mean they do not put virtio devices into IOMMU domains, or they do
put them but ignore any translation rules that are not 1:1?

> We discussed requiring this for virtio 1.0, but in the end,
> most people thought that passing through virtio devices
> isn't worthwhile.

It should be consistent at least. If virtio is not translated, we have
to exclude such devices via ACPI tables from the scope of our IOMMUs.

> We can certainly add that as an option, with a feature bit.
> 
> If you feel otherwise, you can comment on the latest spec draft.

Does the spec at least state that "virtio devices are not subject to any
guest configured IOMMU translation"? Is is this left undefined?

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 11:41:29AM +0200, Paolo Bonzini wrote:
> Il 03/07/2014 10:43, Jan Kiszka ha scritto:
> >On 2014-07-03 10:26, Le Tan wrote:
> >>In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> >>cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> >>because ahci devices should not access memory directly but via their address
> >>space. Add an AddressSpace parameter to map_page(). In order to call
> >>map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> >
> >BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> >more cases that should be checked (for x86). I suppose vhost is
> >incompatible with an IOMMU, but plain virtio should work, same for vmxnet.
> 
> I think PPC folks explicitly wanted virtio to bypass the IOMMU, probably in
> order to get vhost running.  It seems like a bad idea to me, but who am I...
> 
> Paolo

The argument went like this: IOMMU slows things down. People
might want to run a VM where nested virt is *possible*
so IOMMU has to be enabled but still get fast IO for the rest of
the guest.

-- 
MST

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 10:43:57AM +0200, Jan Kiszka wrote:
> On 2014-07-03 10:26, Le Tan wrote:
> > In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> > cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> > because ahci devices should not access memory directly but via their address
> > space. Add an AddressSpace parameter to map_page(). In order to call
> > map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> 
> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> more cases that should be checked (for x86). I suppose vhost is
> incompatible with an IOMMU,

vhost can be made to work: you just need to
update its memory tables as appropriate.
But see below

> but plain virtio should work,

It doesn't: all guests pass in physical addresses at the moment.
We discussed requiring this for virtio 1.0, but in the end,
most people thought that passing through virtio devices
isn't worthwhile.
We can certainly add that as an option, with a feature bit.

If you feel otherwise, you can comment on the latest spec draft.

> same for vmxnet.
> 
> Jan
> 
> > 
> > Signed-off-by: Le Tan <tamlokveer@gmail.com>
> > ---
> >  hw/ide/ahci.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 9bae22e..7bb0a03 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -175,17 +175,18 @@ static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
> >      ahci_check_irq(s);
> >  }
> >  
> > -static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
> > +static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
> > +                     uint32_t wanted)
> >  {
> >      hwaddr len = wanted;
> >  
> >      if (*ptr) {
> > -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> > +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >      }
> >  
> > -    *ptr = cpu_physical_memory_map(addr, &len, 1);
> > +    *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
> >      if (len < wanted) {
> > -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> > +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >          *ptr = NULL;
> >      }
> >  }
> > @@ -198,24 +199,24 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
> >      switch (offset) {
> >          case PORT_LST_ADDR:
> >              pr->lst_addr = val;
> > -            map_page(&s->dev[port].lst,
> > +            map_page(s->as, &s->dev[port].lst,
> >                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> >              s->dev[port].cur_cmd = NULL;
> >              break;
> >          case PORT_LST_ADDR_HI:
> >              pr->lst_addr_hi = val;
> > -            map_page(&s->dev[port].lst,
> > +            map_page(s->as, &s->dev[port].lst,
> >                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> >              s->dev[port].cur_cmd = NULL;
> >              break;
> >          case PORT_FIS_ADDR:
> >              pr->fis_addr = val;
> > -            map_page(&s->dev[port].res_fis,
> > +            map_page(s->as, &s->dev[port].res_fis,
> >                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> >              break;
> >          case PORT_FIS_ADDR_HI:
> >              pr->fis_addr_hi = val;
> > -            map_page(&s->dev[port].res_fis,
> > +            map_page(s->as, &s->dev[port].res_fis,
> >                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> >              break;
> >          case PORT_IRQ_STAT:
> > @@ -1260,9 +1261,9 @@ static int ahci_state_post_load(void *opaque, int version_id)
> >          ad = &s->dev[i];
> >          AHCIPortRegs *pr = &ad->port_regs;
> >  
> > -        map_page(&ad->lst,
> > +        map_page(s->as, &ad->lst,
> >                   ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> > -        map_page(&ad->res_fis,
> > +        map_page(s->as, &ad->res_fis,
> >                   ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
> >          /*
> >           * All pending i/o should be flushed out on a migrate. However,
> > 
> 
> 

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Paolo Bonzini]

Il 03/07/2014 10:43, Jan Kiszka ha scritto:
> On 2014-07-03 10:26, Le Tan wrote:
>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
>> because ahci devices should not access memory directly but via their address
>> space. Add an AddressSpace parameter to map_page(). In order to call
>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
>
> BTW, when doing "git grep cpu_physical_memory_map hw", there are some
> more cases that should be checked (for x86). I suppose vhost is
> incompatible with an IOMMU, but plain virtio should work, same for vmxnet.

I think PPC folks explicitly wanted virtio to bypass the IOMMU, probably 
in order to get vhost running.  It seems like a bad idea to me, but who 
am I...

Paolo

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Paolo Bonzini]

Il 03/07/2014 10:28, Michael S. Tsirkin ha scritto:
> On Thu, Jul 03, 2014 at 04:26:27PM +0800, Le Tan wrote:
>> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
>> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
>> because ahci devices should not access memory directly but via their address
>> space. Add an AddressSpace parameter to map_page(). In order to call
>> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
>>
>> Signed-off-by: Le Tan <tamlokveer@gmail.com>
>
> Makes sense
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I think this is ok for 2.1 as well.

Paolo

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3696 bytes --]

On 2014-07-03 10:26, Le Tan wrote:
> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> because ahci devices should not access memory directly but via their address
> space. Add an AddressSpace parameter to map_page(). In order to call
> map_page(), we should pass the AHCIState.as as the AddressSpace argument.

BTW, when doing "git grep cpu_physical_memory_map hw", there are some
more cases that should be checked (for x86). I suppose vhost is
incompatible with an IOMMU, but plain virtio should work, same for vmxnet.

Jan

> 
> Signed-off-by: Le Tan <tamlokveer@gmail.com>
> ---
>  hw/ide/ahci.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9bae22e..7bb0a03 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -175,17 +175,18 @@ static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>      ahci_check_irq(s);
>  }
>  
> -static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
> +static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
> +                     uint32_t wanted)
>  {
>      hwaddr len = wanted;
>  
>      if (*ptr) {
> -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>      }
>  
> -    *ptr = cpu_physical_memory_map(addr, &len, 1);
> +    *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
>      if (len < wanted) {
> -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>          *ptr = NULL;
>      }
>  }
> @@ -198,24 +199,24 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
>      switch (offset) {
>          case PORT_LST_ADDR:
>              pr->lst_addr = val;
> -            map_page(&s->dev[port].lst,
> +            map_page(s->as, &s->dev[port].lst,
>                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
>              s->dev[port].cur_cmd = NULL;
>              break;
>          case PORT_LST_ADDR_HI:
>              pr->lst_addr_hi = val;
> -            map_page(&s->dev[port].lst,
> +            map_page(s->as, &s->dev[port].lst,
>                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
>              s->dev[port].cur_cmd = NULL;
>              break;
>          case PORT_FIS_ADDR:
>              pr->fis_addr = val;
> -            map_page(&s->dev[port].res_fis,
> +            map_page(s->as, &s->dev[port].res_fis,
>                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>              break;
>          case PORT_FIS_ADDR_HI:
>              pr->fis_addr_hi = val;
> -            map_page(&s->dev[port].res_fis,
> +            map_page(s->as, &s->dev[port].res_fis,
>                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>              break;
>          case PORT_IRQ_STAT:
> @@ -1260,9 +1261,9 @@ static int ahci_state_post_load(void *opaque, int version_id)
>          ad = &s->dev[i];
>          AHCIPortRegs *pr = &ad->port_regs;
>  
> -        map_page(&ad->lst,
> +        map_page(s->as, &ad->lst,
>                   ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> -        map_page(&ad->res_fis,
> +        map_page(s->as, &ad->res_fis,
>                   ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>          /*
>           * All pending i/o should be flushed out on a migrate. However,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 04:26:27PM +0800, Le Tan wrote:
> In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
> cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
> because ahci devices should not access memory directly but via their address
> space. Add an AddressSpace parameter to map_page(). In order to call
> map_page(), we should pass the AHCIState.as as the AddressSpace argument.
> 
> Signed-off-by: Le Tan <tamlokveer@gmail.com>

Makes sense
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
>  hw/ide/ahci.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9bae22e..7bb0a03 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -175,17 +175,18 @@ static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>      ahci_check_irq(s);
>  }
>  
> -static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
> +static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
> +                     uint32_t wanted)
>  {
>      hwaddr len = wanted;
>  
>      if (*ptr) {
> -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>      }
>  
> -    *ptr = cpu_physical_memory_map(addr, &len, 1);
> +    *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
>      if (len < wanted) {
> -        cpu_physical_memory_unmap(*ptr, len, 1, len);
> +        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
>          *ptr = NULL;
>      }
>  }
> @@ -198,24 +199,24 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
>      switch (offset) {
>          case PORT_LST_ADDR:
>              pr->lst_addr = val;
> -            map_page(&s->dev[port].lst,
> +            map_page(s->as, &s->dev[port].lst,
>                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
>              s->dev[port].cur_cmd = NULL;
>              break;
>          case PORT_LST_ADDR_HI:
>              pr->lst_addr_hi = val;
> -            map_page(&s->dev[port].lst,
> +            map_page(s->as, &s->dev[port].lst,
>                       ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
>              s->dev[port].cur_cmd = NULL;
>              break;
>          case PORT_FIS_ADDR:
>              pr->fis_addr = val;
> -            map_page(&s->dev[port].res_fis,
> +            map_page(s->as, &s->dev[port].res_fis,
>                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>              break;
>          case PORT_FIS_ADDR_HI:
>              pr->fis_addr_hi = val;
> -            map_page(&s->dev[port].res_fis,
> +            map_page(s->as, &s->dev[port].res_fis,
>                       ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>              break;
>          case PORT_IRQ_STAT:
> @@ -1260,9 +1261,9 @@ static int ahci_state_post_load(void *opaque, int version_id)
>          ad = &s->dev[i];
>          AHCIPortRegs *pr = &ad->port_regs;
>  
> -        map_page(&ad->lst,
> +        map_page(s->as, &ad->lst,
>                   ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
> -        map_page(&ad->res_fis,
> +        map_page(s->as, &ad->res_fis,
>                   ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
>          /*
>           * All pending i/o should be flushed out on a migrate. However,
> -- 
> 1.7.9.5

[Qemu-devel] [PATCH] ahci: map memory via device's address space instead of address_space_memory

[Le Tan]

In map_page() in hw/ide/ahci.c, replace cpu_physical_memory_map() and
cpu_physical_memory_unmap() with dma_memory_map() and dma_memory_unmap(),
because ahci devices should not access memory directly but via their address
space. Add an AddressSpace parameter to map_page(). In order to call
map_page(), we should pass the AHCIState.as as the AddressSpace argument.

Signed-off-by: Le Tan <tamlokveer@gmail.com>
---
 hw/ide/ahci.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9bae22e..7bb0a03 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -175,17 +175,18 @@ static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
     ahci_check_irq(s);
 }
 
-static void map_page(uint8_t **ptr, uint64_t addr, uint32_t wanted)
+static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
+                     uint32_t wanted)
 {
     hwaddr len = wanted;
 
     if (*ptr) {
-        cpu_physical_memory_unmap(*ptr, len, 1, len);
+        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
     }
 
-    *ptr = cpu_physical_memory_map(addr, &len, 1);
+    *ptr = dma_memory_map(as, addr, &len, DMA_DIRECTION_FROM_DEVICE);
     if (len < wanted) {
-        cpu_physical_memory_unmap(*ptr, len, 1, len);
+        dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
         *ptr = NULL;
     }
 }
@@ -198,24 +199,24 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
     switch (offset) {
         case PORT_LST_ADDR:
             pr->lst_addr = val;
-            map_page(&s->dev[port].lst,
+            map_page(s->as, &s->dev[port].lst,
                      ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
             s->dev[port].cur_cmd = NULL;
             break;
         case PORT_LST_ADDR_HI:
             pr->lst_addr_hi = val;
-            map_page(&s->dev[port].lst,
+            map_page(s->as, &s->dev[port].lst,
                      ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
             s->dev[port].cur_cmd = NULL;
             break;
         case PORT_FIS_ADDR:
             pr->fis_addr = val;
-            map_page(&s->dev[port].res_fis,
+            map_page(s->as, &s->dev[port].res_fis,
                      ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
             break;
         case PORT_FIS_ADDR_HI:
             pr->fis_addr_hi = val;
-            map_page(&s->dev[port].res_fis,
+            map_page(s->as, &s->dev[port].res_fis,
                      ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
             break;
         case PORT_IRQ_STAT:
@@ -1260,9 +1261,9 @@ static int ahci_state_post_load(void *opaque, int version_id)
         ad = &s->dev[i];
         AHCIPortRegs *pr = &ad->port_regs;
 
-        map_page(&ad->lst,
+        map_page(s->as, &ad->lst,
                  ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-        map_page(&ad->res_fis,
+        map_page(s->as, &ad->res_fis,
                  ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
         /*
          * All pending i/o should be flushed out on a migrate. However,
-- 
1.7.9.5