subject:"\[PATCH v3 06\/10\] arm\: simplify MMIO dispatching"

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-24 Thread Andre Przywara

Hi Will,

do you want me to respin the whole series to address the remaining minor
comments in the last four patches or do you want to take patch 01-06
already (which I think Marc has already agreed upon)?
Then I would just send an updated version of the remaining patches.

Cheers,
Andre.




 No, that's fine.
 
 I just wondered what was the rational behind having the
 arm_addr_in_pci_region() call there. It might have guarded something,
 but if you're absolutely positive that this doesn't cause a regression,
 that's OK with me.
 
 Reviewed-by: Marc Zyngier marc.zyng...@arm.com
 
   M.
 
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-24 Thread Will Deacon

On Wed, Jun 24, 2015 at 02:30:05PM +0100, Andre Przywara wrote:
 do you want me to respin the whole series to address the remaining minor
 comments in the last four patches or do you want to take patch 01-06
 already (which I think Marc has already agreed upon)?
 Then I would just send an updated version of the remaining patches.

Yes, please. It's easier to keep track if you just repost the series with
the changes and acks.

Will
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-17 Thread Andre Przywara

Currently we separate any incoming MMIO request into one of the ARM
memory map regions and take care to spare the GIC.
It turns out that this is unnecessary, as we only have one special
region (the IO port area in the first 64 KByte). The MMIO rbtree
takes care about unhandled MMIO ranges, so we can simply drop all the
special range checking (except that for the IO range) in
kvm_cpu__emulate_mmio().
As the GIC is handled in the kernel, a GIC MMIO access should never
reach userland (and we don't know what to do with it anyway).
This lets us delete some more code and simplifies future extensions
(like expanding the GIC regions).
To be in line with the other architectures, move the now simpler
code into a header file.

Signed-off-by: Andre Przywara andre.przyw...@arm.com
---
 arm/include/arm-common/kvm-arch.h | 12 
 arm/include/arm-common/kvm-cpu-arch.h | 14 --
 arm/kvm-cpu.c | 16 
 3 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/arm/include/arm-common/kvm-arch.h 
b/arm/include/arm-common/kvm-arch.h
index 082131d..90d6733 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
return phys_addr = KVM_IOPORT_AREA  phys_addr  limit;
 }
 
-static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
-{
-   u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
-   return phys_addr = KVM_VIRTIO_MMIO_AREA  phys_addr  limit;
-}
-
-static inline bool arm_addr_in_pci_region(u64 phys_addr)
-{
-   u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
-   return phys_addr = KVM_PCI_CFG_AREA  phys_addr  limit;
-}
-
 struct kvm_arch {
/*
 * We may have to align the guest memory for virtio, so keep the
diff --git a/arm/include/arm-common/kvm-cpu-arch.h 
b/arm/include/arm-common/kvm-cpu-arch.h
index 36c7872..329979a 100644
--- a/arm/include/arm-common/kvm-cpu-arch.h
+++ b/arm/include/arm-common/kvm-cpu-arch.h
@@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, 
u16 port, void *dat
return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-  u32 len, u8 is_write);
+static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
+u8 *data, u32 len, u8 is_write)
+{
+   if (arm_addr_in_ioport_region(phys_addr)) {
+   int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
+   u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
+
+   return kvm__emulate_io(vcpu, port, data, direction, len, 1);
+   }
+
+   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
+}
 
 unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
 
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index ab08815..7780251 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-  u32 len, u8 is_write)
-{
-   if (arm_addr_in_virtio_mmio_region(phys_addr)) {
-   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-   } else if (arm_addr_in_ioport_region(phys_addr)) {
-   int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-   u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
-   return kvm__emulate_io(vcpu, port, data, direction, len, 1);
-   } else if (arm_addr_in_pci_region(phys_addr)) {
-   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-   }
-
-   return false;
-}
-
 void kvm_cpu__show_page_tables(struct kvm_cpu *vcpu)
 {
 }
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-17 Thread Marc Zyngier

On 17/06/15 12:21, Andre Przywara wrote:
 Currently we separate any incoming MMIO request into one of the ARM
 memory map regions and take care to spare the GIC.
 It turns out that this is unnecessary, as we only have one special
 region (the IO port area in the first 64 KByte). The MMIO rbtree
 takes care about unhandled MMIO ranges, so we can simply drop all the
 special range checking (except that for the IO range) in
 kvm_cpu__emulate_mmio().
 As the GIC is handled in the kernel, a GIC MMIO access should never
 reach userland (and we don't know what to do with it anyway).
 This lets us delete some more code and simplifies future extensions
 (like expanding the GIC regions).
 To be in line with the other architectures, move the now simpler
 code into a header file.
 
 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
  arm/include/arm-common/kvm-arch.h | 12 
  arm/include/arm-common/kvm-cpu-arch.h | 14 --
  arm/kvm-cpu.c | 16 
  3 files changed, 12 insertions(+), 30 deletions(-)
 
 diff --git a/arm/include/arm-common/kvm-arch.h 
 b/arm/include/arm-common/kvm-arch.h
 index 082131d..90d6733 100644
 --- a/arm/include/arm-common/kvm-arch.h
 +++ b/arm/include/arm-common/kvm-arch.h
 @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
   return phys_addr = KVM_IOPORT_AREA  phys_addr  limit;
  }
  
 -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
 -{
 - u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
 - return phys_addr = KVM_VIRTIO_MMIO_AREA  phys_addr  limit;
 -}
 -
 -static inline bool arm_addr_in_pci_region(u64 phys_addr)
 -{
 - u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
 - return phys_addr = KVM_PCI_CFG_AREA  phys_addr  limit;
 -}
 -
  struct kvm_arch {
   /*
* We may have to align the guest memory for virtio, so keep the
 diff --git a/arm/include/arm-common/kvm-cpu-arch.h 
 b/arm/include/arm-common/kvm-cpu-arch.h
 index 36c7872..329979a 100644
 --- a/arm/include/arm-common/kvm-cpu-arch.h
 +++ b/arm/include/arm-common/kvm-cpu-arch.h
 @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu 
 *vcpu, u16 port, void *dat
   return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -u32 len, u8 is_write);
 +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
 +  u8 *data, u32 len, u8 is_write)
 +{
 + if (arm_addr_in_ioport_region(phys_addr)) {
 + int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 + u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 +
 + return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 + }
 +
 + return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 +}
  
  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
  
 diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
 index ab08815..7780251 100644
 --- a/arm/kvm-cpu.c
 +++ b/arm/kvm-cpu.c
 @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
   return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -u32 len, u8 is_write)
 -{
 - if (arm_addr_in_virtio_mmio_region(phys_addr)) {
 - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 - } else if (arm_addr_in_ioport_region(phys_addr)) {
 - int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 - u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 - return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 - } else if (arm_addr_in_pci_region(phys_addr)) {
 - return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 - }

Can you explain why this arm_addr_in_pci_region(phys_addr) check has
disappeared in your updated version on this function? It may be a non
issue, but I'd very much like to understand.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-17 Thread Andre Przywara

Hi Marc,

On 06/17/2015 01:48 PM, Marc Zyngier wrote:
 On 17/06/15 12:21, Andre Przywara wrote:
 Currently we separate any incoming MMIO request into one of the ARM
 memory map regions and take care to spare the GIC.
 It turns out that this is unnecessary, as we only have one special
 region (the IO port area in the first 64 KByte). The MMIO rbtree
 takes care about unhandled MMIO ranges, so we can simply drop all the
 special range checking (except that for the IO range) in
 kvm_cpu__emulate_mmio().
 As the GIC is handled in the kernel, a GIC MMIO access should never
 reach userland (and we don't know what to do with it anyway).
 This lets us delete some more code and simplifies future extensions
 (like expanding the GIC regions).
 To be in line with the other architectures, move the now simpler
 code into a header file.

 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
  arm/include/arm-common/kvm-arch.h | 12 
  arm/include/arm-common/kvm-cpu-arch.h | 14 --
  arm/kvm-cpu.c | 16 
  3 files changed, 12 insertions(+), 30 deletions(-)

 diff --git a/arm/include/arm-common/kvm-arch.h 
 b/arm/include/arm-common/kvm-arch.h
 index 082131d..90d6733 100644
 --- a/arm/include/arm-common/kvm-arch.h
 +++ b/arm/include/arm-common/kvm-arch.h
 @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 
 phys_addr)
  return phys_addr = KVM_IOPORT_AREA  phys_addr  limit;
  }
  
 -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
 -{
 -u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
 -return phys_addr = KVM_VIRTIO_MMIO_AREA  phys_addr  limit;
 -}
 -
 -static inline bool arm_addr_in_pci_region(u64 phys_addr)
 -{
 -u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
 -return phys_addr = KVM_PCI_CFG_AREA  phys_addr  limit;
 -}
 -
  struct kvm_arch {
  /*
   * We may have to align the guest memory for virtio, so keep the
 diff --git a/arm/include/arm-common/kvm-cpu-arch.h 
 b/arm/include/arm-common/kvm-cpu-arch.h
 index 36c7872..329979a 100644
 --- a/arm/include/arm-common/kvm-cpu-arch.h
 +++ b/arm/include/arm-common/kvm-cpu-arch.h
 @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu 
 *vcpu, u16 port, void *dat
  return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -   u32 len, u8 is_write);
 +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 
 phys_addr,
 + u8 *data, u32 len, u8 is_write)
 +{
 +if (arm_addr_in_ioport_region(phys_addr)) {
 +int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 +u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 +
 +return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 +}
 +
 +return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 +}
  
  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
  
 diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
 index ab08815..7780251 100644
 --- a/arm/kvm-cpu.c
 +++ b/arm/kvm-cpu.c
 @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
  return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -   u32 len, u8 is_write)
 -{
 -if (arm_addr_in_virtio_mmio_region(phys_addr)) {
 -return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 -} else if (arm_addr_in_ioport_region(phys_addr)) {
 -int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 -u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 -return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 -} else if (arm_addr_in_pci_region(phys_addr)) {
 -return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 -}
 
 Can you explain why this arm_addr_in_pci_region(phys_addr) check has
 disappeared in your updated version on this function? It may be a non
 issue, but I'd very much like to understand.

If you look above the calls to kvm__emulate_mmio() are exactly the same
for the PCI and the virtio_mmio region, also as the areas are
non-overlapping the if branches can be reordered.
arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
while arm_addr_in_pci_region() gives true between 1GB and 2GB.

So this translates into: do kvm__emulate_io() for anything below 64K and
kvm__emulate_mmio() for everything else except for the GIC area,
admittedly in a quite convoluted way.

So my patch just removes the check for the GIC region and rewrites it to
match that description in the last sentence, with the rationale given in
the commit message.
Does that make sense?
If you desperately want some extra barfing for misguided GIC requests,
I'd rather introduce that to the no match code path in
kvm__emulate_mmio or register some dummy MMIO regions for the GIC with

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

2015-06-17 Thread Marc Zyngier

On 17/06/15 14:49, Andre Przywara wrote:
 Hi Marc,
 
 On 06/17/2015 01:48 PM, Marc Zyngier wrote:
 On 17/06/15 12:21, Andre Przywara wrote:
 Currently we separate any incoming MMIO request into one of the ARM
 memory map regions and take care to spare the GIC.
 It turns out that this is unnecessary, as we only have one special
 region (the IO port area in the first 64 KByte). The MMIO rbtree
 takes care about unhandled MMIO ranges, so we can simply drop all the
 special range checking (except that for the IO range) in
 kvm_cpu__emulate_mmio().
 As the GIC is handled in the kernel, a GIC MMIO access should never
 reach userland (and we don't know what to do with it anyway).
 This lets us delete some more code and simplifies future extensions
 (like expanding the GIC regions).
 To be in line with the other architectures, move the now simpler
 code into a header file.

 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
  arm/include/arm-common/kvm-arch.h | 12 
  arm/include/arm-common/kvm-cpu-arch.h | 14 --
  arm/kvm-cpu.c | 16 
  3 files changed, 12 insertions(+), 30 deletions(-)

 diff --git a/arm/include/arm-common/kvm-arch.h 
 b/arm/include/arm-common/kvm-arch.h
 index 082131d..90d6733 100644
 --- a/arm/include/arm-common/kvm-arch.h
 +++ b/arm/include/arm-common/kvm-arch.h
 @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 
 phys_addr)
 return phys_addr = KVM_IOPORT_AREA  phys_addr  limit;
  }
  
 -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
 -{
 -   u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
 -   return phys_addr = KVM_VIRTIO_MMIO_AREA  phys_addr  limit;
 -}
 -
 -static inline bool arm_addr_in_pci_region(u64 phys_addr)
 -{
 -   u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
 -   return phys_addr = KVM_PCI_CFG_AREA  phys_addr  limit;
 -}
 -
  struct kvm_arch {
 /*
  * We may have to align the guest memory for virtio, so keep the
 diff --git a/arm/include/arm-common/kvm-cpu-arch.h 
 b/arm/include/arm-common/kvm-cpu-arch.h
 index 36c7872..329979a 100644
 --- a/arm/include/arm-common/kvm-cpu-arch.h
 +++ b/arm/include/arm-common/kvm-cpu-arch.h
 @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu 
 *vcpu, u16 port, void *dat
 return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -  u32 len, u8 is_write);
 +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 
 phys_addr,
 +u8 *data, u32 len, u8 is_write)
 +{
 +   if (arm_addr_in_ioport_region(phys_addr)) {
 +   int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 +   u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 +
 +   return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 +   }
 +
 +   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 +}
  
  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
  
 diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
 index ab08815..7780251 100644
 --- a/arm/kvm-cpu.c
 +++ b/arm/kvm-cpu.c
 @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
 return false;
  }
  
 -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
 -  u32 len, u8 is_write)
 -{
 -   if (arm_addr_in_virtio_mmio_region(phys_addr)) {
 -   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 -   } else if (arm_addr_in_ioport_region(phys_addr)) {
 -   int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
 -   u16 port = (phys_addr - KVM_IOPORT_AREA)  USHRT_MAX;
 -   return kvm__emulate_io(vcpu, port, data, direction, len, 1);
 -   } else if (arm_addr_in_pci_region(phys_addr)) {
 -   return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
 -   }

 Can you explain why this arm_addr_in_pci_region(phys_addr) check has
 disappeared in your updated version on this function? It may be a non
 issue, but I'd very much like to understand.
 
 If you look above the calls to kvm__emulate_mmio() are exactly the same
 for the PCI and the virtio_mmio region, also as the areas are
 non-overlapping the if branches can be reordered.
 arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
 while arm_addr_in_pci_region() gives true between 1GB and 2GB.
 
 So this translates into: do kvm__emulate_io() for anything below 64K and
 kvm__emulate_mmio() for everything else except for the GIC area,
 admittedly in a quite convoluted way.
 
 So my patch just removes the check for the GIC region and rewrites it to
 match that description in the last sentence, with the rationale given in
 the commit message.
 Does that make sense?
 If you desperately want some extra barfing for misguided GIC requests,
 I'd rather introduce that to the no match code path in
 kvm__emulate_mmio or register some

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[PATCH v3 06/10] arm: simplify MMIO dispatching

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

6 matches

Site Navigation

Mail list logo

Footer information