[v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tiejun Chen]

intel_iommu_map_page() does nothing if VT-d shares EPT page table.
So rmrr_identity_mapping() never create RMRR mapping but in some
cases like some GFX drivers it still need to access RMRR.

Here we will create those RMRR mappings even in shared EPT case.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

v3:

* Use set_mmio_p2m_entry() to replace p2m_set_entry()
* Use ASSERT to check

v2:

* Fix coding style.
* Still need to abide intel_iommu_map_page(), so we should do nothing
  if dom0 and iommu supports pass thru.

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 042b882..f08a1fc 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
 
     while ( base_pfn < end_pfn )
     {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
-                                  IOMMUF_readable|IOMMUF_writable) )
+        if ( iommu_use_hap_pt(d) )
+        {
+            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
+            if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
+                return -1;
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+                                       IOMMUF_readable|IOMMUF_writable) )
             return -1;
         base_pfn++;
     }
-- 
1.9.1

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tim Deegan]

At 19:00 +0800 on 24 Jul (1406224818), Tiejun Chen wrote:
> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
> So rmrr_identity_mapping() never create RMRR mapping but in some
> cases like some GFX drivers it still need to access RMRR.
> 
> Here we will create those RMRR mappings even in shared EPT case.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

For the interaction with p2m code:

Acked-by: Tim Deegn <tim@xen.org>

though I am still worried about what happens if this overwrites an
existing mapping. 

Tim.

> ---
>  xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> v3:
> 
> * Use set_mmio_p2m_entry() to replace p2m_set_entry()
> * Use ASSERT to check
> 
> v2:
> 
> * Fix coding style.
> * Still need to abide intel_iommu_map_page(), so we should do nothing
>   if dom0 and iommu supports pass thru.
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 042b882..f08a1fc 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
>  
>      while ( base_pfn < end_pfn )
>      {
> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> -                                  IOMMUF_readable|IOMMUF_writable) )
> +        if ( iommu_use_hap_pt(d) )
> +        {
> +            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
> +            if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
> +                return -1;
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +                                       IOMMUF_readable|IOMMUF_writable) )
>              return -1;
>          base_pfn++;
>      }
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 19:11, Tim Deegan wrote:
> At 19:00 +0800 on 24 Jul (1406224818), Tiejun Chen wrote:
>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>> So rmrr_identity_mapping() never create RMRR mapping but in some
>> cases like some GFX drivers it still need to access RMRR.
>>
>> Here we will create those RMRR mappings even in shared EPT case.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>
> For the interaction with p2m code:
>
> Acked-by: Tim Deegn <tim@xen.org>

Thanks a lot.

>
> though I am still worried about what happens if this overwrites an
> existing mapping.
>

As I understand RMRR should be specific. Its unlikely created for 
different mapping since this should be fixed in BIOS phase. And this is 
why that function is named as rmrr_identity_mapping().

Tiejun

> Tim.
>
>> ---
>>   xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> v3:
>>
>> * Use set_mmio_p2m_entry() to replace p2m_set_entry()
>> * Use ASSERT to check
>>
>> v2:
>>
>> * Fix coding style.
>> * Still need to abide intel_iommu_map_page(), so we should do nothing
>>    if dom0 and iommu supports pass thru.
>>
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
>> index 042b882..f08a1fc 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
>>
>>       while ( base_pfn < end_pfn )
>>       {
>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> -                                  IOMMUF_readable|IOMMUF_writable) )
>> +        if ( iommu_use_hap_pt(d) )
>> +        {
>> +            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
>> +            if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
>> +                return -1;
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>               return -1;
>>           base_pfn++;
>>       }
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tim Deegan]

At 19:25 +0800 on 24 Jul (1406226315), Chen, Tiejun wrote:
> On 2014/7/24 19:11, Tim Deegan wrote:
> > At 19:00 +0800 on 24 Jul (1406224818), Tiejun Chen wrote:
> >> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
> >> So rmrr_identity_mapping() never create RMRR mapping but in some
> >> cases like some GFX drivers it still need to access RMRR.
> >>
> >> Here we will create those RMRR mappings even in shared EPT case.
> >>
> >> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >
> > For the interaction with p2m code:
> >
> > Acked-by: Tim Deegn <tim@xen.org>
> 
> Thanks a lot.
> 
> >
> > though I am still worried about what happens if this overwrites an
> > existing mapping.
> >
> 
> As I understand RMRR should be specific. Its unlikely created for 
> different mapping since this should be fixed in BIOS phase. And this is 
> why that function is named as rmrr_identity_mapping().

But the BIOS only sets up _Xen's_ memory map.  The toolstack sets up
the _VM's_ memory map, and unless it can avoid the RMRRs of all 
devices in the system (and add them to guest E820s) it risks a clash
here.

If you can hot-plug one of these devices into a running guest, then
there's no way to be safe, as the guest might have been booted on
another system and migrated.

So I think it would be prudent to at least try to detect a clash here
and return an error. 

(BTW, I think this patch can go in as-is, and the address-space clash
be fixed up separately.)

Tim.

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 19:35, Tim Deegan wrote:
> At 19:25 +0800 on 24 Jul (1406226315), Chen, Tiejun wrote:
>> On 2014/7/24 19:11, Tim Deegan wrote:
>>> At 19:00 +0800 on 24 Jul (1406224818), Tiejun Chen wrote:
>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>> cases like some GFX drivers it still need to access RMRR.
>>>>
>>>> Here we will create those RMRR mappings even in shared EPT case.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> For the interaction with p2m code:
>>>
>>> Acked-by: Tim Deegn <tim@xen.org>
>>
>> Thanks a lot.
>>
>>>
>>> though I am still worried about what happens if this overwrites an
>>> existing mapping.
>>>
>>
>> As I understand RMRR should be specific. Its unlikely created for
>> different mapping since this should be fixed in BIOS phase. And this is
>> why that function is named as rmrr_identity_mapping().
>
> But the BIOS only sets up _Xen's_ memory map.  The toolstack sets up
> the _VM's_ memory map, and unless it can avoid the RMRRs of all
> devices in the system (and add them to guest E820s) it risks a clash
> here.

RMRR seems be used barely. Here in my case, just GFX passthrough needs 
this since some windows GFX drivers may access those stolen memory now.

>
> If you can hot-plug one of these devices into a running guest, then
> there's no way to be safe, as the guest might have been booted on
> another system and migrated.

I guess the original RMRR implementation don't consider live migration 
so current RMRR shouldn't support live migration, right?

So this should be anther story we need to address.

>
> So I think it would be prudent to at least try to detect a clash here
> and return an error.

Or do you already have some better ways to do this? I'd like to code 
them in another thread.

Tiejun

>
> (BTW, I think this patch can go in as-is, and the address-space clash
> be fixed up separately.)
>
> Tim.
>
>

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 13:51, <tiejun.chen@intel.com> wrote:
> On 2014/7/24 19:35, Tim Deegan wrote:
>> At 19:25 +0800 on 24 Jul (1406226315), Chen, Tiejun wrote:
>>> On 2014/7/24 19:11, Tim Deegan wrote:
>>>> though I am still worried about what happens if this overwrites an
>>>> existing mapping.
>>>
>>> As I understand RMRR should be specific. Its unlikely created for
>>> different mapping since this should be fixed in BIOS phase. And this is
>>> why that function is named as rmrr_identity_mapping().
>>
>> But the BIOS only sets up _Xen's_ memory map.  The toolstack sets up
>> the _VM's_ memory map, and unless it can avoid the RMRRs of all
>> devices in the system (and add them to guest E820s) it risks a clash
>> here.
> 
> RMRR seems be used barely. Here in my case, just GFX passthrough needs 
> this since some windows GFX drivers may access those stolen memory now.

USB legacy emulation is another use case iirc, as seen on at least
one of the systems I have here.

Furthermore an RMRR (as pointed out a couple of months ago)
may be related to more than one device (at least in theory), and
in such case it is insecure to assign these devices to distinct
domains.

As said in an earlier reply to Tim - I think this half baked solution
isn't worth checking in without the other issues with RMRRs
properly taken care of.

>> If you can hot-plug one of these devices into a running guest, then
>> there's no way to be safe, as the guest might have been booted on
>> another system and migrated.
> 
> I guess the original RMRR implementation don't consider live migration 
> so current RMRR shouldn't support live migration, right?

You didn't read Tim's reply properly: He was referring to the case
where a passed through device gets hotplugged into a guest for
the first time _after_ the guest was already live migrated at least
once.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/24 20:16, Jan Beulich wrote:
>>>> On 24.07.14 at 13:51, <tiejun.chen@intel.com> wrote:
>> On 2014/7/24 19:35, Tim Deegan wrote:
>>> At 19:25 +0800 on 24 Jul (1406226315), Chen, Tiejun wrote:
>>>> On 2014/7/24 19:11, Tim Deegan wrote:
>>>>> though I am still worried about what happens if this overwrites an
>>>>> existing mapping.
>>>>
>>>> As I understand RMRR should be specific. Its unlikely created for
>>>> different mapping since this should be fixed in BIOS phase. And this is
>>>> why that function is named as rmrr_identity_mapping().
>>>
>>> But the BIOS only sets up _Xen's_ memory map.  The toolstack sets up
>>> the _VM's_ memory map, and unless it can avoid the RMRRs of all
>>> devices in the system (and add them to guest E820s) it risks a clash
>>> here.
>>
>> RMRR seems be used barely. Here in my case, just GFX passthrough needs
>> this since some windows GFX drivers may access those stolen memory now.
>
> USB legacy emulation is another use case iirc, as seen on at least
> one of the systems I have here.
>
> Furthermore an RMRR (as pointed out a couple of months ago)

I'm poor in this problem, so could you point where I can get this 
discussion? I think I should take a look at that to know about more.

> may be related to more than one device (at least in theory), and

Are you saying one RMRR corresponds to multiple devfns?

> in such case it is insecure to assign these devices to distinct
> domains.

But looks we always create one RMRR when an associated devfn is assigned 
to one given domain, so this mean its always insecure before I introduce 
this patch, right?

But if I'm wrong please correct me.

>
> As said in an earlier reply to Tim - I think this half baked solution
> isn't worth checking in without the other issues with RMRRs
> properly taken care of.
>
>>> If you can hot-plug one of these devices into a running guest, then
>>> there's no way to be safe, as the guest might have been booted on
>>> another system and migrated.
>>
>> I guess the original RMRR implementation don't consider live migration
>> so current RMRR shouldn't support live migration, right?
>
> You didn't read Tim's reply properly: He was referring to the case
> where a passed through device gets hotplugged into a guest for
> the first time _after_ the guest was already live migrated at least
> once.
>

Thanks for you explanation.

Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 08:48, <tiejun.chen@intel.com> wrote:
> On 2014/7/24 20:16, Jan Beulich wrote:
>>>>> On 24.07.14 at 13:51, <tiejun.chen@intel.com> wrote:
>>> RMRR seems be used barely. Here in my case, just GFX passthrough needs
>>> this since some windows GFX drivers may access those stolen memory now.
>>
>> USB legacy emulation is another use case iirc, as seen on at least
>> one of the systems I have here.
>>
>> Furthermore an RMRR (as pointed out a couple of months ago)
> 
> I'm poor in this problem, so could you point where I can get this 
> discussion? I think I should take a look at that to know about more.

http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg00824.html
(continued in March; the list server doesn't properly deal with
cross-month follow-ups, so you'll need to search for the same
subject in
http://lists.xenproject.org/archives/html/xen-devel/2014-03/threads.html)

>> may be related to more than one device (at least in theory), and
> 
> Are you saying one RMRR corresponds to multiple devfns?

Yes, just look at acpi_parse_one_rmrr() and its helper
acpi_parse_dev_scope(): There's nothing there enforcing just
a single device per RMRR.

>> in such case it is insecure to assign these devices to distinct
>> domains.
> 
> But looks we always create one RMRR when an associated devfn is assigned 
> to one given domain, so this mean its always insecure before I introduce 
> this patch, right?

If you meant "already" instead of "always", then yes. Your patch
is just widening the issue.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/25 15:07, Jan Beulich wrote:
>>>> On 25.07.14 at 08:48, <tiejun.chen@intel.com> wrote:
>> On 2014/7/24 20:16, Jan Beulich wrote:
>>>>>> On 24.07.14 at 13:51, <tiejun.chen@intel.com> wrote:
>>>> RMRR seems be used barely. Here in my case, just GFX passthrough needs
>>>> this since some windows GFX drivers may access those stolen memory now.
>>>
>>> USB legacy emulation is another use case iirc, as seen on at least
>>> one of the systems I have here.
>>>
>>> Furthermore an RMRR (as pointed out a couple of months ago)
>>
>> I'm poor in this problem, so could you point where I can get this
>> discussion? I think I should take a look at that to know about more.
>
> http://lists.xenproject.org/archives/html/xen-devel/2014-02/msg00824.html
> (continued in March; the list server doesn't properly deal with
> cross-month follow-ups, so you'll need to search for the same
> subject in
> http://lists.xenproject.org/archives/html/xen-devel/2014-03/threads.html)
>
>>> may be related to more than one device (at least in theory), and
>>
>> Are you saying one RMRR corresponds to multiple devfns?
>
> Yes, just look at acpi_parse_one_rmrr() and its helper
> acpi_parse_dev_scope(): There's nothing there enforcing just
> a single device per RMRR.
>
>>> in such case it is insecure to assign these devices to distinct
>>> domains.
>>
>> But looks we always create one RMRR when an associated devfn is assigned
>> to one given domain, so this mean its always insecure before I introduce
>> this patch, right?
>
> If you meant "already" instead of "always", then yes. Your patch
> is just widening the issue.
>

Jan,

Such this kind of problem just happens in shared EPT case, right? In 
non-shared EPT case, we don't create these EPT tables to override other 
EPTs, so its okay?

Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 09:53, <tiejun.chen@intel.com> wrote:
> Such this kind of problem just happens in shared EPT case, right? In 
> non-shared EPT case, we don't create these EPT tables to override other 
> EPTs, so its okay?

At least I think/hope it is okay in the non-shared case - there's still
the problem of the two translation tables not being consistent with
one another, but I can't currently see problems arising from that.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tim Deegan]

At 09:02 +0100 on 25 Jul (1406275321), Jan Beulich wrote:
> >>> On 25.07.14 at 09:53, <tiejun.chen@intel.com> wrote:
> > Such this kind of problem just happens in shared EPT case, right? In 
> > non-shared EPT case, we don't create these EPT tables to override other 
> > EPTs, so its okay?
> 
> At least I think/hope it is okay in the non-shared case - there's still
> the problem of the two translation tables not being consistent with
> one another, but I can't currently see problems arising from that.

It's _not_ OK in the non-shared case: if a guest has RAM at that
address and uses a passed-through device to DMA to it, the DMA will
end up in the RMRR area.  Add we know that this guest has at least one
passed-through device. :P

Tim.

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 13:35, <tim@xen.org> wrote:
> (BTW, I think this patch can go in as-is, and the address-space clash
> be fixed up separately.)

Not sure - I agree it shouldn't make things worse except when there
is an RMRR but the device doesn't actually access it (i.e. would be
fine without this patch). But I'm not sure committing half solutions is
really appropriate for not really pressing issues.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-07-24:
>>>> On 24.07.14 at 13:35, <tim@xen.org> wrote:
>> (BTW, I think this patch can go in as-is, and the address-space
>> clash be fixed up separately.)
> 
> Not sure - I agree it shouldn't make things worse except when there is
> an RMRR but the device doesn't actually access it (i.e. would be fine
> without this patch). But I'm not sure committing half solutions is
> really appropriate for not really pressing issues.

I think we are talking two issues. The address-apace clash has nothing to do with the issue this patch tries to fix. We will work out another patch to fix it as Tim suggested.

> 
> Jan


Best regards,
Yang

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 02:56, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-07-24:
>>>>> On 24.07.14 at 13:35, <tim@xen.org> wrote:
>>> (BTW, I think this patch can go in as-is, and the address-space
>>> clash be fixed up separately.)
>> 
>> Not sure - I agree it shouldn't make things worse except when there is
>> an RMRR but the device doesn't actually access it (i.e. would be fine
>> without this patch). But I'm not sure committing half solutions is
>> really appropriate for not really pressing issues.
> 
> I think we are talking two issues. The address-apace clash has nothing to do 
> with the issue this patch tries to fix. We will work out another patch to fix 
> it as Tim suggested.

No, we aren't: This patch _introduces_ the address space clash.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-07-25:
>>>> On 25.07.14 at 02:56, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-07-24:
>>>>>> On 24.07.14 at 13:35, <tim@xen.org> wrote:
>>>> (BTW, I think this patch can go in as-is, and the address-space
>>>> clash be fixed up separately.)
>>> 
>>> Not sure - I agree it shouldn't make things worse except when there
>>> is an RMRR but the device doesn't actually access it (i.e. would be
>>> fine without this patch). But I'm not sure committing half
>>> solutions is really appropriate for not really pressing issues.
>> 
>> I think we are talking two issues. The address-apace clash has
>> nothing to do with the issue this patch tries to fix. We will work
>> out another patch to fix it as Tim suggested.
> 
> No, we aren't: This patch _introduces_ the address space clash.

In non-sharept case, do we reserve the RMRR in e820 table? If not, the guest may use it as DMA buffer and it actually the same problem we are talking here. 

> 
> Jan


Best regards,
Yang

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 10:19, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-07-25:
>>>>> On 25.07.14 at 02:56, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-07-24:
>>>>>>> On 24.07.14 at 13:35, <tim@xen.org> wrote:
>>>>> (BTW, I think this patch can go in as-is, and the address-space
>>>>> clash be fixed up separately.)
>>>> 
>>>> Not sure - I agree it shouldn't make things worse except when there
>>>> is an RMRR but the device doesn't actually access it (i.e. would be
>>>> fine without this patch). But I'm not sure committing half
>>>> solutions is really appropriate for not really pressing issues.
>>> 
>>> I think we are talking two issues. The address-apace clash has
>>> nothing to do with the issue this patch tries to fix. We will work
>>> out another patch to fix it as Tim suggested.
>> 
>> No, we aren't: This patch _introduces_ the address space clash.
> 
> In non-sharept case, do we reserve the RMRR in e820 table? If not, the guest 
> may use it as DMA buffer and it actually the same problem we are talking 
> here. 

True, indeed. Yet I still consider the patch is fixing a problem at the
price of making that situation worse.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

> So I think it would be prudent to at least try to detect a clash here
> and return an error.

Could we check if this pte already present like this?

@@ -1870,6 +1872,16 @@ static int rmrr_identity_mapping(struct domain *d,
          if ( iommu_use_hap_pt(d) )
          {
              ASSERT(!iommu_passthrough || !is_hardware_domain(d));
+            /* get last level pte */
+            pg_maddr = addr_to_dma_page_maddr(d, ((paddr_t)gfn << 
PAGE_SHIFT_4K), 0);
+            page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
+            pte = page + address_level_offset(addr, 1);
+            if ( dma_pte_present(*pte) )
+            {
+                printk(XENLOG_ERR VTDPREFIX
+                       "Overlapping RMRRs at %"PRIx64".\n", 
(paddr_t)base_gfn);
+                return -1;
+            }
              if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
                  return -1;
          }

Thanks
Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 08:47, <tiejun.chen@intel.com> wrote:
>>  So I think it would be prudent to at least try to detect a clash here
>> and return an error.
> 
> Could we check if this pte already present like this?
> 
> @@ -1870,6 +1872,16 @@ static int rmrr_identity_mapping(struct domain *d,
>           if ( iommu_use_hap_pt(d) )
>           {
>               ASSERT(!iommu_passthrough || !is_hardware_domain(d));
> +            /* get last level pte */
> +            pg_maddr = addr_to_dma_page_maddr(d, ((paddr_t)gfn << PAGE_SHIFT_4K), 0);
> +            page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
> +            pte = page + address_level_offset(addr, 1);
> +            if ( dma_pte_present(*pte) )
> +            {
> +                printk(XENLOG_ERR VTDPREFIX
> +                       "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)base_gfn);
> +                return -1;
> +            }
>               if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
>                   return -1;
>           }

I think you'd be better off using P2M functions here, even more so
considering that you're in an iommu_use_hap_pt() guarded code
section.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/25 16:07, Jan Beulich wrote:
>>>> On 25.07.14 at 08:47, <tiejun.chen@intel.com> wrote:
>>>   So I think it would be prudent to at least try to detect a clash here
>>> and return an error.
>>
>> Could we check if this pte already present like this?
>>
>> @@ -1870,6 +1872,16 @@ static int rmrr_identity_mapping(struct domain *d,
>>            if ( iommu_use_hap_pt(d) )
>>            {
>>                ASSERT(!iommu_passthrough || !is_hardware_domain(d));
>> +            /* get last level pte */
>> +            pg_maddr = addr_to_dma_page_maddr(d, ((paddr_t)gfn << PAGE_SHIFT_4K), 0);
>> +            page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
>> +            pte = page + address_level_offset(addr, 1);
>> +            if ( dma_pte_present(*pte) )
>> +            {
>> +                printk(XENLOG_ERR VTDPREFIX
>> +                       "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)base_gfn);
>> +                return -1;
>> +            }
>>                if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
>>                    return -1;
>>            }
>
> I think you'd be better off using P2M functions here, even more so
> considering that you're in an iommu_use_hap_pt() guarded code
> section.

Are you pointing p2m->get_entry?

Thanks
Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 10:45, <tiejun.chen@intel.com> wrote:
> On 2014/7/25 16:07, Jan Beulich wrote:
>>>>> On 25.07.14 at 08:47, <tiejun.chen@intel.com> wrote:
>>>>   So I think it would be prudent to at least try to detect a clash here
>>>> and return an error.
>>>
>>> Could we check if this pte already present like this?
>>>
>>> @@ -1870,6 +1872,16 @@ static int rmrr_identity_mapping(struct domain *d,
>>>            if ( iommu_use_hap_pt(d) )
>>>            {
>>>                ASSERT(!iommu_passthrough || !is_hardware_domain(d));
>>> +            /* get last level pte */
>>> +            pg_maddr = addr_to_dma_page_maddr(d, ((paddr_t)gfn << PAGE_SHIFT_4K), 0);
>>> +            page = (struct dma_pte *)map_vtd_domain_page(pg_maddr);
>>> +            pte = page + address_level_offset(addr, 1);
>>> +            if ( dma_pte_present(*pte) )
>>> +            {
>>> +                printk(XENLOG_ERR VTDPREFIX
>>> +                       "Overlapping RMRRs at %"PRIx64".\n", (paddr_t)base_gfn);
>>> +                return -1;
>>> +            }
>>>                if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
>>>                    return -1;
>>>            }
>>
>> I think you'd be better off using P2M functions here, even more so
>> considering that you're in an iommu_use_hap_pt() guarded code
>> section.
> 
> Are you pointing p2m->get_entry?

Sort of. For the check to really be meaningful the get and set
would need to be done without the respective lock dropped
intermediately.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Thursday, July 24, 2014 4:00 AM
> 
> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
> So rmrr_identity_mapping() never create RMRR mapping but in some
> cases like some GFX drivers it still need to access RMRR.

this is not accurate. as long as RMRR is reported, it's always necessary
to have the identity mapping there, not just needed in SOME CASES. :-)

Acked-by: Kevin Tian <kevin.tian@intel.com>, but please also work out
a solution for address overlapping issue like Tim pointed. out.

Thanks
Kevin

> 
> Here we will create those RMRR mappings even in shared EPT case.
> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> v3:
> 
> * Use set_mmio_p2m_entry() to replace p2m_set_entry()
> * Use ASSERT to check
> 
> v2:
> 
> * Fix coding style.
> * Still need to abide intel_iommu_map_page(), so we should do nothing
>   if dom0 and iommu supports pass thru.
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index 042b882..f08a1fc 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1867,8 +1867,14 @@ static int rmrr_identity_mapping(struct domain *d,
> 
>      while ( base_pfn < end_pfn )
>      {
> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> -
> IOMMUF_readable|IOMMUF_writable) )
> +        if ( iommu_use_hap_pt(d) )
> +        {
> +            ASSERT(!iommu_passthrough || !is_hardware_domain(d));
> +            if ( set_mmio_p2m_entry(d, base_pfn, _mfn(base_pfn)) )
> +                return -1;
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +
> IOMMUF_readable|IOMMUF_writable) )
>              return -1;
>          base_pfn++;
>      }
> --
> 1.9.1

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>  From: Chen, Tiejun
>> Sent: Thursday, July 24, 2014 4:00 AM
>> 
>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>> So rmrr_identity_mapping() never create RMRR mapping but in some
>> cases like some GFX drivers it still need to access RMRR.
> 
> this is not accurate. as long as RMRR is reported, it's always necessary
> to have the identity mapping there, not just needed in SOME CASES. :-)

Actually (as also said before) I think "in some cases" is quite correct:
There's no guarantee that a device will actually access the region(s)
an RMRR may specify for it. A particular example would be the USB
case where iiuc these regions are needed only until legacy mode
emulation gets turned off.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-07-25:
>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>> From: Chen, Tiejun
>>> Sent: Thursday, July 24, 2014 4:00 AM
>>> 
>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>> cases like some GFX drivers it still need to access RMRR.
>> 
>> this is not accurate. as long as RMRR is reported, it's always
>> necessary to have the identity mapping there, not just needed in
>> SOME CASES. :-)
> 
> Actually (as also said before) I think "in some cases" is quite correct:
> There's no guarantee that a device will actually access the region(s)
> an RMRR may specify for it. A particular example would be the USB case
> where iiuc these regions are needed only until legacy mode emulation gets turned off.

Yes, even for Intel GFX, the RMRR is never really accessed. So we never saw any issue even without this patch.

> 
> Jan


Best regards,
Yang

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/25 16:24, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-07-25:
>>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>>> From: Chen, Tiejun
>>>> Sent: Thursday, July 24, 2014 4:00 AM
>>>>
>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>> cases like some GFX drivers it still need to access RMRR.
>>>
>>> this is not accurate. as long as RMRR is reported, it's always
>>> necessary to have the identity mapping there, not just needed in
>>> SOME CASES. :-)
>>
>> Actually (as also said before) I think "in some cases" is quite correct:
>> There's no guarantee that a device will actually access the region(s)
>> an RMRR may specify for it. A particular example would be the USB case
>> where iiuc these regions are needed only until legacy mode emulation gets turned off.
>
> Yes, even for Intel GFX, the RMRR is never really accessed. So we never saw any issue even without this patch.
>

Yang,

Windows GFX driver really accesses this range now. Without this patch, 
the blue screen appears.

Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Chen, Tiejun wrote on 2014-07-25:
> On 2014/7/25 16:24, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2014-07-25:
>>>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>>>> From: Chen, Tiejun
>>>>> Sent: Thursday, July 24, 2014 4:00 AM
>>>>> 
>>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>>> cases like some GFX drivers it still need to access RMRR.
>>>> 
>>>> this is not accurate. as long as RMRR is reported, it's always
>>>> necessary to have the identity mapping there, not just needed in
>>>> SOME CASES. :-)
>>> 
>>> Actually (as also said before) I think "in some cases" is quite correct:
>>> There's no guarantee that a device will actually access the
>>> region(s) an RMRR may specify for it. A particular example would be
>>> the USB case where iiuc these regions are needed only until legacy
>>> mode emulation
> gets turned off.
>> 
>> Yes, even for Intel GFX, the RMRR is never really accessed. So we never
>> saw any issue even without this patch.
>> 
> 
> Yang,
> 
> Windows GFX driver really accesses this range now. Without this patch,
> the blue screen appears.

Okay, so for Linux guest, it works well even without having RMRR mapping.

> 
> Tiejun


Best regards,
Yang

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/25 16:28, Zhang, Yang Z wrote:
> Chen, Tiejun wrote on 2014-07-25:
>> On 2014/7/25 16:24, Zhang, Yang Z wrote:
>>> Jan Beulich wrote on 2014-07-25:
>>>>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>>>>> From: Chen, Tiejun
>>>>>> Sent: Thursday, July 24, 2014 4:00 AM
>>>>>>
>>>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>>>> cases like some GFX drivers it still need to access RMRR.
>>>>>
>>>>> this is not accurate. as long as RMRR is reported, it's always
>>>>> necessary to have the identity mapping there, not just needed in
>>>>> SOME CASES. :-)
>>>>
>>>> Actually (as also said before) I think "in some cases" is quite correct:
>>>> There's no guarantee that a device will actually access the
>>>> region(s) an RMRR may specify for it. A particular example would be
>>>> the USB case where iiuc these regions are needed only until legacy
>>>> mode emulation
>> gets turned off.
>>>
>>> Yes, even for Intel GFX, the RMRR is never really accessed. So we never
>>> saw any issue even without this patch.
>>>
>>
>> Yang,
>>
>> Windows GFX driver really accesses this range now. Without this patch,
>> the blue screen appears.
>
> Okay, so for Linux guest, it works well even without having RMRR mapping.
>

Yes.

Tiejun

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.07.14 at 10:24, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-07-25:
>>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>>> From: Chen, Tiejun
>>>> Sent: Thursday, July 24, 2014 4:00 AM
>>>> 
>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>> cases like some GFX drivers it still need to access RMRR.
>>> 
>>> this is not accurate. as long as RMRR is reported, it's always
>>> necessary to have the identity mapping there, not just needed in
>>> SOME CASES. :-)
>> 
>> Actually (as also said before) I think "in some cases" is quite correct:
>> There's no guarantee that a device will actually access the region(s)
>> an RMRR may specify for it. A particular example would be the USB case
>> where iiuc these regions are needed only until legacy mode emulation gets 
> turned off.
> 
> Yes, even for Intel GFX, the RMRR is never really accessed. So we never saw 
> any issue even without this patch.

So then what's the patch good for? If it's just addressing a theoretical
issue, then this supports my intention to commit it only when the other
(theoretical only) problem also gets taken care of.

Jan

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/25 16:36, Jan Beulich wrote:
>>>> On 25.07.14 at 10:24, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-07-25:
>>>>>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
>>>>>> From: Chen, Tiejun
>>>>> Sent: Thursday, July 24, 2014 4:00 AM
>>>>>
>>>>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>>>>> So rmrr_identity_mapping() never create RMRR mapping but in some
>>>>> cases like some GFX drivers it still need to access RMRR.
>>>>
>>>> this is not accurate. as long as RMRR is reported, it's always
>>>> necessary to have the identity mapping there, not just needed in
>>>> SOME CASES. :-)
>>>
>>> Actually (as also said before) I think "in some cases" is quite correct:
>>> There's no guarantee that a device will actually access the region(s)
>>> an RMRR may specify for it. A particular example would be the USB case
>>> where iiuc these regions are needed only until legacy mode emulation gets
>> turned off.
>>
>> Yes, even for Intel GFX, the RMRR is never really accessed. So we never saw
>> any issue even without this patch.
>
> So then what's the patch good for? If it's just addressing a theoretical

No, this is not a theoretical problem.

I already reply to Yang, on BDW windows GFX driver really accesses this 
range now. Without this patch, the blue screen appears.

Tiejun

> issue, then this supports my intention to commit it only when the other
> (theoretical only) problem also gets taken care of.
>
> Jan
>
>
>

Re: [v3][PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, July 25, 2014 1:16 AM
> 
> >>> On 24.07.14 at 19:12, <kevin.tian@intel.com> wrote:
> >>  From: Chen, Tiejun
> >> Sent: Thursday, July 24, 2014 4:00 AM
> >>
> >> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
> >> So rmrr_identity_mapping() never create RMRR mapping but in some
> >> cases like some GFX drivers it still need to access RMRR.
> >
> > this is not accurate. as long as RMRR is reported, it's always necessary
> > to have the identity mapping there, not just needed in SOME CASES. :-)
> 
> Actually (as also said before) I think "in some cases" is quite correct:
> There's no guarantee that a device will actually access the region(s)
> an RMRR may specify for it. A particular example would be the USB
> case where iiuc these regions are needed only until legacy mode
> emulation gets turned off.
> 

the device may or may not access RMRR, but from virtualization p.o.v
as long as RMRR presents and you assign a device associated, we should
ALWAYS setup RMRR mapping since we don't know how device actually
work.

Thanks
Kevin