Re: [Xen-devel] [PATCH v5 20/22] xen/arm: ITS: Map ITS translation space

[Ian Campbell]

On Tue, 2015-08-18 at 23:37 +0100, Marc Zyngier wrote:
> On Tue, 18 Aug 2015 20:14:43 +0100 Julien Grall  
> wrote:
> 
> > Marc pointed me today that if the processor is writing into 
> > GITS_TRANSLATER it may be able to deadlock the system.
> > 
> > Reading more closely the spec (8.1.3 IHI0069A), there is undefined 
> > behavior when writing to this register with wrong access size.
> > 
> > Currently the page table are shared between the processor and the SMMU, 
> > 
> > so that means that a domain will be able to deadlock the processor and 
> > therefore the whole platform.
> 
> Indeed. A CPU should *never* be able to write to the GITS_TRANSLATER
> register. What would be the meaning anyway? How would a DeviceID be
> sampled? This is definitely UNPREDICTIBLE territory, and you want to
> make sure a guest cannot directly write to the HW.
> 
> > So we should never expose GITS_TRANSLATER into the processor page 
> > table. 
> > Which means unsharing some parts if not all of the page tables between 
> > the processor and the SMMU.
> 
> Agreed. It looks to me like the CPU should only see the the virtual
> ITS, and nothing else.

It's rather unfortunate that using an ITS therefore precludes sharing stage
-2 page tables between MMU and SMMU, which it seems otherwise the
architecture designers have tried hard to allow.

Do you know if this will be fixed in some future revision (although given
we now need to have the functionality anyway I'm not sure it help more than
 saving a few pages of memory :-()

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 20/22] xen/arm: ITS: Map ITS translation space

[Marc Zyngier]

On 02/09/15 16:45, Ian Campbell wrote:
> On Tue, 2015-08-18 at 23:37 +0100, Marc Zyngier wrote:
>> On Tue, 18 Aug 2015 20:14:43 +0100 Julien Grall  
>> wrote:
>>
>>> Marc pointed me today that if the processor is writing into 
>>> GITS_TRANSLATER it may be able to deadlock the system.
>>>
>>> Reading more closely the spec (8.1.3 IHI0069A), there is undefined 
>>> behavior when writing to this register with wrong access size.
>>>
>>> Currently the page table are shared between the processor and the SMMU, 
>>>
>>> so that means that a domain will be able to deadlock the processor and 
>>> therefore the whole platform.
>>
>> Indeed. A CPU should *never* be able to write to the GITS_TRANSLATER
>> register. What would be the meaning anyway? How would a DeviceID be
>> sampled? This is definitely UNPREDICTIBLE territory, and you want to
>> make sure a guest cannot directly write to the HW.
>>
>>> So we should never expose GITS_TRANSLATER into the processor page 
>>> table. 
>>> Which means unsharing some parts if not all of the page tables between 
>>> the processor and the SMMU.
>>
>> Agreed. It looks to me like the CPU should only see the the virtual
>> ITS, and nothing else.
> 
> It's rather unfortunate that using an ITS therefore precludes sharing stage
> -2 page tables between MMU and SMMU, which it seems otherwise the
> architecture designers have tried hard to allow.
> 
> Do you know if this will be fixed in some future revision (although given
> we now need to have the functionality anyway I'm not sure it help more than
>  saving a few pages of memory :-()

I don't have any idea if something is being worked on to address this,
but I think you may be able to share at least the page tables describing
the memory, which should really be the bulk of the page tables.

M.
-- 
Jazz is not dead. It just smells funny...

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 20/22] xen/arm: ITS: Map ITS translation space

[Julien Grall]

Hi,

On 27/07/2015 04:12, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K vijaya.ku...@caviumnetworks.com

ITS translation space contains GITS_TRANSLATOR register


s/GITS_TRANSLATOR/GITS_TRANSLATOR/


which is written by device to raise LPI. This space needs
to mapped to every domain address space for all physical
ITS available,so that device can access GITS_TRANSLATOR


Ditto


register using SMMU.


Marc pointed me today that if the processor is writing into 
GITS_TRANSLATER it may be able to deadlock the system.


Reading more closely the spec (8.1.3 IHI0069A), there is undefined 
behavior when writing to this register with wrong access size.


Currently the page table are shared between the processor and the SMMU, 
so that means that a domain will be able to deadlock the processor and 
therefore the whole platform.


So we should never expose GITS_TRANSLATER into the processor page table. 
Which means unsharing some parts if not all of the page tables between 
the processor and the SMMU.


While it's not required for this series, you don't support guest, this 
would be mandatory to fix it before any usage of the vITS by a guest.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 20/22] xen/arm: ITS: Map ITS translation space

[Marc Zyngier]

On Tue, 18 Aug 2015 20:14:43 +0100
Julien Grall julien.gr...@citrix.com wrote:

 Hi,
 
 On 27/07/2015 04:12, vijay.kil...@gmail.com wrote:
  From: Vijaya Kumar K vijaya.ku...@caviumnetworks.com
 
  ITS translation space contains GITS_TRANSLATOR register
 
 s/GITS_TRANSLATOR/GITS_TRANSLATOR/

I assume you mean GITS_TRANSLATER? ;-)
 
  which is written by device to raise LPI. This space needs
  to mapped to every domain address space for all physical
  ITS available,so that device can access GITS_TRANSLATOR
 
 Ditto
 
  register using SMMU.
 
 Marc pointed me today that if the processor is writing into 
 GITS_TRANSLATER it may be able to deadlock the system.
 
 Reading more closely the spec (8.1.3 IHI0069A), there is undefined 
 behavior when writing to this register with wrong access size.
 
 Currently the page table are shared between the processor and the SMMU, 
 so that means that a domain will be able to deadlock the processor and 
 therefore the whole platform.

Indeed. A CPU should *never* be able to write to the GITS_TRANSLATER
register. What would be the meaning anyway? How would a DeviceID be
sampled? This is definitely UNPREDICTIBLE territory, and you want to
make sure a guest cannot directly write to the HW.

 So we should never expose GITS_TRANSLATER into the processor page table. 
 Which means unsharing some parts if not all of the page tables between 
 the processor and the SMMU.

Agreed. It looks to me like the CPU should only see the the virtual
ITS, and nothing else.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 20/22] xen/arm: ITS: Map ITS translation space

[Julien Grall]

Hi Vijay,

On 27/07/2015 04:12, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K vijaya.ku...@caviumnetworks.com

ITS translation space contains GITS_TRANSLATOR register
which is written by device to raise LPI. This space needs
to mapped to every domain address space for all physical
ITS available,so that device can access GITS_TRANSLATOR
register using SMMU.

Signed-off-by: Vijaya Kumar K vijaya.ku...@caviumnetworks.com
Acked-by: Ian Campbell ian.campb...@citrix.com
---
  xen/arch/arm/vgic-v3-its.c |   38 +-
  1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index e182cee..27523f4 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -1060,6 +1060,42 @@ static const struct mmio_handler_ops 
vgic_gits_mmio_handler = {
  .write_handler = vgic_v3_gits_mmio_write,
  };

+/*
+ * Map the 64K ITS translation space in guest.
+ * This is required purely for device smmu writes.
+*/
+
+static int vits_map_translation_space(struct domain *d)
+{
+uint64_t addr, size;
+int ret;
+
+if ( !is_hardware_domain(d) )


Well this code is surely wrong. If you happen to have a guest domain, 
you won't map anything and say you've done it.


Given that vits_domain_init is only called for DOM0, you should drop 
this check.



+return 0;
+
+ASSERT(is_domain_direct_mapped(d));
+
+addr = d-arch.vgic.vits-gits_base + SZ_64K;
+size = SZ_64K;
+
+/* Using 1:1 mapping to map translation space */
+/* TODO: Handle DomU mapping */
+ret = map_mmio_regions(d,
+   paddr_to_pfn(addr  PAGE_MASK),
+   DIV_ROUND_UP(size, PAGE_SIZE),
+   paddr_to_pfn(addr  PAGE_MASK));
+
+if ( ret )
+{
+ dprintk(XENLOG_G_ERR, vITS: Unable to map to dom%d access to
+  0x%PRIx64 - 0x%PRIx64\n,


Unable to map 0x...-0x... to dom%d


+ d-domain_id,
+ addr  PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+}
+
+return ret;
+}
+
  int vits_domain_init(struct domain *d)
  {
  struct vgic_its *vits;
@@ -1120,7 +1156,7 @@ int vits_domain_init(struct domain *d)

  register_mmio_handler(d, vgic_gits_mmio_handler, vits-gits_base, 
SZ_64K);

-return 0;
+return vits_map_translation_space(d);
  }

  void vits_domain_free(struct domain *d)



Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel