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

[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

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

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

[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

[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

[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

[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

[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 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: 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 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

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

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 11:28:52AM +0300, Michael S. Tsirkin wrote:
> 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>
> 

Stefan, Kevin, you are going to pick this one?

> > ---
> >  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

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

[Paolo Bonzini]

> Stefan, Kevin, you are going to pick this one?

I think there's no hurry for 2.1.

Also...

> > > ---
> > > -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)

... it is a bit cleaner to pass the AHCIState here.

Paolo

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

[Stefan Hajnoczi]

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

On Sun, Jul 06, 2014 at 09:15:38AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 11:28:52AM +0300, Michael S. Tsirkin wrote:
> > 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>
> > 
> 
> Stefan, Kevin, you are going to pick this one?

Done.

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

[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 --]