Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

>  >> It should suffice to give 3 Gb (or event slightly less) of memory to
>  >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>  >
>  > Yes. So I can't produce that real case of conflict with those existing
>  > RMRR in my platform.
>
> When you pass 3Gb to the guest, its memory map should extend to
> about 0xC0000000, well beyond the range the RMRRs reference. So

Yes. So I set memory size as 2816M which also cover all RMRR ranges in 
my platform.

> you ought to be able to see the collision (or if you don't you ought to
> have ways to find out why they're not happening, as that would be a
> sign of something else being bogus).
>

Then I can see that work as we expect:

# xl cr hvm.cfg
Parsing config from hvm.cfg
libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed: 
Operation not permitted
libxl: error: libxl_create.c:1329:domcreate_attach_pci: 
libxl_device_pci_add failed: -3

And

# xl dmesg
...
(XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
(XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
(XEN) Cannot identity map d1:ad000, already mapped to 115d51.
(XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
(XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
(XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
(XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
...

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>   >> It should suffice to give 3 Gb (or event slightly less) of memory to
>>  >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>>  >
>>  > Yes. So I can't produce that real case of conflict with those existing
>>  > RMRR in my platform.
>>
>> When you pass 3Gb to the guest, its memory map should extend to
>> about 0xC0000000, well beyond the range the RMRRs reference. So
> 
> Yes. So I set memory size as 2816M which also cover all RMRR ranges in 
> my platform.
> 
>> you ought to be able to see the collision (or if you don't you ought to
>> have ways to find out why they're not happening, as that would be a
>> sign of something else being bogus).
>>
> 
> Then I can see that work as we expect:
> 
> # xl cr hvm.cfg
> Parsing config from hvm.cfg
> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed: 
> Operation not permitted
> libxl: error: libxl_create.c:1329:domcreate_attach_pci: 
> libxl_device_pci_add failed: -3
> 
> And
> 
> # xl dmesg
> ...
> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
> ...

So after all device assignment fails in that case, which is what I was
expecting to happen. Which gets me back to the question: What's
the value of the two patches for you if with them you can't pass
through anymore the device you want passed through for the actual
work you're doing?

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/22 16:53, Jan Beulich wrote:
>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>    >> It should suffice to give 3 Gb (or event slightly less) of memory to
>>>   >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>>>   >
>>>   > Yes. So I can't produce that real case of conflict with those existing
>>>   > RMRR in my platform.
>>>
>>> When you pass 3Gb to the guest, its memory map should extend to
>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>
>> Yes. So I set memory size as 2816M which also cover all RMRR ranges in
>> my platform.
>>
>>> you ought to be able to see the collision (or if you don't you ought to
>>> have ways to find out why they're not happening, as that would be a
>>> sign of something else being bogus).
>>>
>>
>> Then I can see that work as we expect:
>>
>> # xl cr hvm.cfg
>> Parsing config from hvm.cfg
>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>> Operation not permitted
>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>> libxl_device_pci_add failed: -3
>>
>> And
>>
>> # xl dmesg
>> ...
>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
>> ...
>
> So after all device assignment fails in that case, which is what I was
> expecting to happen. Which gets me back to the question: What's
> the value of the two patches for you if with them you can't pass
> through anymore the device you want passed through for the actual
> work you're doing?
>

I don't understand what you mean again. This is true we already known 
previously because this is just a part of the whole solution, right? So 
I can't understand why we can't apply them now unless you're saying 
they're wrong.

When we want to implement a bigger or complicated feature, I think its 
reasonable to phase that with multiple steps.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
> On 2014/9/22 16:53, Jan Beulich wrote:
>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>>    >> It should suffice to give 3 Gb (or event slightly less) of memory to
>>>>   >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>>>>   >
>>>>   > Yes. So I can't produce that real case of conflict with those existing
>>>>   > RMRR in my platform.
>>>>
>>>> When you pass 3Gb to the guest, its memory map should extend to
>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>>
>>> Yes. So I set memory size as 2816M which also cover all RMRR ranges in
>>> my platform.
>>>
>>>> you ought to be able to see the collision (or if you don't you ought to
>>>> have ways to find out why they're not happening, as that would be a
>>>> sign of something else being bogus).
>>>>
>>>
>>> Then I can see that work as we expect:
>>>
>>> # xl cr hvm.cfg
>>> Parsing config from hvm.cfg
>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>>> Operation not permitted
>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>>> libxl_device_pci_add failed: -3
>>>
>>> And
>>>
>>> # xl dmesg
>>> ...
>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
>>> ...
>>
>> So after all device assignment fails in that case, which is what I was
>> expecting to happen. Which gets me back to the question: What's
>> the value of the two patches for you if with them you can't pass
>> through anymore the device you want passed through for the actual
>> work you're doing?
> 
> I don't understand what you mean again. This is true we already known 
> previously because this is just a part of the whole solution, right? So 
> I can't understand why we can't apply them now unless you're saying 
> they're wrong.

You want these two patches applied despite having acknowledged
that even for you they cause a regression (at the very least an
apparent one).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/22 18:36, Jan Beulich wrote:
>>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
>> On 2014/9/22 16:53, Jan Beulich wrote:
>>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>>>     >> It should suffice to give 3 Gb (or event slightly less) of memory to
>>>>>    >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>>>>>    >
>>>>>    > Yes. So I can't produce that real case of conflict with those existing
>>>>>    > RMRR in my platform.
>>>>>
>>>>> When you pass 3Gb to the guest, its memory map should extend to
>>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>>>
>>>> Yes. So I set memory size as 2816M which also cover all RMRR ranges in
>>>> my platform.
>>>>
>>>>> you ought to be able to see the collision (or if you don't you ought to
>>>>> have ways to find out why they're not happening, as that would be a
>>>>> sign of something else being bogus).
>>>>>
>>>>
>>>> Then I can see that work as we expect:
>>>>
>>>> # xl cr hvm.cfg
>>>> Parsing config from hvm.cfg
>>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>>>> Operation not permitted
>>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>>>> libxl_device_pci_add failed: -3
>>>>
>>>> And
>>>>
>>>> # xl dmesg
>>>> ...
>>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
>>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
>>>> ...
>>>
>>> So after all device assignment fails in that case, which is what I was
>>> expecting to happen. Which gets me back to the question: What's
>>> the value of the two patches for you if with them you can't pass
>>> through anymore the device you want passed through for the actual
>>> work you're doing?
>>
>> I don't understand what you mean again. This is true we already known
>> previously because this is just a part of the whole solution, right? So
>> I can't understand why we can't apply them now unless you're saying
>> they're wrong.
>
> You want these two patches applied despite having acknowledged
> that even for you they cause a regression (at the very least an
> apparent one).
>

Why did you say this is a regression?

Without these two patches, any assigned device with RMRR dependency 
can't work at all since RMRR table never be created. But if we apply 
these two patches, RMRR table can be created safely, right? Then the 
assigned device can work based on them.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 23.09.14 at 03:56, <tiejun.chen@intel.com> wrote:
> On 2014/9/22 18:36, Jan Beulich wrote:
>>>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/22 16:53, Jan Beulich wrote:
>>>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>>>>     >> It should suffice to give 3 Gb (or event slightly less) of memory to
>>>>>>    >> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
>>>>>>    >
>>>>>>    > Yes. So I can't produce that real case of conflict with those existing
>>>>>>    > RMRR in my platform.
>>>>>>
>>>>>> When you pass 3Gb to the guest, its memory map should extend to
>>>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>>>>
>>>>> Yes. So I set memory size as 2816M which also cover all RMRR ranges in
>>>>> my platform.
>>>>>
>>>>>> you ought to be able to see the collision (or if you don't you ought to
>>>>>> have ways to find out why they're not happening, as that would be a
>>>>>> sign of something else being bogus).
>>>>>>
>>>>>
>>>>> Then I can see that work as we expect:
>>>>>
>>>>> # xl cr hvm.cfg
>>>>> Parsing config from hvm.cfg
>>>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>>>>> Operation not permitted
>>>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>>>>> libxl_device_pci_add failed: -3
>>>>>
>>>>> And
>>>>>
>>>>> # xl dmesg
>>>>> ...
>>>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>>>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>>>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>>>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>>>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed (-1)
>>>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>>>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
>>>>> ...
>>>>
>>>> So after all device assignment fails in that case, which is what I was
>>>> expecting to happen. Which gets me back to the question: What's
>>>> the value of the two patches for you if with them you can't pass
>>>> through anymore the device you want passed through for the actual
>>>> work you're doing?
>>>
>>> I don't understand what you mean again. This is true we already known
>>> previously because this is just a part of the whole solution, right? So
>>> I can't understand why we can't apply them now unless you're saying
>>> they're wrong.
>>
>> You want these two patches applied despite having acknowledged
>> that even for you they cause a regression (at the very least an
>> apparent one).
>>
> 
> Why did you say this is a regression?

Did things work for you _regardless_ of guest memory size?

> Without these two patches, any assigned device with RMRR dependency 
> can't work at all since RMRR table never be created.

Whether a device works without the RMRR being mapped is an
unknown without knowing the specifics of the device - see USB
devices which don't normally need these regions post boot.

> But if we apply 
> these two patches, RMRR table can be created safely, right? Then the 
> assigned device can work based on them.

As long as the guest has little enough memory. As said before, if
the RMRR specifies regions below 1Mb, I don't think you'll be able
to create any guest with a respective device passed through.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, September 23, 2014 5:14 AM
> 
> >>> On 23.09.14 at 03:56, <tiejun.chen@intel.com> wrote:
> > On 2014/9/22 18:36, Jan Beulich wrote:
> >>>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
> >>> On 2014/9/22 16:53, Jan Beulich wrote:
> >>>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
> >>>>>>     >> It should suffice to give 3 Gb (or event slightly less) of
> memory to
> >>>>>>    >> the DomU (if your Dom0 can hopefully tolerate running with
> just 1Gb).
> >>>>>>    >
> >>>>>>    > Yes. So I can't produce that real case of conflict with those
> existing
> >>>>>>    > RMRR in my platform.
> >>>>>>
> >>>>>> When you pass 3Gb to the guest, its memory map should extend to
> >>>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
> >>>>>
> >>>>> Yes. So I set memory size as 2816M which also cover all RMRR ranges
> in
> >>>>> my platform.
> >>>>>
> >>>>>> you ought to be able to see the collision (or if you don't you ought to
> >>>>>> have ways to find out why they're not happening, as that would be a
> >>>>>> sign of something else being bogus).
> >>>>>>
> >>>>>
> >>>>> Then I can see that work as we expect:
> >>>>>
> >>>>> # xl cr hvm.cfg
> >>>>> Parsing config from hvm.cfg
> >>>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
> >>>>> Operation not permitted
> >>>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
> >>>>> libxl_device_pci_add failed: -3
> >>>>>
> >>>>> And
> >>>>>
> >>>>> # xl dmesg
> >>>>> ...
> >>>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
> >>>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
> >>>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
> >>>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
> >>>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1 failed
> (-1)
> >>>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
> >>>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0
> >>>>> ...
> >>>>
> >>>> So after all device assignment fails in that case, which is what I was
> >>>> expecting to happen. Which gets me back to the question: What's
> >>>> the value of the two patches for you if with them you can't pass
> >>>> through anymore the device you want passed through for the actual
> >>>> work you're doing?
> >>>
> >>> I don't understand what you mean again. This is true we already known
> >>> previously because this is just a part of the whole solution, right? So
> >>> I can't understand why we can't apply them now unless you're saying
> >>> they're wrong.
> >>
> >> You want these two patches applied despite having acknowledged
> >> that even for you they cause a regression (at the very least an
> >> apparent one).
> >>
> >
> > Why did you say this is a regression?
> 
> Did things work for you _regardless_ of guest memory size?
> 
> > Without these two patches, any assigned device with RMRR dependency
> > can't work at all since RMRR table never be created.
> 
> Whether a device works without the RMRR being mapped is an
> unknown without knowing the specifics of the device - see USB
> devices which don't normally need these regions post boot.
> 
> > But if we apply
> > these two patches, RMRR table can be created safely, right? Then the
> > assigned device can work based on them.
> 
> As long as the guest has little enough memory. As said before, if
> the RMRR specifies regions below 1Mb, I don't think you'll be able
> to create any guest with a respective device passed through.
> 

Hi, Jan,

I'm a bit lost what's the exact regression here. This patch definitely is not aimed
to solve all the problems, which is what Tiejun's another big series is doing. it's
a step forward to allow GPU pass-through for Windows VM, w/o regression in 
other areas (which are mostly broken today). However, seems your regression
is caused by patching other code:

----
So I gave this a try on the one box I have which exposes RMRRs
(since those are for USB devices I also used your patch to drop
the USB special casing as done in your later patch series, and I
further had to fiddle with vtd_ept_page_compatible() in order to
get page table sharing to actually work on that box [I'll send the
resulting patch later])
----

If that's the case, it's not the regression typically defined.

Thanks
Kevin

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.09.14 at 02:28, <kevin.tian@intel.com> wrote:
> I'm a bit lost what's the exact regression here. This patch definitely is 
> not aimed
> to solve all the problems, which is what Tiejun's another big series is 
> doing. it's
> a step forward to allow GPU pass-through for Windows VM, w/o regression in 
> other areas (which are mostly broken today). However, seems your regression
> is caused by patching other code:
> 
> ----
> So I gave this a try on the one box I have which exposes RMRRs
> (since those are for USB devices I also used your patch to drop
> the USB special casing as done in your later patch series, and I
> further had to fiddle with vtd_ept_page_compatible() in order to
> get page table sharing to actually work on that box [I'll send the
> resulting patch later])
> ----
> 
> If that's the case, it's not the regression typically defined.

Sure - I had to do this extra patching to have a way to simulate
the effect. I simply don't have another system where RMRRs
would be associated with other than USB devices.

The facts about the 2 patches here are:
- they don't generally help (namely not for large enough guests)
- they may yield devices unassignable that used to work fine

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Chen, Tiejun wrote on 2014-09-23:
> On 2014/9/22 18:36, Jan Beulich wrote:
>>>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/22 16:53, Jan Beulich wrote:
>>>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>>>>>> It should suffice to give 3 Gb (or event slightly less)
>>>>>> of memory
> to
>>>>>>>> the DomU (if your Dom0 can hopefully tolerate running with
>>>>>> just
> 1Gb).
>>>>>>> 
>>>>>>> Yes. So I can't produce that real case of conflict with those existing
>>>>>>> RMRR in my platform.
>>>>>> 
>>>>>> When you pass 3Gb to the guest, its memory map should extend to
>>>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>>>> 
>>>>> Yes. So I set memory size as 2816M which also cover all RMRR
>>>>> ranges in my platform.
>>>>> 
>>>>>> you ought to be able to see the collision (or if you don't you
>>>>>> ought to have ways to find out why they're not happening, as
>>>>>> that would be a sign of something else being bogus).
>>>>>> 
>>>>> 
>>>>> Then I can see that work as we expect:
>>>>> 
>>>>> # xl cr hvm.cfg
>>>>> Parsing config from hvm.cfg
>>>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>>>>> Operation not permitted
>>>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>>>>> libxl_device_pci_add failed: -3
>>>>> 
>>>>> And
>>>>> 
>>>>> # xl dmesg
>>>>> ...
>>>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>>>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>>>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>>>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>>>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1
>>>>> failed
>>>>> (-1)
>>>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>>>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0 ...
>>>> 
>>>> So after all device assignment fails in that case, which is what I
>>>> was expecting to happen. Which gets me back to the question:
>>>> What's the value of the two patches for you if with them you can't
>>>> pass through anymore the device you want passed through for the
>>>> actual work you're doing?
>>> 
>>> I don't understand what you mean again. This is true we already
>>> known previously because this is just a part of the whole solution, right?
>>> So I can't understand why we can't apply them now unless you're
>>> saying they're wrong.
>> 
>> You want these two patches applied despite having acknowledged that
>> even for you they cause a regression (at the very least an apparent
>> one).
>> 
> 
> Why did you say this is a regression?
> 
> Without these two patches, any assigned device with RMRR dependency
> can't work at all since RMRR table never be created. But if we apply
> these two patches, RMRR table can be created safely, right? Then the
> assigned device can work based on them.

Since we still have arguments on the whole RMRR patch set, so I list the existing RMRR problem to make sure all of us on the same page. And then we can have a discussion on how to solve them perfectly. I also give my suggestion but it may not be the best solution. Also, if there is any missing problem, please tell me.

1. RMRR region isn't reserved in guest e820 table and guest is able to touch it.

Possible solution: set RMRR region as reserved in guest e820 table and create identity map in EPT and VT-d page table.

2. RMRR region may conflict with MMIO.

Possible solution: Refuse to assign device or reallocate the MMIO.

3. RMRR region isn't checked when updating EPT table and VT-d table.

Possible solution: Return error when trying to update EPT and VT-d table if the gfn is inside RMRR region.

4. RMRR region isn't setup in page table in sharing EPT case.

Tiejun's two patches are able to fix this issue.

5. rmrr_identity_mapping() blindly overwrites what may already be in the page tables(EPT table in share case and VT-table in non-share case).

Possible solution: Actually, it should be same to issue 1. If RMRR region is reserved in guest e820 table, guest should not touch it. Otherwise, any unpredictable behavior to guest is acceptable.

6. Live migration with RMRR region and hotplug.

Possible solution: Do the checking in tool stack: If the device which requires RMRR but the corresponding region is not reserved in guest e820 or have overlap with MMIO, then refuse to do the hotplug.

One question, should we fix all of them at once or can we fix them one by one based on severity? For example, the issue 6 happens rarely and I think we can leave it after Xen 4.5.

Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/24 16:23, Zhang, Yang Z wrote:
> Chen, Tiejun wrote on 2014-09-23:
>> On 2014/9/22 18:36, Jan Beulich wrote:
>>>>>> On 22.09.14 at 11:05, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/22 16:53, Jan Beulich wrote:
>>>>>>>> On 22.09.14 at 07:46, <tiejun.chen@intel.com> wrote:
>>>>>>>>> It should suffice to give 3 Gb (or event slightly less)
>>>>>>> of memory
>> to
>>>>>>>>> the DomU (if your Dom0 can hopefully tolerate running with
>>>>>>> just
>> 1Gb).
>>>>>>>>
>>>>>>>> Yes. So I can't produce that real case of conflict with those existing
>>>>>>>> RMRR in my platform.
>>>>>>>
>>>>>>> When you pass 3Gb to the guest, its memory map should extend to
>>>>>>> about 0xC0000000, well beyond the range the RMRRs reference. So
>>>>>>
>>>>>> Yes. So I set memory size as 2816M which also cover all RMRR
>>>>>> ranges in my platform.
>>>>>>
>>>>>>> you ought to be able to see the collision (or if you don't you
>>>>>>> ought to have ways to find out why they're not happening, as
>>>>>>> that would be a sign of something else being bogus).
>>>>>>>
>>>>>>
>>>>>> Then I can see that work as we expect:
>>>>>>
>>>>>> # xl cr hvm.cfg
>>>>>> Parsing config from hvm.cfg
>>>>>> libxl: error: libxl_pci.c:949:do_pci_add: xc_assign_device failed:
>>>>>> Operation not permitted
>>>>>> libxl: error: libxl_create.c:1329:domcreate_attach_pci:
>>>>>> libxl_device_pci_add failed: -3
>>>>>>
>>>>>> And
>>>>>>
>>>>>> # xl dmesg
>>>>>> ...
>>>>>> (XEN) [VT-D]iommu.c:1589: d0:PCI: unmap 0000:00:02.0
>>>>>> (XEN) [VT-D]iommu.c:1452: d1:PCI: map 0000:00:02.0
>>>>>> (XEN) Cannot identity map d1:ad000, already mapped to 115d51.
>>>>>> (XEN) [VT-D]iommu.c:2296: IOMMU: mapping reserved region failed
>>>>>> (XEN) XEN_DOMCTL_assign_device: assign 0000:00:02.0 to dom1
>>>>>> failed
>>>>>> (-1)
>>>>>> (XEN) [VT-D]iommu.c:1589: d1:PCI: unmap 0000:00:02.0
>>>>>> (XEN) [VT-D]iommu.c:1452: d0:PCI: map 0000:00:02.0 ...
>>>>>
>>>>> So after all device assignment fails in that case, which is what I
>>>>> was expecting to happen. Which gets me back to the question:
>>>>> What's the value of the two patches for you if with them you can't
>>>>> pass through anymore the device you want passed through for the
>>>>> actual work you're doing?
>>>>
>>>> I don't understand what you mean again. This is true we already
>>>> known previously because this is just a part of the whole solution, right?
>>>> So I can't understand why we can't apply them now unless you're
>>>> saying they're wrong.
>>>
>>> You want these two patches applied despite having acknowledged that
>>> even for you they cause a regression (at the very least an apparent
>>> one).
>>>
>>
>> Why did you say this is a regression?
>>
>> Without these two patches, any assigned device with RMRR dependency
>> can't work at all since RMRR table never be created. But if we apply
>> these two patches, RMRR table can be created safely, right? Then the
>> assigned device can work based on them.
>
> Since we still have arguments on the whole RMRR patch set, so I list the existing RMRR problem to make sure all of us on the same page. And then we can have a discussion on how to solve them perfectly. I also give my suggestion but it may not be the best solution. Also, if there is any missing problem, please tell me.
>

Thanks for your summary.

> 1. RMRR region isn't reserved in guest e820 table and guest is able to touch it.
>
> Possible solution: set RMRR region as reserved in guest e820 table and create identity map in EPT and VT-d page table.
>
> 2. RMRR region may conflict with MMIO.
>
> Possible solution: Refuse to assign device or reallocate the MMIO.
>
> 3. RMRR region isn't checked when updating EPT table and VT-d table.
>
> Possible solution: Return error when trying to update EPT and VT-d table if the gfn is inside RMRR region.
>
> 4. RMRR region isn't setup in page table in sharing EPT case.
>
> Tiejun's two patches are able to fix this issue.

I think these four point should be covered with current patches 
including another series of patches. Certainly I need to refine them 
eventually but they should be addressing these concerns.

>
> 5. rmrr_identity_mapping() blindly overwrites what may already be in the page tables(EPT table in share case and VT-table in non-share case).
>
> Possible solution: Actually, it should be same to issue 1. If RMRR region is reserved in guest e820 table, guest should not touch it. Otherwise, any unpredictable behavior to guest is acceptable.
>
> 6. Live migration with RMRR region and hotplug.
>
> Possible solution: Do the checking in tool stack: If the device which requires RMRR but the corresponding region is not reserved in guest e820 or have overlap with MMIO, then refuse to do the hotplug.
>

In tools stack, how can we get ultimate e820 table built by hvmloader?

7. One RMRR can be mapped for multiple devices in multiple domains

So we may need to do something to stop this potential damage between 
domains.

Thanks
Tiejun

> One question, should we fix all of them at once or can we fix them one by one based on severity? For example, the issue 6 happens rarely and I think we can leave it after Xen 4.5.
>
> Best regards,
> Yang
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
> Since we still have arguments on the whole RMRR patch set, so I list the 
> existing RMRR problem to make sure all of us on the same page. And then we 
> can have a discussion on how to solve them perfectly. I also give my 
> suggestion but it may not be the best solution. Also, if there is any missing 
> problem, please tell me.

Thanks for doing this; it looks complete to me (except for lacking an
explicit statement on the consequences of some of the changes).

> 1. RMRR region isn't reserved in guest e820 table and guest is able to touch 
> it.
> 
> Possible solution: set RMRR region as reserved in guest e820 table and 
> create identity map in EPT and VT-d page table.

And relocate guest RAM accordingly.

> 2. RMRR region may conflict with MMIO.
> 
> Possible solution: Refuse to assign device or reallocate the MMIO.

"Reallocate" is perhaps the wrong term here: Since the allocation of
MMIO resources happens in hvmloader, it's really that this allocation
needs to be done with the RMRRs in mind from the beginning.

> 3. RMRR region isn't checked when updating EPT table and VT-d table.
> 
> Possible solution: Return error when trying to update EPT and VT-d table if 
> the gfn is inside RMRR region.
> 
> 4. RMRR region isn't setup in page table in sharing EPT case.
> 
> Tiejun's two patches are able to fix this issue.
> 
> 5. rmrr_identity_mapping() blindly overwrites what may already be in the 
> page tables(EPT table in share case and VT-table in non-share case).
> 
> Possible solution: Actually, it should be same to issue 1. If RMRR region is 
> reserved in guest e820 table, guest should not touch it. Otherwise, any 
> unpredictable behavior to guest is acceptable.

This leaves aside that not all updates are necessarily caused by
guest activity (i.e. qemu and the tool stack could affect such too).

> 6. Live migration with RMRR region and hotplug.
> 
> Possible solution: Do the checking in tool stack: If the device which 
> requires RMRR but the corresponding region is not reserved in guest e820 or 
> have overlap with MMIO, then refuse to do the hotplug.
> 
> One question, should we fix all of them at once or can we fix them one by 
> one based on severity? For example, the issue 6 happens rarely and I think we 
> can leave it after Xen 4.5.

I really only see two routes for 4.5: Either fix everything properly
or disallow assignment of devices associated with RMRRs altogether
(with log messages clearly pointing to an override command line
option).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
> In tools stack, how can we get ultimate e820 table built by hvmloader?

What is this needed for?

> 7. One RMRR can be mapped for multiple devices in multiple domains
> 
> So we may need to do something to stop this potential damage between 
> domains.

Good point. We should enforce these to be group assigned (along
the lines of what is already done with XEN_DOMCTL_get_device_group).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/24 16:47, Jan Beulich wrote:
>>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
>> In tools stack, how can we get ultimate e820 table built by hvmloader?
>
> What is this needed for?

I mean in case of a hotplug after the migration, we should check any 
overlap with current existing RAM/MMIO, right? So we need to know 
current existing guest RAM/MMIO layout since this ultimate e820 table is 
just built by hvmloader, not in xen internal.

Thanks
Tiejun

>
>> 7. One RMRR can be mapped for multiple devices in multiple domains
>>
>> So we may need to do something to stop this potential damage between
>> domains.
>
> Good point. We should enforce these to be group assigned (along
> the lines of what is already done with XEN_DOMCTL_get_device_group).
>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 24.09.14 at 10:53, <tiejun.chen@intel.com> wrote:
> On 2014/9/24 16:47, Jan Beulich wrote:
>>>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
>>> In tools stack, how can we get ultimate e820 table built by hvmloader?
>>
>> What is this needed for?
> 
> I mean in case of a hotplug after the migration, we should check any 
> overlap with current existing RAM/MMIO, right? So we need to know 
> current existing guest RAM/MMIO layout since this ultimate e820 table is 
> just built by hvmloader, not in xen internal.

Actually I think we'd be better off not complicating the tool stack for
this - with proper checking for collisions in the P2M updates, the
assignment ought to fail without the tool stack doing anything.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-24:
> >>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
> > Since we still have arguments on the whole RMRR patch set, so I list the
> > existing RMRR problem to make sure all of us on the same page. And then we
> > can have a discussion on how to solve them perfectly. I also give my
> > suggestion but it may not be the best solution. Also, if there is any missing
> > problem, please tell me.
> 
> Thanks for doing this; it looks complete to me (except for lacking an
> explicit statement on the consequences of some of the changes).
> 
> > 1. RMRR region isn't reserved in guest e820 table and guest is able to touch
> > it.
> >
> > Possible solution: set RMRR region as reserved in guest e820 table and
> > create identity map in EPT and VT-d page table.
> 
> And relocate guest RAM accordingly.

ok. Let's look into more detail:
1. Whether there has device assigned or not, the RMRR mapping must be created when building e820 table if the VT-d is enabled.
2. Despite of share or non-share case, the RMRR region must be identity map in EPT and VT-d page table.
3. Tiejun's patch uses hypercall to get the RMRR info, is it ok? Or should we get it from xenstore, and then both tools and hvmloader can access it?

> 
> > 2. RMRR region may conflict with MMIO.
> >
> > Possible solution: Refuse to assign device or reallocate the MMIO.
> 
> "Reallocate" is perhaps the wrong term here: Since the allocation of
> MMIO resources happens in hvmloader, it's really that this allocation
> needs to be done with the RMRRs in mind from the beginning.

Yes, you are right.

> 
> > 3. RMRR region isn't checked when updating EPT table and VT-d table.
> >
> > Possible solution: Return error when trying to update EPT and VT-d table if
> > the gfn is inside RMRR region.

So just do a simple check in EPT table and VT-d table updating is ok?

> >
> > 4. RMRR region isn't setup in page table in sharing EPT case.
> >
> > Tiejun's two patches are able to fix this issue.
> >
> > 5. rmrr_identity_mapping() blindly overwrites what may already be in the
> > page tables(EPT table in share case and VT-table in non-share case).
> >
> > Possible solution: Actually, it should be same to issue 1. If RMRR region is
> > reserved in guest e820 table, guest should not touch it. Otherwise, any
> > unpredictable behavior to guest is acceptable.
> 
> This leaves aside that not all updates are necessarily caused by
> guest activity (i.e. qemu and the tool stack could affect such too).

Yes, but qemu and tool stack should not touch the memory marked as reserved in e820. They should only touch the memory that they know.

> 
> > 6. Live migration with RMRR region and hotplug.
> >
> > Possible solution: Do the checking in tool stack: If the device which
> > requires RMRR but the corresponding region is not reserved in guest e820 or
> > have overlap with MMIO, then refuse to do the hotplug.
> >
> > One question, should we fix all of them at once or can we fix them one by
> > one based on severity? For example, the issue 6 happens rarely and I think we
> > can leave it after Xen 4.5.
> 
> I really only see two routes for 4.5: Either fix everything properly
> or disallow assignment of devices associated with RMRRs altogether
> (with log messages clearly pointing to an override command line
> option).

Ok. It makes sense. Do you think it possible to cacth the Xen 4.5 if everything is fixed properly?

> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-24:
>>>> On 24.09.14 at 10:53, <tiejun.chen@intel.com> wrote:
>> On 2014/9/24 16:47, Jan Beulich wrote:
>>>>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
>>>> In tools stack, how can we get ultimate e820 table built by hvmloader?
>>> 
>>> What is this needed for?
>> 
>> I mean in case of a hotplug after the migration, we should check any
>> overlap with current existing RAM/MMIO, right? So we need to know
>> current existing guest RAM/MMIO layout since this ultimate e820
>> table is just built by hvmloader, not in xen internal.
> 
> Actually I think we'd be better off not complicating the tool stack
> for this - with proper checking for collisions in the P2M updates, the
> assignment ought to fail without the tool stack doing anything.

How do the checking in P2M updates? Only hvmloader knows whether the RMRR region is reserved. If we want do the check in hypervisor, we need to report those info to hypervisor. 

> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.09.14 at 03:53, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-24:
>> >>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
>> > 1. RMRR region isn't reserved in guest e820 table and guest is able to touch
>> > it.
>> >
>> > Possible solution: set RMRR region as reserved in guest e820 table and
>> > create identity map in EPT and VT-d page table.
>> 
>> And relocate guest RAM accordingly.
> 
> ok. Let's look into more detail:
> 1. Whether there has device assigned or not, the RMRR mapping must be 
> created when building e820 table if the VT-d is enabled.
> 2. Despite of share or non-share case, the RMRR region must be identity map 
> in EPT and VT-d page table.
> 3. Tiejun's patch uses hypercall to get the RMRR info, is it ok? Or should 
> we get it from xenstore, and then both tools and hvmloader can access it?

The hypercall is clearly the route to go imo - xenstore would be a
pretty crude mechanism for conveying that information (and using
the hypercall approach precludes neither the tools nor hvmloader
from obtaining that data).

>> > 3. RMRR region isn't checked when updating EPT table and VT-d table.
>> >
>> > Possible solution: Return error when trying to update EPT and VT-d table if
>> > the gfn is inside RMRR region.
> 
> So just do a simple check in EPT table and VT-d table updating is ok?

I think so - anything more sophisticated (like checking in the tools)
will not give any better results (except for a more explicit error
message maybe, but this can certainly be had equally well by using a
very specific error code should the hypervisor side check fail).

>> > 6. Live migration with RMRR region and hotplug.
>> >
>> > Possible solution: Do the checking in tool stack: If the device which
>> > requires RMRR but the corresponding region is not reserved in guest e820 or
>> > have overlap with MMIO, then refuse to do the hotplug.
>> >
>> > One question, should we fix all of them at once or can we fix them one by
>> > one based on severity? For example, the issue 6 happens rarely and I think we
>> > can leave it after Xen 4.5.
>> 
>> I really only see two routes for 4.5: Either fix everything properly
>> or disallow assignment of devices associated with RMRRs altogether
>> (with log messages clearly pointing to an override command line
>> option).
> 
> Ok. It makes sense. Do you think it possible to cacth the Xen 4.5 if 
> everything is fixed properly?

Considering it's a bug that gets fixed, I would think so. But as for
anything more involved, Konrad as the release manager will have
the final say.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-24:
>>>>> On 24.09.14 at 10:53, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/24 16:47, Jan Beulich wrote:
>>>>>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
>>>>> In tools stack, how can we get ultimate e820 table built by hvmloader?
>>>> 
>>>> What is this needed for?
>>> 
>>> I mean in case of a hotplug after the migration, we should check any
>>> overlap with current existing RAM/MMIO, right? So we need to know
>>> current existing guest RAM/MMIO layout since this ultimate e820
>>> table is just built by hvmloader, not in xen internal.
>> 
>> Actually I think we'd be better off not complicating the tool stack
>> for this - with proper checking for collisions in the P2M updates, the
>> assignment ought to fail without the tool stack doing anything.
> 
> How do the checking in P2M updates? Only hvmloader knows whether the RMRR 
> region is reserved. If we want do the check in hypervisor, we need to report 
> those info to hypervisor. 

First of all the hypervisor has the information - it is where the
information comes from that tools and hvmloader consume. And
then the check will need to be a collision check: If while
establishing an identity mapping another mapping is found to be
already in place, the request will need to be failed. And similarly
if a "normal" mapping request finds a 1:1 mapping already in place,
this ought to result in failure too. Of course a prerequisite to this
is that error get properly bubbled up through all layers.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Konrad Rzeszutek Wilk]

On Thu, Sep 25, 2014 at 09:08:14AM +0100, Jan Beulich wrote:
> >>> On 25.09.14 at 03:53, <yang.z.zhang@intel.com> wrote:
> > Jan Beulich wrote on 2014-09-24:
> >> >>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
> >> > 1. RMRR region isn't reserved in guest e820 table and guest is able to touch
> >> > it.
> >> >
> >> > Possible solution: set RMRR region as reserved in guest e820 table and
> >> > create identity map in EPT and VT-d page table.
> >> 
> >> And relocate guest RAM accordingly.
> > 
> > ok. Let's look into more detail:
> > 1. Whether there has device assigned or not, the RMRR mapping must be 
> > created when building e820 table if the VT-d is enabled.
> > 2. Despite of share or non-share case, the RMRR region must be identity map 
> > in EPT and VT-d page table.
> > 3. Tiejun's patch uses hypercall to get the RMRR info, is it ok? Or should 
> > we get it from xenstore, and then both tools and hvmloader can access it?
> 
> The hypercall is clearly the route to go imo - xenstore would be a
> pretty crude mechanism for conveying that information (and using
> the hypercall approach precludes neither the tools nor hvmloader
> from obtaining that data).
> 
> >> > 3. RMRR region isn't checked when updating EPT table and VT-d table.
> >> >
> >> > Possible solution: Return error when trying to update EPT and VT-d table if
> >> > the gfn is inside RMRR region.
> > 
> > So just do a simple check in EPT table and VT-d table updating is ok?
> 
> I think so - anything more sophisticated (like checking in the tools)
> will not give any better results (except for a more explicit error
> message maybe, but this can certainly be had equally well by using a
> very specific error code should the hypervisor side check fail).
> 
> >> > 6. Live migration with RMRR region and hotplug.
> >> >
> >> > Possible solution: Do the checking in tool stack: If the device which
> >> > requires RMRR but the corresponding region is not reserved in guest e820 or
> >> > have overlap with MMIO, then refuse to do the hotplug.
> >> >
> >> > One question, should we fix all of them at once or can we fix them one by
> >> > one based on severity? For example, the issue 6 happens rarely and I think we
> >> > can leave it after Xen 4.5.
> >> 
> >> I really only see two routes for 4.5: Either fix everything properly
> >> or disallow assignment of devices associated with RMRRs altogether
> >> (with log messages clearly pointing to an override command line
> >> option).
> > 
> > Ok. It makes sense. Do you think it possible to cacth the Xen 4.5 if 
> > everything is fixed properly?
> 
> Considering it's a bug that gets fixed, I would think so. But as for
> anything more involved, Konrad as the release manager will have
> the final say.

What are the non-bugs (features) that we speak of? Are the the ones that had
been posted (and then one written by Jan and then reworked)?

Please do be aware that the feature freeze window time has just elapsed (yesterday)
so anything to be considered a feature should have been posted before or on that date.

Please keep in mind that bug-fixes can be posted and checked in _after_ the
feature freeze. But as the RCs numbers increases the likehood of bugs being
checked in goes down (to reduce the risk of further regressions instroduced
by bug-fixes).
> 
> Jan
> 

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 25.09.14 at 16:12, <konrad.wilk@oracle.com> wrote:
> On Thu, Sep 25, 2014 at 09:08:14AM +0100, Jan Beulich wrote:
>> Considering it's a bug that gets fixed, I would think so. But as for
>> anything more involved, Konrad as the release manager will have
>> the final say.
> 
> What are the non-bugs (features) that we speak of?

There are no non-bug changes here. It's just that the issue has
always been there, so is also not a regression.

> Are the the ones that had
> been posted (and then one written by Jan and then reworked)?

Yes, and which need further reworking.

> Please do be aware that the feature freeze window time has just elapsed 
> (yesterday)
> so anything to be considered a feature should have been posted before or on 
> that date.
> 
> Please keep in mind that bug-fixes can be posted and checked in _after_ the
> feature freeze. But as the RCs numbers increases the likehood of bugs being
> checked in goes down (to reduce the risk of further regressions instroduced
> by bug-fixes).

That's the main reason why I pointed at you as the release manager
- together with this not being a new bug our willingness to add a fix
for this will presumably decrease as time goes on.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-25:
>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-09-24:
>>>>>> On 24.09.14 at 10:53, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/24 16:47, Jan Beulich wrote:
>>>>>>>> On 24.09.14 at 10:35, <tiejun.chen@intel.com> wrote:
>>>>>> In tools stack, how can we get ultimate e820 table built by hvmloader?
>>>>> 
>>>>> What is this needed for?
>>>> 
>>>> I mean in case of a hotplug after the migration, we should check
>>>> any overlap with current existing RAM/MMIO, right? So we need to
>>>> know current existing guest RAM/MMIO layout since this ultimate
>>>> e820 table is just built by hvmloader, not in xen internal.
>>> 
>>> Actually I think we'd be better off not complicating the tool stack
>>> for this - with proper checking for collisions in the P2M updates,
>>> the assignment ought to fail without the tool stack doing anything.
>> 
>> How do the checking in P2M updates? Only hvmloader knows whether the
>> RMRR region is reserved. If we want do the check in hypervisor, we
>> need to report those info to hypervisor.
> 
> First of all the hypervisor has the information - it is where the
> information comes from that tools and hvmloader consume. And then the
> check will need to be a collision check: If while establishing an
> identity mapping another mapping is found to be already in place, the
> request will need to be failed. And similarly if a "normal" mapping
> request finds a 1:1 mapping already in place, this ought to result in
> failure too. Of course a prerequisite to this is that error get properly bubbled up through all layers.

If there is another mapping already there and it is different from the one we are establishing, we should return failure. But if the existing mapping is same to the one we are establishing, how we can know whether it is a 'normal memory 1:1' mapping or it is created by previous operation of RMRR creating(e.g. there already a device with RMRR assigned to this guest). What I am thinking is that we need a flag to know whether the mapping is RMRR mapping or regular memory mapping.

> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 26.09.14 at 03:24, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-25:
>>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
>>> How do the checking in P2M updates? Only hvmloader knows whether the
>>> RMRR region is reserved. If we want do the check in hypervisor, we
>>> need to report those info to hypervisor.
>> 
>> First of all the hypervisor has the information - it is where the
>> information comes from that tools and hvmloader consume. And then the
>> check will need to be a collision check: If while establishing an
>> identity mapping another mapping is found to be already in place, the
>> request will need to be failed. And similarly if a "normal" mapping
>> request finds a 1:1 mapping already in place, this ought to result in
>> failure too. Of course a prerequisite to this is that error get properly 
> bubbled up through all layers.
> 
> If there is another mapping already there and it is different from the one 
> we are establishing, we should return failure. But if the existing mapping is 
> same to the one we are establishing, how we can know whether it is a 'normal 
> memory 1:1' mapping or it is created by previous operation of RMRR 
> creating(e.g. there already a device with RMRR assigned to this guest). What 
> I am thinking is that we need a flag to know whether the mapping is RMRR 
> mapping or regular memory mapping.

If the new and old mappings are the same, nothing needs to be done
at all (as is already the case in one direction in the patches we have
seen). And yes, for the case when there is an occasional 1:1 mapping
we of course will need some way of identifying intentional ones.

Jan

Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Konrad Rzeszutek Wilk]

On Thu, Sep 25, 2014 at 04:14:28PM +0100, Jan Beulich wrote:
> >>> On 25.09.14 at 16:12, <konrad.wilk@oracle.com> wrote:
> > On Thu, Sep 25, 2014 at 09:08:14AM +0100, Jan Beulich wrote:
> >> Considering it's a bug that gets fixed, I would think so. But as for
> >> anything more involved, Konrad as the release manager will have
> >> the final say.

Hey Paul,

I CC-ed you on this as I have a question below.

> > 
> > What are the non-bugs (features) that we speak of?
> 
> There are no non-bug changes here. It's just that the issue has
> always been there, so is also not a regression.

OK, patches do not introduce regressions - but they fix bugs
with PCIe passthrough devices (especially when passing in USB and GPUs?).
> 
> > Are the the ones that had
> > been posted (and then one written by Jan and then reworked)?
> 
> Yes, and which need further reworking.
> 
> > Please do be aware that the feature freeze window time has just elapsed 
> > (yesterday)
> > so anything to be considered a feature should have been posted before or on 
> > that date.
> > 
> > Please keep in mind that bug-fixes can be posted and checked in _after_ the
> > feature freeze. But as the RCs numbers increases the likehood of bugs being
> > checked in goes down (to reduce the risk of further regressions instroduced
> > by bug-fixes).
> 
> That's the main reason why I pointed at you as the release manager
> - together with this not being a new bug our willingness to add a fix
> for this will presumably decrease as time goes on.

The initial justification (this is my understanding - please correct
me if I am incorrect) for these patches is that it will expand the
use-case of PCI passthrough for the use case of graphics cards.
Which is fantastic because we really need to make sure that all works.

When I saw "xen: add Intel IGD passthrough support" patches I jumped on
them to help them along to get in QEMU. They now seem to have hit a roadblock
were they are wating on Chen to redesign them - such that they are only
using the registers from the GPU card - and not the bridge one. That of course
depends on the GPU drivers (Windows in particular, Linux ones I posted an
RFC ones that could be used) being updated to do that. .. And I have not
seen any update on that.

Of course even if they do go out (the Windows drivers), and then the QEMU
work can progress - it won't be in QEMU 2.0 (which is in Xen 4.5), but rather
in QEMU 2.x.  I haven't seen any emails from Chen Tiejun to Stefano about
backporting those in qemu-xen in our tree.

But oddly enough those patches are in the qemu-traditional!

My understanding is that the multiple ioreq-server patches are also an
requirement for this. But I don't know if they work with qemu-traditional
(I think they should work fine with that, but not sure - CC-ing Paul
to find out).

Then there is the complication of that folks in China are celebrating
from Oct 1st -> Oct 7th - so that means Chien (I presume she works there,
but I could be very well mistaken - please correct me) would have to
answer/fix all the patches in the next two working days.

With the first RC going out on October 15th, that means a total of four
working days to get all of this done, reviewed, fixed, and tested.

And working under stress to get this done can mean introducing
subtle bugs (this is based on my personal experience, so your
experience might be different).

Anyhow, I really need to know if - are all of the code pieces 
(minus the patches we are discussing) for this full GPU 
functionality in the Xen codebase and will it work with Xen 4.5
or will it require extra additional patches? And also, can
all of those patches be tested/reviewed/posted by Oct 13th?

As in, is this patchset the last piece of the puzzle?

And by tested I mean with the passthrough and without, and
with various size guests doing migrations (without devices)
multiple times (say ten) - and 32 or 64 bit. The QA matrix
means at least 8 variations (32/64 - with PCIe passthrough,
and without, 2GB or 8GB or 12GB) by my reckoning.

Sorry for being so aggressive about the testing part but I am
very adverse to regressions - as they have bit me in the past
and left me with an strong aversion to them.

Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 26.09.14 at 15:55, <konrad.wilk@oracle.com> wrote:
> On Thu, Sep 25, 2014 at 04:14:28PM +0100, Jan Beulich wrote:
>> >>> On 25.09.14 at 16:12, <konrad.wilk@oracle.com> wrote:
>> > What are the non-bugs (features) that we speak of?
>> 
>> There are no non-bug changes here. It's just that the issue has
>> always been there, so is also not a regression.
> 
> OK, patches do not introduce regressions - but they fix bugs
> with PCIe passthrough devices (especially when passing in USB and GPUs?).

Whether the full eventual patch set introduce regressions I don't
know - I would hope not, but you never know. The two patches
discussed here do introduce at least what one could perceive as
regression: A device previously passing through fine to a guest
may all of the sudden no longer do so.

But what I was trying to say above wasn't anything related to
whether the patches introduce regressions, but that the patches
don't _fix_ any regression - the brokenness was always there.

All of the questions you raised further down are likely misaddressed
at me - they would need answering by the respective patch
authors.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/25 16:08, Jan Beulich wrote:
>>>> On 25.09.14 at 03:53, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-09-24:
>>>>>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
>>>> 1. RMRR region isn't reserved in guest e820 table and guest is able to touch
>>>> it.
>>>>
>>>> Possible solution: set RMRR region as reserved in guest e820 table and
>>>> create identity map in EPT and VT-d page table.
>>>
>>> And relocate guest RAM accordingly.
>>
>> ok. Let's look into more detail:
>> 1. Whether there has device assigned or not, the RMRR mapping must be
>> created when building e820 table if the VT-d is enabled.
>> 2. Despite of share or non-share case, the RMRR region must be identity map
>> in EPT and VT-d page table.
>> 3. Tiejun's patch uses hypercall to get the RMRR info, is it ok? Or should
>> we get it from xenstore, and then both tools and hvmloader can access it?
>
> The hypercall is clearly the route to go imo - xenstore would be a

Yes.

And based on recent discussion looks nothing to block covering these all 
7 problems with current hypercall implementation, right? I means we 
already took more time to get current hypercall codes, and looks its 
clearly good enough, right? So maybe we just can go through fixing these 
problems we were discussing.

Thanks
Tiejun

> pretty crude mechanism for conveying that information (and using
> the hypercall approach precludes neither the tools nor hvmloader
> from obtaining that data).
>
>>>> 3. RMRR region isn't checked when updating EPT table and VT-d table.
>>>>
>>>> Possible solution: Return error when trying to update EPT and VT-d table if
>>>> the gfn is inside RMRR region.
>>
>> So just do a simple check in EPT table and VT-d table updating is ok?
>
> I think so - anything more sophisticated (like checking in the tools)
> will not give any better results (except for a more explicit error
> message maybe, but this can certainly be had equally well by using a
> very specific error code should the hypervisor side check fail).
>
>>>> 6. Live migration with RMRR region and hotplug.
>>>>
>>>> Possible solution: Do the checking in tool stack: If the device which
>>>> requires RMRR but the corresponding region is not reserved in guest e820 or
>>>> have overlap with MMIO, then refuse to do the hotplug.
>>>>
>>>> One question, should we fix all of them at once or can we fix them one by
>>>> one based on severity? For example, the issue 6 happens rarely and I think we
>>>> can leave it after Xen 4.5.
>>>
>>> I really only see two routes for 4.5: Either fix everything properly
>>> or disallow assignment of devices associated with RMRRs altogether
>>> (with log messages clearly pointing to an override command line
>>> option).
>>
>> Ok. It makes sense. Do you think it possible to cacth the Xen 4.5 if
>> everything is fixed properly?
>
> Considering it's a bug that gets fixed, I would think so. But as for
> anything more involved, Konrad as the release manager will have
> the final say.
>
> Jan
>
>
>

Re: Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Konrad Rzeszutek Wilk wrote on 2014-09-26:
> On Thu, Sep 25, 2014 at 04:14:28PM +0100, Jan Beulich wrote:
>>>>> On 25.09.14 at 16:12, <konrad.wilk@oracle.com> wrote:
>>> On Thu, Sep 25, 2014 at 09:08:14AM +0100, Jan Beulich wrote:
>>>> Considering it's a bug that gets fixed, I would think so. But as for
>>>> anything more involved, Konrad as the release manager will have
>>>> the final say.
> 
> Hey Paul,
> 
> I CC-ed you on this as I have a question below.
> 
>>> 
>>> What are the non-bugs (features) that we speak of?
>> 
>> There are no non-bug changes here. It's just that the issue has
>> always been there, so is also not a regression.
> 
> OK, patches do not introduce regressions - but they fix bugs
> with PCIe passthrough devices (especially when passing in USB and GPUs?).
>> 
>>> Are the the ones that had
>>> been posted (and then one written by Jan and then reworked)?
>> 
>> Yes, and which need further reworking.
>> 
>>> Please do be aware that the feature freeze window time has just
>>> elapsed (yesterday) so anything to be considered a feature should have
>>> been posted before or on that date.
>>> 
>>> Please keep in mind that bug-fixes can be posted and checked in
>>> _after_ the feature freeze. But as the RCs numbers increases the
>>> likehood of bugs being checked in goes down (to reduce the risk of
>>> further regressions instroduced by bug-fixes).
>> 
>> That's the main reason why I pointed at you as the release manager
>> - together with this not being a new bug our willingness to add a fix
>> for this will presumably decrease as time goes on.
> 
> The initial justification (this is my understanding - please correct
> me if I am incorrect) for these patches is that it will expand the
> use-case of PCI passthrough for the use case of graphics cards.
> Which is fantastic because we really need to make sure that all works.
> 
> When I saw "xen: add Intel IGD passthrough support" patches I jumped on
> them to help them along to get in QEMU. They now seem to have hit a
> roadblock were they are wating on Chen to redesign them - such that they
> are only using the registers from the GPU card - and not the bridge one.
> That of course depends on the GPU drivers (Windows in particular, Linux
> ones I posted an RFC ones that could be used) being updated to do that.
> .. And I have not seen any update on that.
> 
> Of course even if they do go out (the Windows drivers), and then the QEMU
> work can progress - it won't be in QEMU 2.0 (which is in Xen 4.5), but rather
> in QEMU 2.x.  I haven't seen any emails from Chen Tiejun to Stefano about
> backporting those in qemu-xen in our tree.

This should be another story. We are not expecting to push the IGD passthrough patch to Qemu to catch the Xen 4.5. Besides, as you mentioned, they want us to redesign the whole them with modification from driver side. It really not an easy task and will require more time to do it.

> 
> But oddly enough those patches are in the qemu-traditional!

Yes, but as you known, qemu-traditional is only used by Xen and we don't need to consider other usage mode.

> 
> My understanding is that the multiple ioreq-server patches are also an
> requirement for this. But I don't know if they work with qemu-traditional
> (I think they should work fine with that, but not sure - CC-ing Paul
> to find out).
> 
> Then there is the complication of that folks in China are celebrating
> from Oct 1st -> Oct 7th - so that means Chien (I presume she works there,

s/she/he :)

> but I could be very well mistaken - please correct me) would have to
> answer/fix all the patches in the next two working days.
> 
> With the first RC going out on October 15th, that means a total of four
> working days to get all of this done, reviewed, fixed, and tested.
> 
> And working under stress to get this done can mean introducing
> subtle bugs (this is based on my personal experience, so your
> experience might be different).
> 
> Anyhow, I really need to know if - are all of the code pieces
> (minus the patches we are discussing) for this full GPU
> functionality in the Xen codebase and will it work with Xen 4.5
> or will it require extra additional patches? And also, can

Qemu-traditional + Xen 4.5 + RMRR patchset will work.

> all of those patches be tested/reviewed/posted by Oct 13th?
> 
> As in, is this patchset the last piece of the puzzle?
> 
> And by tested I mean with the passthrough and without, and
> with various size guests doing migrations (without devices)
> multiple times (say ten) - and 32 or 64 bit. The QA matrix
> means at least 8 variations (32/64 - with PCIe passthrough,
> and without, 2GB or 8GB or 12GB) by my reckoning.

I am not sure whether we can catch Oct 13th. Does this means those patch will not be accepted by you if we cannot catch Oct 13th?

> 
> Sorry for being so aggressive about the testing part but I am
> very adverse to regressions - as they have bit me in the past
> and left me with an strong aversion to them.


Best regards,
Yang

Re: Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Pasi Kärkkäinen]

On Mon, Sep 29, 2014 at 01:26:52AM +0000, Zhang, Yang Z wrote:
> 
> > but I could be very well mistaken - please correct me) would have to
> > answer/fix all the patches in the next two working days.
> > 
> > With the first RC going out on October 15th, that means a total of four
> > working days to get all of this done, reviewed, fixed, and tested.
> > 
> > And working under stress to get this done can mean introducing
> > subtle bugs (this is based on my personal experience, so your
> > experience might be different).
> > 
> > Anyhow, I really need to know if - are all of the code pieces
> > (minus the patches we are discussing) for this full GPU
> > functionality in the Xen codebase and will it work with Xen 4.5
> > or will it require extra additional patches? And also, can
> 
> Qemu-traditional + Xen 4.5 + RMRR patchset will work.
> 

Speaking of qemu-traditional.. can you please check this (unapplied) patch:

"[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.":
http://lists.xenproject.org/archives/html/xen-devel/2013-02/msg00538.html

And some discussion/comments about it:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg01385.html

That patch has been floating around for over 1,5 years, so it would be nice to get it cleaned up and applied finally..
based on the description it sounds like it's necessary to fix windows BSOD on certain Intel IGD hardware / drivers.


-- Pasi

Re: Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Konrad Rzeszutek Wilk]

> Qemu-traditional + Xen 4.5 + RMRR patchset will work.
> 
> > all of those patches be tested/reviewed/posted by Oct 13th?
> > 
> > As in, is this patchset the last piece of the puzzle?
> > 
> > And by tested I mean with the passthrough and without, and
> > with various size guests doing migrations (without devices)
> > multiple times (say ten) - and 32 or 64 bit. The QA matrix
> > means at least 8 variations (32/64 - with PCIe passthrough,
> > and without, 2GB or 8GB or 12GB) by my reckoning.
> 
> I am not sure whether we can catch Oct 13th. Does this means those patch will not be accepted by you if we cannot catch Oct 13th?

Before today I would have said no - as I think the rework of these
patches, the addition of a new hypervisor, review, etc in four days
would be highly unlikely - and that meant you would be so busy
working towards a deadline and very likely missing it.

But we are slipping the schedule due to another issue so the RC0 will
move from October 15th to October 25th.

That means you will have some breathing room to work this out.

So lets see those patches and then evaluate them case-by-case basis.
Naturally before even I can give an exception on them - they have
to be OK with the maintainers.

And tested quite extensively.
> 
> > 
> > Sorry for being so aggressive about the testing part but I am
> > very adverse to regressions - as they have bit me in the past
> > and left me with an strong aversion to them.
> 
> 
> Best regards,
> Yang
> 
> 

Re: Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Konrad Rzeszutek Wilk]

On Mon, Sep 29, 2014 at 12:14:05PM -0400, Konrad Rzeszutek Wilk wrote:
> > Qemu-traditional + Xen 4.5 + RMRR patchset will work.
> > 
> > > all of those patches be tested/reviewed/posted by Oct 13th?
> > > 
> > > As in, is this patchset the last piece of the puzzle?
> > > 
> > > And by tested I mean with the passthrough and without, and
> > > with various size guests doing migrations (without devices)
> > > multiple times (say ten) - and 32 or 64 bit. The QA matrix
> > > means at least 8 variations (32/64 - with PCIe passthrough,
> > > and without, 2GB or 8GB or 12GB) by my reckoning.
> > 
> > I am not sure whether we can catch Oct 13th. Does this means those patch will not be accepted by you if we cannot catch Oct 13th?
> 
> Before today I would have said no - as I think the rework of these
> patches, the addition of a new hypervisor, review, etc in four days

And my math is off. The national holiday is from Oct 1st->7th, so that
means: Sep 29th->30th, 8th->10th, and the 13th (instead of 15th to
give it an day to run through oss-test and then potentially fixing
it on the next day). That is 5 days.

> would be highly unlikely - and that meant you would be so busy
> working towards a deadline and very likely missing it.
> 
> But we are slipping the schedule due to another issue so the RC0 will
> move from October 15th to October 25th.
> 
> That means you will have some breathing room to work this out.
> 
> So lets see those patches and then evaluate them case-by-case basis.
> Naturally before even I can give an exception on them - they have
> to be OK with the maintainers.
> 
> And tested quite extensively.
> > 
> > > 
> > > Sorry for being so aggressive about the testing part but I am
> > > very adverse to regressions - as they have bit me in the past
> > > and left me with an strong aversion to them.
> > 
> > 
> > Best regards,
> > Yang
> > 
> > 

Re: Discussion on whether to continue with the patches for Xen 4.5 Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Konrad Rzeszutek Wilk wrote on 2014-09-30:
>> Qemu-traditional + Xen 4.5 + RMRR patchset will work.
>> 
>>> all of those patches be tested/reviewed/posted by Oct 13th?
>>> 
>>> As in, is this patchset the last piece of the puzzle?
>>> 
>>> And by tested I mean with the passthrough and without, and with
>>> various size guests doing migrations (without devices) multiple
>>> times (say ten) - and 32 or 64 bit. The QA matrix means at least 8
>>> variations (32/64 - with PCIe passthrough, and without, 2GB or 8GB
>>> or 12GB) by my reckoning.
>> 
>> I am not sure whether we can catch Oct 13th. Does this means those
>> patch
> will not be accepted by you if we cannot catch Oct 13th?
> 
> Before today I would have said no - as I think the rework of these
> patches, the addition of a new hypervisor, review, etc in four days
> would be highly unlikely - and that meant you would be so busy working
> towards a deadline and very likely missing it.
> 
> But we are slipping the schedule due to another issue so the RC0 will
> move from October 15th to October 25th.
> 
> That means you will have some breathing room to work this out.

Yes, it is good news to us but maybe bad news to you. :)

> 
> So lets see those patches and then evaluate them case-by-case basis.
> Naturally before even I can give an exception on them - they have to
> be OK with the maintainers.
> 
> And tested quite extensively.
>> 
>>> 
>>> Sorry for being so aggressive about the testing part but I am very
>>> adverse to regressions - as they have bit me in the past and left
>>> me with an strong aversion to them.
>> 
>> 
>> Best regards,
>> Yang
>> 
>>


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-26:
>>>> On 26.09.14 at 03:24, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-09-25:
>>>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
>>>> How do the checking in P2M updates? Only hvmloader knows whether
>>>> the RMRR region is reserved. If we want do the check in
>>>> hypervisor, we need to report those info to hypervisor.
>>> 
>>> First of all the hypervisor has the information - it is where the
>>> information comes from that tools and hvmloader consume. And then
>>> the check will need to be a collision check: If while establishing
>>> an identity mapping another mapping is found to be already in
>>> place, the request will need to be failed. And similarly if a
>>> "normal" mapping request finds a 1:1 mapping already in place, this
>>> ought to result in failure too. Of course a prerequisite to this is
>>> that error get properly
>> bubbled up through all layers.
>> 
>> If there is another mapping already there and it is different from
>> the one we are establishing, we should return failure. But if the
>> existing mapping is same to the one we are establishing, how we can
>> know whether it is a 'normal memory 1:1' mapping or it is created by
>> previous operation of RMRR creating(e.g. there already a device with
>> RMRR assigned to this guest). What I am thinking is that we need a
>> flag to know whether the mapping is RMRR mapping or regular memory
> mapping.
> 
> If the new and old mappings are the same, nothing needs to be done at
> all (as is already the case in one direction in the patches we have
> seen). And yes, for the case when there is an occasional 1:1 mapping
> we of course will need some way of identifying intentional ones.

So how about adding a new p2m type to do this? It may also helpful when creating a guest without device attached.(I mentioned it in another thread).

> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-25:
>>>> On 25.09.14 at 03:53, <yang.z.zhang@intel.com> wrote:
>> Jan Beulich wrote on 2014-09-24:
>>>>>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
>>>> 1. RMRR region isn't reserved in guest e820 table and guest is
>>>> able to touch it.
>>>> 
>>>> Possible solution: set RMRR region as reserved in guest e820
>>>> table and create identity map in EPT and VT-d page table.
>>> 
>>> And relocate guest RAM accordingly.
>> 
>> ok. Let's look into more detail:
>> 1. Whether there has device assigned or not, the RMRR mapping must
>> be created when building e820 table if the VT-d is enabled.

My original propose should be wrong: if there is no device attached when creating guest, we should not create the identity map in guest. But I don't know how to setup the EPT mapping for RMRR region in this case. Any idea?

>> 2. Despite of share or non-share case, the RMRR region must be
>> identity map in EPT and VT-d page table.
>> 3. Tiejun's patch uses hypercall to get the RMRR info, is it ok? Or
>> should we get it from xenstore, and then both tools and hvmloader
>> can
> access it?
> 
> The hypercall is clearly the route to go imo - xenstore would be a
> pretty crude mechanism for conveying that information (and using the
> hypercall approach precludes neither the tools nor hvmloader from obtaining that data).

Ok. If hypercall is enough, then we can keep it.

> 
>>>> 3. RMRR region isn't checked when updating EPT table and VT-d table.
>>>> 
>>>> Possible solution: Return error when trying to update EPT and
>>>> VT-d table if the gfn is inside RMRR region.
>> 
>> So just do a simple check in EPT table and VT-d table updating is ok?
> 
> I think so - anything more sophisticated (like checking in the tools)
> will not give any better results (except for a more explicit error
> message maybe, but this can certainly be had equally well by using a
> very specific error code should the hypervisor side check fail).

Agree.

Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.09.14 at 05:49, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-26:
>>>>> On 26.09.14 at 03:24, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-09-25:
>>>>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
>>>>> How do the checking in P2M updates? Only hvmloader knows whether
>>>>> the RMRR region is reserved. If we want do the check in
>>>>> hypervisor, we need to report those info to hypervisor.
>>>> 
>>>> First of all the hypervisor has the information - it is where the
>>>> information comes from that tools and hvmloader consume. And then
>>>> the check will need to be a collision check: If while establishing
>>>> an identity mapping another mapping is found to be already in
>>>> place, the request will need to be failed. And similarly if a
>>>> "normal" mapping request finds a 1:1 mapping already in place, this
>>>> ought to result in failure too. Of course a prerequisite to this is
>>>> that error get properly
>>> bubbled up through all layers.
>>> 
>>> If there is another mapping already there and it is different from
>>> the one we are establishing, we should return failure. But if the
>>> existing mapping is same to the one we are establishing, how we can
>>> know whether it is a 'normal memory 1:1' mapping or it is created by
>>> previous operation of RMRR creating(e.g. there already a device with
>>> RMRR assigned to this guest). What I am thinking is that we need a
>>> flag to know whether the mapping is RMRR mapping or regular memory
>> mapping.
>> 
>> If the new and old mappings are the same, nothing needs to be done at
>> all (as is already the case in one direction in the patches we have
>> seen). And yes, for the case when there is an occasional 1:1 mapping
>> we of course will need some way of identifying intentional ones.
> 
> So how about adding a new p2m type to do this? It may also helpful when 
> creating a guest without device attached.(I mentioned it in another thread).

If that makes it easier to implement - why not? But I think you're aware
that raising memory management related questions without Tim in the
loop isn't going to yield a result you can reasonably rely on later being
accepted in patch form.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.09.14 at 05:51, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-25:
>>>>> On 25.09.14 at 03:53, <yang.z.zhang@intel.com> wrote:
>>> Jan Beulich wrote on 2014-09-24:
>>>>>>> On 24.09.14 at 10:23, <yang.z.zhang@intel.com> wrote:
>>>>> 1. RMRR region isn't reserved in guest e820 table and guest is
>>>>> able to touch it.
>>>>> 
>>>>> Possible solution: set RMRR region as reserved in guest e820
>>>>> table and create identity map in EPT and VT-d page table.
>>>> 
>>>> And relocate guest RAM accordingly.
>>> 
>>> ok. Let's look into more detail:
>>> 1. Whether there has device assigned or not, the RMRR mapping must
>>> be created when building e820 table if the VT-d is enabled.
> 
> My original propose should be wrong: if there is no device attached when 
> creating guest, we should not create the identity map in guest. But I don't 
> know how to setup the EPT mapping for RMRR region in this case. Any idea?

Of course these mappings shouldn't be created blindly. There simply
shouldn't be mapped anything in these wholes, so that the identity
mappings can be put there if needed. That's why we need tool stack
awareness of these regions after all.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tim Deegan]

At 08:07 +0100 on 30 Sep (1412060848), Jan Beulich wrote:
> >>> On 30.09.14 at 05:49, <yang.z.zhang@intel.com> wrote:
> > Jan Beulich wrote on 2014-09-26:
> >>>>> On 26.09.14 at 03:24, <yang.z.zhang@intel.com> wrote:
> >>> Jan Beulich wrote on 2014-09-25:
> >>>>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
> >>>>> How do the checking in P2M updates? Only hvmloader knows whether
> >>>>> the RMRR region is reserved. If we want do the check in
> >>>>> hypervisor, we need to report those info to hypervisor.
> >>>> 
> >>>> First of all the hypervisor has the information - it is where the
> >>>> information comes from that tools and hvmloader consume. And then
> >>>> the check will need to be a collision check: If while establishing
> >>>> an identity mapping another mapping is found to be already in
> >>>> place, the request will need to be failed. And similarly if a
> >>>> "normal" mapping request finds a 1:1 mapping already in place, this
> >>>> ought to result in failure too. Of course a prerequisite to this is
> >>>> that error get properly
> >>> bubbled up through all layers.
> >>> 
> >>> If there is another mapping already there and it is different from
> >>> the one we are establishing, we should return failure. But if the
> >>> existing mapping is same to the one we are establishing, how we can
> >>> know whether it is a 'normal memory 1:1' mapping or it is created by
> >>> previous operation of RMRR creating(e.g. there already a device with
> >>> RMRR assigned to this guest). What I am thinking is that we need a
> >>> flag to know whether the mapping is RMRR mapping or regular memory
> >> mapping.
> >> 
> >> If the new and old mappings are the same, nothing needs to be done at
> >> all (as is already the case in one direction in the patches we have
> >> seen). And yes, for the case when there is an occasional 1:1 mapping
> >> we of course will need some way of identifying intentional ones.
> > 
> > So how about adding a new p2m type to do this? It may also helpful when 
> > creating a guest without device attached.(I mentioned it in another thread).
> 
> If that makes it easier to implement - why not? But I think you're aware
> that raising memory management related questions without Tim in the
> loop isn't going to yield a result you can reasonably rely on later being
> accepted in patch form.

Although adding new p2m types is generally OK, I'm not sure I see the
need here.  The useful cases are trivial:
 - gfn space unoccupied -> insert mapping; success.
 - gfn space already occupied by 1:1 RMRR mapping -> do nothing; success.
 - gfn space already occupied by other mapping -> fail.

The remaining case is where the gfn space happens to be entirely
occupied by an existing 1:1 mapping of RMRR that wasn't put there by
RMRR code.  If you really want to detect this I think a simple
reference count of how may times this VM has had the RMRR mapped will
do.  If that count is 0 and you see a mapping, fail (even if the
mapping looks right).

Cheers,

Tim.

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Tim Deegan [mailto:tim@xen.org]
> Sent: Thursday, October 02, 2014 3:30 AM
> 
> At 08:07 +0100 on 30 Sep (1412060848), Jan Beulich wrote:
> > >>> On 30.09.14 at 05:49, <yang.z.zhang@intel.com> wrote:
> > > Jan Beulich wrote on 2014-09-26:
> > >>>>> On 26.09.14 at 03:24, <yang.z.zhang@intel.com> wrote:
> > >>> Jan Beulich wrote on 2014-09-25:
> > >>>>>>> On 25.09.14 at 04:30, <yang.z.zhang@intel.com> wrote:
> > >>>>> How do the checking in P2M updates? Only hvmloader knows whether
> > >>>>> the RMRR region is reserved. If we want do the check in
> > >>>>> hypervisor, we need to report those info to hypervisor.
> > >>>>
> > >>>> First of all the hypervisor has the information - it is where the
> > >>>> information comes from that tools and hvmloader consume. And then
> > >>>> the check will need to be a collision check: If while establishing
> > >>>> an identity mapping another mapping is found to be already in
> > >>>> place, the request will need to be failed. And similarly if a
> > >>>> "normal" mapping request finds a 1:1 mapping already in place, this
> > >>>> ought to result in failure too. Of course a prerequisite to this is
> > >>>> that error get properly
> > >>> bubbled up through all layers.
> > >>>
> > >>> If there is another mapping already there and it is different from
> > >>> the one we are establishing, we should return failure. But if the
> > >>> existing mapping is same to the one we are establishing, how we can
> > >>> know whether it is a 'normal memory 1:1' mapping or it is created by
> > >>> previous operation of RMRR creating(e.g. there already a device with
> > >>> RMRR assigned to this guest). What I am thinking is that we need a
> > >>> flag to know whether the mapping is RMRR mapping or regular memory
> > >> mapping.
> > >>
> > >> If the new and old mappings are the same, nothing needs to be done at
> > >> all (as is already the case in one direction in the patches we have
> > >> seen). And yes, for the case when there is an occasional 1:1 mapping
> > >> we of course will need some way of identifying intentional ones.
> > >
> > > So how about adding a new p2m type to do this? It may also helpful when
> > > creating a guest without device attached.(I mentioned it in another
> thread).
> >
> > If that makes it easier to implement - why not? But I think you're aware
> > that raising memory management related questions without Tim in the
> > loop isn't going to yield a result you can reasonably rely on later being
> > accepted in patch form.
> 
> Although adding new p2m types is generally OK, I'm not sure I see the
> need here.  The useful cases are trivial:
>  - gfn space unoccupied -> insert mapping; success.
>  - gfn space already occupied by 1:1 RMRR mapping -> do nothing; success.
>  - gfn space already occupied by other mapping -> fail.
> 
> The remaining case is where the gfn space happens to be entirely
> occupied by an existing 1:1 mapping of RMRR that wasn't put there by
> RMRR code.  If you really want to detect this I think a simple
> reference count of how may times this VM has had the RMRR mapped will
> do.  If that count is 0 and you see a mapping, fail (even if the
> mapping looks right).
> 

I can't consider a valid case for above scenario, i.e. an existing 1:1
mapping of RMRR that wasn't put there by RMRR code. If there is,
it has to be a bug. The only valid case we'll see is that two devices
have same RMRR range, and two devices are assigned to the same
VM. In such case, assignment of the 2nd device would see an existing
RMRR identity mapping, but then it's desired.

So I'd agree with the simple policy of what Tim suggested:
>  - gfn space unoccupied -> insert mapping; success.
>  - gfn space already occupied by 1:1 RMRR mapping -> do nothing; success.
>  - gfn space already occupied by other mapping -> fail.

And then the reference code should be in the per-VM RMRR structure.

Thanks
Kevin

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 19.09.14 at 10:30, <tiejun.chen@intel.com> wrote:
> On 2014/9/19 16:06, Jan Beulich wrote:
>>>>> On 19.09.14 at 09:40, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/19 15:10, Jan Beulich wrote:
>>>>>>> On 19.09.14 at 08:50, <tiejun.chen@intel.com> wrote:
>>>>> On 2014/9/19 14:26, Jan Beulich wrote:
>>>>>>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>>>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>>>>>>> ab818fff
>>>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>>>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>>>>>>> af7fffff
>>>>>>
>>>>>> So how does passing through either of these work for a guest with
>>>>>> 4Gb or more of memory assigned with just the original 2 patches
>>>>>> (and with shared page tables)? There ought to be a collision detected
>>>>>> when trying to do the identity mapping.
>>>>>
>>>>> Do you mean this point, mfn_valid(mfn)? If yes, I remember we made
>>>>> agreement previously about how to cover three cases including this scenario:
>>>>>
>>>>> "
>>>>> #1: !mfn_valid(mfn)
>>>>>
>>>>> We can create those mapping safely.
>>>>>
>>>>> #2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw
>>>>>
>>>>> We already have these matched mappings.
>>>>>
>>>>> #3: Others
>>>>>
>>>>> Return with that waring message: "Cannot identity map d%d:%lx, already
>>>>> mapped to %lx but mismatch.\n"
>>>>> "
>>>>> And this is just as we did in patch #1:
>>>>>
>>>>> +
>>>>> +    if ( !mfn_valid(mfn) )
>>>>> +        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>>>>> p2m_mmio_direct,
>>>>> +                            p2m_access_rw);
>>>>> +    else if ( mfn_x(mfn) == gfn &&
>>>>> +              p2mt == p2m_mmio_direct &&
>>>>> +              a == p2m_access_rw )
>>>>> +        ret = 0;
>>>>> +    else
>>>>> +        printk(XENLOG_G_WARNING
>>>>> +               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
>>>>> +               d->domain_id, gfn, mfn_x(mfn));
>>>>
>>>> Right, but the important point is that when the warning gets printed
>>>> -EBUSY gets returned, i.e. in the end the device assignment is to fail.
>>>> Are you seeing the warning when creating a large enough guest? If
>>>
>>> My platform just own 4G memory, so after I try to set 'memory=15360' in
>>> domu.cfg, I can't boot such a VM:
>>>
>>> # xl cr domu.cfg
>>> Parsing config from domu.cfg
>>> libxl: error: libxl.c:4202:libxl_set_memory_target: new target 0 for
>>> dom0 is below the minimum threshhold
>>> ...
>>> failed to free memory for the domain
>>>
>>>> not - can you explain why you don't see it (as I can't)?
>>>
>>> Do you know exactly how to test this case as you expect here? Then I can
>>> take a further look to step on your question. Or I guess you are hinting
>>> something wrong should be happened but I never realize that previously.
>>
>> It should suffice to give 3 Gb (or event slightly less) of memory to
>> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).
> 
> Yes. So I can't produce that real case of conflict with those existing 
> RMRR in my platform.

When you pass 3Gb to the guest, its memory map should extend to
about 0xC0000000, well beyond the range the RMRRs reference. So
you ought to be able to see the collision (or if you don't you ought to
have ways to find out why they're not happening, as that would be a
sign of something else being bogus).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/19 16:06, Jan Beulich wrote:
>>>> On 19.09.14 at 09:40, <tiejun.chen@intel.com> wrote:
>> On 2014/9/19 15:10, Jan Beulich wrote:
>>>>>> On 19.09.14 at 08:50, <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/19 14:26, Jan Beulich wrote:
>>>>>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>>>>>> ab818fff
>>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>>>>>> af7fffff
>>>>>
>>>>> So how does passing through either of these work for a guest with
>>>>> 4Gb or more of memory assigned with just the original 2 patches
>>>>> (and with shared page tables)? There ought to be a collision detected
>>>>> when trying to do the identity mapping.
>>>>
>>>> Do you mean this point, mfn_valid(mfn)? If yes, I remember we made
>>>> agreement previously about how to cover three cases including this scenario:
>>>>
>>>> "
>>>> #1: !mfn_valid(mfn)
>>>>
>>>> We can create those mapping safely.
>>>>
>>>> #2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw
>>>>
>>>> We already have these matched mappings.
>>>>
>>>> #3: Others
>>>>
>>>> Return with that waring message: "Cannot identity map d%d:%lx, already
>>>> mapped to %lx but mismatch.\n"
>>>> "
>>>> And this is just as we did in patch #1:
>>>>
>>>> +
>>>> +    if ( !mfn_valid(mfn) )
>>>> +        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>>>> p2m_mmio_direct,
>>>> +                            p2m_access_rw);
>>>> +    else if ( mfn_x(mfn) == gfn &&
>>>> +              p2mt == p2m_mmio_direct &&
>>>> +              a == p2m_access_rw )
>>>> +        ret = 0;
>>>> +    else
>>>> +        printk(XENLOG_G_WARNING
>>>> +               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
>>>> +               d->domain_id, gfn, mfn_x(mfn));
>>>
>>> Right, but the important point is that when the warning gets printed
>>> -EBUSY gets returned, i.e. in the end the device assignment is to fail.
>>> Are you seeing the warning when creating a large enough guest? If
>>
>> My platform just own 4G memory, so after I try to set 'memory=15360' in
>> domu.cfg, I can't boot such a VM:
>>
>> # xl cr domu.cfg
>> Parsing config from domu.cfg
>> libxl: error: libxl.c:4202:libxl_set_memory_target: new target 0 for
>> dom0 is below the minimum threshhold
>> ...
>> failed to free memory for the domain
>>
>>> not - can you explain why you don't see it (as I can't)?
>>
>> Do you know exactly how to test this case as you expect here? Then I can
>> take a further look to step on your question. Or I guess you are hinting
>> something wrong should be happened but I never realize that previously.
>
> It should suffice to give 3 Gb (or event slightly less) of memory to
> the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).

Yes. So I can't produce that real case of conflict with those existing 
RMRR in my platform.

I will have to try seeking a appropriate machine.

Thanks
Tiejun

>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 19.09.14 at 09:40, <tiejun.chen@intel.com> wrote:
> On 2014/9/19 15:10, Jan Beulich wrote:
>>>>> On 19.09.14 at 08:50, <tiejun.chen@intel.com> wrote:
>>> On 2014/9/19 14:26, Jan Beulich wrote:
>>>>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>>>>> ab818fff
>>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>>>>> af7fffff
>>>>
>>>> So how does passing through either of these work for a guest with
>>>> 4Gb or more of memory assigned with just the original 2 patches
>>>> (and with shared page tables)? There ought to be a collision detected
>>>> when trying to do the identity mapping.
>>>
>>> Do you mean this point, mfn_valid(mfn)? If yes, I remember we made
>>> agreement previously about how to cover three cases including this scenario:
>>>
>>> "
>>> #1: !mfn_valid(mfn)
>>>
>>> We can create those mapping safely.
>>>
>>> #2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw
>>>
>>> We already have these matched mappings.
>>>
>>> #3: Others
>>>
>>> Return with that waring message: "Cannot identity map d%d:%lx, already
>>> mapped to %lx but mismatch.\n"
>>> "
>>> And this is just as we did in patch #1:
>>>
>>> +
>>> +    if ( !mfn_valid(mfn) )
>>> +        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>>> p2m_mmio_direct,
>>> +                            p2m_access_rw);
>>> +    else if ( mfn_x(mfn) == gfn &&
>>> +              p2mt == p2m_mmio_direct &&
>>> +              a == p2m_access_rw )
>>> +        ret = 0;
>>> +    else
>>> +        printk(XENLOG_G_WARNING
>>> +               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
>>> +               d->domain_id, gfn, mfn_x(mfn));
>>
>> Right, but the important point is that when the warning gets printed
>> -EBUSY gets returned, i.e. in the end the device assignment is to fail.
>> Are you seeing the warning when creating a large enough guest? If
> 
> My platform just own 4G memory, so after I try to set 'memory=15360' in 
> domu.cfg, I can't boot such a VM:
> 
> # xl cr domu.cfg
> Parsing config from domu.cfg
> libxl: error: libxl.c:4202:libxl_set_memory_target: new target 0 for 
> dom0 is below the minimum threshhold
> ...
> failed to free memory for the domain
> 
>> not - can you explain why you don't see it (as I can't)?
> 
> Do you know exactly how to test this case as you expect here? Then I can 
> take a further look to step on your question. Or I guess you are hinting 
> something wrong should be happened but I never realize that previously.

It should suffice to give 3 Gb (or event slightly less) of memory to
the DomU (if your Dom0 can hopefully tolerate running with just 1Gb).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/19 15:10, Jan Beulich wrote:
>>>> On 19.09.14 at 08:50, <tiejun.chen@intel.com> wrote:
>> On 2014/9/19 14:26, Jan Beulich wrote:
>>>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>>>> ab818fff
>>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>>>> af7fffff
>>>
>>> So how does passing through either of these work for a guest with
>>> 4Gb or more of memory assigned with just the original 2 patches
>>> (and with shared page tables)? There ought to be a collision detected
>>> when trying to do the identity mapping.
>>
>> Do you mean this point, mfn_valid(mfn)? If yes, I remember we made
>> agreement previously about how to cover three cases including this scenario:
>>
>> "
>> #1: !mfn_valid(mfn)
>>
>> We can create those mapping safely.
>>
>> #2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw
>>
>> We already have these matched mappings.
>>
>> #3: Others
>>
>> Return with that waring message: "Cannot identity map d%d:%lx, already
>> mapped to %lx but mismatch.\n"
>> "
>> And this is just as we did in patch #1:
>>
>> +
>> +    if ( !mfn_valid(mfn) )
>> +        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>> p2m_mmio_direct,
>> +                            p2m_access_rw);
>> +    else if ( mfn_x(mfn) == gfn &&
>> +              p2mt == p2m_mmio_direct &&
>> +              a == p2m_access_rw )
>> +        ret = 0;
>> +    else
>> +        printk(XENLOG_G_WARNING
>> +               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
>> +               d->domain_id, gfn, mfn_x(mfn));
>
> Right, but the important point is that when the warning gets printed
> -EBUSY gets returned, i.e. in the end the device assignment is to fail.
> Are you seeing the warning when creating a large enough guest? If

My platform just own 4G memory, so after I try to set 'memory=15360' in 
domu.cfg, I can't boot such a VM:

# xl cr domu.cfg
Parsing config from domu.cfg
libxl: error: libxl.c:4202:libxl_set_memory_target: new target 0 for 
dom0 is below the minimum threshhold
...
failed to free memory for the domain

> not - can you explain why you don't see it (as I can't)?

Do you know exactly how to test this case as you expect here? Then I can 
take a further look to step on your question. Or I guess you are hinting 
something wrong should be happened but I never realize that previously.

Thanks
Tiejun

>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 19.09.14 at 08:50, <tiejun.chen@intel.com> wrote:
> On 2014/9/19 14:26, Jan Beulich wrote:
>>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>>> ab818fff
>>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>>> af7fffff
>>
>> So how does passing through either of these work for a guest with
>> 4Gb or more of memory assigned with just the original 2 patches
>> (and with shared page tables)? There ought to be a collision detected
>> when trying to do the identity mapping.
> 
> Do you mean this point, mfn_valid(mfn)? If yes, I remember we made 
> agreement previously about how to cover three cases including this scenario:
> 
> "
> #1: !mfn_valid(mfn)
> 
> We can create those mapping safely.
> 
> #2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw
> 
> We already have these matched mappings.
> 
> #3: Others
> 
> Return with that waring message: "Cannot identity map d%d:%lx, already 
> mapped to %lx but mismatch.\n"
> "
> And this is just as we did in patch #1:
> 
> +
> +    if ( !mfn_valid(mfn) )
> +        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, 
> p2m_mmio_direct,
> +                            p2m_access_rw);
> +    else if ( mfn_x(mfn) == gfn &&
> +              p2mt == p2m_mmio_direct &&
> +              a == p2m_access_rw )
> +        ret = 0;
> +    else
> +        printk(XENLOG_G_WARNING
> +               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
> +               d->domain_id, gfn, mfn_x(mfn));

Right, but the important point is that when the warning gets printed
-EBUSY gets returned, i.e. in the end the device assignment is to fail.
Are you seeing the warning when creating a large enough guest? If
not - can you explain why you don't see it (as I can't)?

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/19 14:26, Jan Beulich wrote:
>>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
>> On 2014/9/18 17:09, Jan Beulich wrote:
>>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>>> --- 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_identity_p2m_entry(d, base_pfn) )
>>>> +                return -1;
>>>> +        }
>>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>>>                return -1;
>>>>            base_pfn++;
>>>>        }
>>>
>>> So I gave this a try on the one box I have which exposes RMRRs
>>> (since those are for USB devices I also used your patch to drop
>>> the USB special casing as done in your later patch series, and I
>>> further had to fiddle with vtd_ept_page_compatible() in order to
>>> get page table sharing to actually work on that box [I'll send the
>>> resulting patch later]) - with the result that passing through an
>>> affected USB controller (as expected) doesn't work anymore. Which
>>
>> With or without these two patches, USB always can be passed through in
>> my platform. Note I'm using ubuntu as a Guest OS.
>>
>>> raises the question why the two patches alone would work at all.
>>> Could you please share information on the address ranges specified
>>> by the RMRR(s) in your case? I simply wonder whether things just
>>
>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address
>> ab818fff
>> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
>> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
>> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address
>> af7fffff
>
> So how does passing through either of these work for a guest with
> 4Gb or more of memory assigned with just the original 2 patches
> (and with shared page tables)? There ought to be a collision detected
> when trying to do the identity mapping.

Do you mean this point, mfn_valid(mfn)? If yes, I remember we made 
agreement previously about how to cover three cases including this scenario:

"
#1: !mfn_valid(mfn)

We can create those mapping safely.

#2: mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2m_access_rw

We already have these matched mappings.

#3: Others

Return with that waring message: "Cannot identity map d%d:%lx, already 
mapped to %lx but mismatch.\n"
"
And this is just as we did in patch #1:

+
+    if ( !mfn_valid(mfn) )
+        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K, 
p2m_mmio_direct,
+                            p2m_access_rw);
+    else if ( mfn_x(mfn) == gfn &&
+              p2mt == p2m_mmio_direct &&
+              a == p2m_access_rw )
+        ret = 0;
+    else
+        printk(XENLOG_G_WARNING
+               "Cannot identity map d%d:%lx, already mapped to %lx.\n",
+               d->domain_id, gfn, mfn_x(mfn));

Thanks
Tiejun

>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 19.09.14 at 04:43, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-18:
>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>> --- 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_identity_p2m_entry(d,
>>> base_pfn) ) +                return -1; +        } +        else if (
>>> intel_iommu_map_page(d, base_pfn, base_pfn, + +
>>> IOMMUF_readable|IOMMUF_writable) )
>>>              return -1;
>>>          base_pfn++;
>>>      }
>> 
>> So I gave this a try on the one box I have which exposes RMRRs (since
>> those are for USB devices I also used your patch to drop the USB special
>> casing as done in your later patch series, and I further had to fiddle
>> with vtd_ept_page_compatible() in order to get page table sharing to
>> actually work on that box [I'll send the resulting patch later]) - with
>> the result that passing through an affected USB controller (as expected)
>> doesn't work anymore. Which raises the question why the two patches
>> alone would work at all. Could you please share information on the
>> address ranges specified by the RMRR(s) in your case? I simply wonder
>> whether things just happen to work for you on the particular system(s)
>> you're testing on, as I'd generally expect an address space collision to
>> be possible for any RMRR.
>> 
>> I think you understand the consequences: If the series here has no way
>> of reliably working without the other one, "iommu=no-sharept"
>> is going to be the solution for you, at once being one more argument
>> towards dropping page table sharing altogether. The one argument in
>> favor of the two patches here would be that they at least detect the
>> collision now, thus forcing people to suppress page table sharing.
>> 
>> But what's worse, I can't see how the non-sharing case is being
>> handled correctly either (independent of the series here):
>> rmrr_identity_mapping() blindly overwrites what may already be in the
>> page tables, breaking consistency with the CPU-side P2M (iiuc this is
>> a problem even for PV, including Dom0). Plus there's nothing being
>> done to prevent subsequent overwriting of these
>> 1:1 entries by "normal" P2M manipulations. All in all another argument
>> not to allow (at least by default) passing through of devices
>> associated with one or more RMRRs.
> 
> This is another issue that current RMRR handling logic is not right which I 
> think we have discussed long time ago.
> http://lists.xen.org/archives/html/xen-devel/2014-07/msg03249.html 
> 
> Obviously, current RMRR handling logic is totally wrong. It is not a simple 
> task to do it which I think Tiejun already spent about two months but we 
> still have some divergences. From my point, this patch will mitigate this 
> issue. At least, it doesn't make thing worse.

In fact it does (as pointed out by the tests I conducted for that
very reason) cause a regression (at least a perceived one), which
iirc was already mentioned earlier: Devices associated with RMRRs
where the RMRRs aren't actually getting accessed post-boot can
currently be passed through fine (and without other than a purely
theoretical security implication) now, but won't with the patch in
place.

Apart from that, the absolute minimum of extra work needed here
would imo be to make the separate-pt case detect address space
collisions the same way as the shared-pt case does with the two
patches here in place, so that there's no divergence in behavior.

Yet again - for 4.5 I'd favor disallowing assignment of devices
associated with RMRRs altogether.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 19.09.14 at 03:20, <tiejun.chen@intel.com> wrote:
> On 2014/9/18 17:09, Jan Beulich wrote:
>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>> --- 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_identity_p2m_entry(d, base_pfn) )
>>> +                return -1;
>>> +        }
>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>>               return -1;
>>>           base_pfn++;
>>>       }
>>
>> So I gave this a try on the one box I have which exposes RMRRs
>> (since those are for USB devices I also used your patch to drop
>> the USB special casing as done in your later patch series, and I
>> further had to fiddle with vtd_ept_page_compatible() in order to
>> get page table sharing to actually work on that box [I'll send the
>> resulting patch later]) - with the result that passing through an
>> affected USB controller (as expected) doesn't work anymore. Which
> 
> With or without these two patches, USB always can be passed through in 
> my platform. Note I'm using ubuntu as a Guest OS.
> 
>> raises the question why the two patches alone would work at all.
>> Could you please share information on the address ranges specified
>> by the RMRR(s) in your case? I simply wonder whether things just
> 
> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address 
> ab818fff
> (XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
> (XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
> (XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address 
> af7fffff

So how does passing through either of these work for a guest with
4Gb or more of memory assigned with just the original 2 patches
(and with shared page tables)? There ought to be a collision detected
when trying to do the identity mapping.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-18:
>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>> --- 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_identity_p2m_entry(d,
>> base_pfn) ) +                return -1; +        } +        else if (
>> intel_iommu_map_page(d, base_pfn, base_pfn, + +
>> IOMMUF_readable|IOMMUF_writable) )
>>              return -1;
>>          base_pfn++;
>>      }
> 
> So I gave this a try on the one box I have which exposes RMRRs (since
> those are for USB devices I also used your patch to drop the USB special
> casing as done in your later patch series, and I further had to fiddle
> with vtd_ept_page_compatible() in order to get page table sharing to
> actually work on that box [I'll send the resulting patch later]) - with
> the result that passing through an affected USB controller (as expected)
> doesn't work anymore. Which raises the question why the two patches
> alone would work at all. Could you please share information on the
> address ranges specified by the RMRR(s) in your case? I simply wonder
> whether things just happen to work for you on the particular system(s)
> you're testing on, as I'd generally expect an address space collision to
> be possible for any RMRR.
> 
> I think you understand the consequences: If the series here has no way
> of reliably working without the other one, "iommu=no-sharept"
> is going to be the solution for you, at once being one more argument
> towards dropping page table sharing altogether. The one argument in
> favor of the two patches here would be that they at least detect the
> collision now, thus forcing people to suppress page table sharing.
> 
> But what's worse, I can't see how the non-sharing case is being
> handled correctly either (independent of the series here):
> rmrr_identity_mapping() blindly overwrites what may already be in the
> page tables, breaking consistency with the CPU-side P2M (iiuc this is
> a problem even for PV, including Dom0). Plus there's nothing being
> done to prevent subsequent overwriting of these
> 1:1 entries by "normal" P2M manipulations. All in all another argument
> not to allow (at least by default) passing through of devices
> associated with one or more RMRRs.

This is another issue that current RMRR handling logic is not right which I think we have discussed long time ago.
http://lists.xen.org/archives/html/xen-devel/2014-07/msg03249.html

Obviously, current RMRR handling logic is totally wrong. It is not a simple task to do it which I think Tiejun already spent about two months but we still have some divergences. From my point, this patch will mitigate this issue. At least, it doesn't make thing worse. Considering the Xen 4.5 release, we should let this patch in. For remain part, Tiejun will continue working on it to make the whole thing work well. Besides, since you have some misunderstood on our work style, why not give a chance to let us prove it?

> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/18 17:09, Jan Beulich wrote:
>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>> --- 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_identity_p2m_entry(d, base_pfn) )
>> +                return -1;
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>               return -1;
>>           base_pfn++;
>>       }
>
> So I gave this a try on the one box I have which exposes RMRRs
> (since those are for USB devices I also used your patch to drop
> the USB special casing as done in your later patch series, and I
> further had to fiddle with vtd_ept_page_compatible() in order to
> get page table sharing to actually work on that box [I'll send the
> resulting patch later]) - with the result that passing through an
> affected USB controller (as expected) doesn't work anymore. Which

With or without these two patches, USB always can be passed through in 
my platform. Note I'm using ubuntu as a Guest OS.

> raises the question why the two patches alone would work at all.
> Could you please share information on the address ranges specified
> by the RMRR(s) in your case? I simply wonder whether things just

(XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:14.0
(XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ab805000 end_address 
ab818fff
(XEN) [VT-D]dmar.c:807: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:383:  endpoint: 0000:00:02.0
(XEN) [VT-D]dmar.c:676:   RMRR region: base_addr ad000000 end_address 
af7fffff


> happen to work for you on the particular system(s) you're testing
> on, as I'd generally expect an address space collision to be possible
> for any RMRR.
>
> I think you understand the consequences: If the series here has no
> way of reliably working without the other one, "iommu=no-sharept"

This already is our known way but we need to support the PT in both 
shared and non-shared cases.

> is going to be the solution for you, at once being one more argument
> towards dropping page table sharing altogether. The one argument
> in favor of the two patches here would be that they at least detect
> the collision now, thus forcing people to suppress page table sharing.

Sorry this sort of estimate is out of the scope I can answer properly. 
Maybe Yang or Kevin can do follow this if possible.

>
> But what's worse, I can't see how the non-sharing case is being
> handled correctly either (independent of the series here):
> rmrr_identity_mapping() blindly overwrites what may already be
> in the page tables, breaking consistency with the CPU-side P2M
> (iiuc this is a problem even for PV, including Dom0). Plus there's
> nothing being done to prevent subsequent overwriting of these
> 1:1 entries by "normal" P2M manipulations. All in all another

I'm not sure this as well.

Yang and Kevin,

What are your comments about this point?

> argument not to allow (at least by default) passing through of
> devices associated with one or more RMRRs.

Here I have to wait Kevin and Yang's comments since they're familiar 
with these internals than me :), then I can try to figure out 
appropriate ways to fix these arguments if they really exist.

Thanks
Tiejun

>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
> --- 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_identity_p2m_entry(d, base_pfn) )
> +                return -1;
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +                                       IOMMUF_readable|IOMMUF_writable) )
>              return -1;
>          base_pfn++;
>      }

So I gave this a try on the one box I have which exposes RMRRs
(since those are for USB devices I also used your patch to drop
the USB special casing as done in your later patch series, and I
further had to fiddle with vtd_ept_page_compatible() in order to
get page table sharing to actually work on that box [I'll send the
resulting patch later]) - with the result that passing through an
affected USB controller (as expected) doesn't work anymore. Which
raises the question why the two patches alone would work at all.
Could you please share information on the address ranges specified
by the RMRR(s) in your case? I simply wonder whether things just
happen to work for you on the particular system(s) you're testing
on, as I'd generally expect an address space collision to be possible
for any RMRR.

I think you understand the consequences: If the series here has no
way of reliably working without the other one, "iommu=no-sharept"
is going to be the solution for you, at once being one more argument
towards dropping page table sharing altogether. The one argument
in favor of the two patches here would be that they at least detect
the collision now, thus forcing people to suppress page table sharing.

But what's worse, I can't see how the non-sharing case is being
handled correctly either (independent of the series here):
rmrr_identity_mapping() blindly overwrites what may already be
in the page tables, breaking consistency with the CPU-side P2M
(iiuc this is a problem even for PV, including Dom0). Plus there's
nothing being done to prevent subsequent overwriting of these
1:1 entries by "normal" P2M manipulations. All in all another
argument not to allow (at least by default) passing through of
devices associated with one or more RMRRs.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 18.09.14 at 09:41, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-09-18:
>>>>> On 18.09.14 at 04:02, <yang.z.zhang@intel.com> wrote:
>>> [...]
>>> You didn't object my comment, and the patch was applied to Xen.
>>> 
>>> Later, you gave the comment:
>>> "I should also say that while I certainly understand the
>>> argumentation above, I would still want to go this route only with
>>> the promise that B is going to be worked on reasonably soon after
>>> the release, ideally with the goal of backporting the changes for 4.4.1."
>> 
>> No, the order of things was that the comment was given first, and then
> 
> The patch is applied at
> 
> commit 077fc1c04d70ef1748ac2daa6622b3320a1a004c
> Author: Yang Zhang <yang.z.zhang@Intel.com>
> Date:   Thu Feb 13 15:50:22 2014 +0000
> 
> And you comment is gived at 
> "Thu, 13 Feb 2014 15:55:43 +0000"

Oh, okay - I'm sorry, I misremembered having committed it myself,
which I certainly wouldn't have done before giving the comment.
But it looks like Tim committing it raced with me sending the reply.
And had you replied rejecting the requested promise, I think I
would have considered suggesting to revert the change. Anyway -
things didn't work well back then, and in order to avoid getting into
a similar situation again I'm now being more restrictive when it
comes to committing partial solutions. But as said - I'm reconsidering
whether to take the partial solution here, but it'll take me to conduct
a few more experiments before I can respond one way or the other.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-18:
>>>> On 18.09.14 at 04:02, <yang.z.zhang@intel.com> wrote:
>> [...]
>> You didn't object my comment, and the patch was applied to Xen.
>> 
>> Later, you gave the comment:
>> "I should also say that while I certainly understand the
>> argumentation above, I would still want to go this route only with
>> the promise that B is going to be worked on reasonably soon after
>> the release, ideally with the goal of backporting the changes for 4.4.1."
> 
> No, the order of things was that the comment was given first, and then

The patch is applied at

commit 077fc1c04d70ef1748ac2daa6622b3320a1a004c
Author: Yang Zhang <yang.z.zhang@Intel.com>
Date:   Thu Feb 13 15:50:22 2014 +0000

And you comment is gived at 
"Thu, 13 Feb 2014 15:55:43 +0000"

> - you remaining silent - the patch got applied under the assumption
> that without your objection you accepted the work item.

I have explained this several months ago. I don't want to say it again.

> 
>> I really think you need to apology to all Intel developers for
>> saying such irresponsible words to Intel over and over again.
> 
> I don't think so - the lack of objection on your part meant silent
> agreement to me (and I think to Tim too). It's certainly unfortunate
> if your implication of silence is a different one than mine, but I'm
> afraid we both have to live with that. So I apologize for not taking
> possible cultural differences into account. Yet I guess you agree that
> with email being the communication medium there is _no_ way to
> _enforce_ a response by someone, and hence we have to rely on people
> responding where necessary (leaving room for interpretation otherwise).
> 
> Apart from that, looking at how slowly more involved things (XSA-59

XSA-59 involved hardware guys input and I think Don gave the update on regular weekly meeting.

> being a very prominent recent example, and the continuing lack of any
> kind of action towards properly dealing with the ATS related
> invalidation timeouts being another, and these certainly not being the

Again, Don's meeting minutes said we are having some divergences internally and it is still in discussion. You can certainly complain the slow response but it is not a 'promise' thing.

> only ones) go when Intel input is required, the issue of accepting any
> kinds of promises towards future work is clearly broader than just the one issue referred to above.
> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 18.09.14 at 04:02, <yang.z.zhang@intel.com> wrote:
>[...]
> You didn't object my comment, and the patch was applied to Xen.
> 
> Later, you gave the comment:
> "I should also say that while I certainly understand the argumentation 
> above, I would still want to go this route only with the promise that B is 
> going to be worked on reasonably soon after the release, ideally with the 
> goal of backporting the changes for 4.4.1."

No, the order of things was that the comment was given first, and then
- you remaining silent - the patch got applied under the assumption that
without your objection you accepted the work item.

> I really think you need to apology to all Intel developers for saying such 
> irresponsible words to Intel over and over again.

I don't think so - the lack of objection on your part meant silent
agreement to me (and I think to Tim too). It's certainly unfortunate
if your implication of silence is a different one than mine, but I'm
afraid we both have to live with that. So I apologize for not taking
possible cultural differences into account. Yet I guess you agree
that with email being the communication medium there is _no_ way
to _enforce_ a response by someone, and hence we have to rely
on people responding where necessary (leaving room for
interpretation otherwise).

Apart from that, looking at how slowly more involved things (XSA-59
being a very prominent recent example, and the continuing lack of
any kind of action towards properly dealing with the ATS related
invalidation timeouts being another, and these certainly not being the
only ones) go when Intel input is required, the issue of accepting any
kinds of promises towards future work is clearly broader than just the
one issue referred to above.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Zhang, Yang Z]

Jan Beulich wrote on 2014-09-17:
>>>> On 17.09.14 at 04:42, <kevin.tian@intel.com> wrote:
>>>> From: Chen, Tiejun
>>> Sent: Tuesday, September 16, 2014 6:01 PM On 2014/9/16 9:24, Chen,
>>> Tiejun wrote:
>>>> Jan,
>>>> 
>>>> What's your last judgement?
>>>> 
>>>> Could we apply these two separate patches in advance? I think
>>>> actually they aren't blocking the remains what we tried to do in
>>>> another RFC series, but now we really need them to make sure GFX
>>>> passthrough can work well.
>>>> 
>>>> For that RFC I have to take more time to cover all scenarios, so
>>>> as you saw its really a slow process I can push forward.
>>> 
>>> Any consideration?
>>> 
>> 
>> Given the facts that:
>> 
>> - earlier two patches can fix an important usage of GPU pass-through
>> for Windows VM
>> 
>> - earlier two patches don't make things worse
>> 
>> - current patch set still needs quite some effort for a robust RMRR
>> implementation
>> 
>> I'd suggest taking earlier two patches to catch 4.5 release, while
>> the big patch set is ongoing developed.
> 
> For the record: I'm intending to take another look, but as time permits.
> For now my position stands that I don't look forward to take just
> those two patches. This is based on the bad experience we had with the
> promise by Intel to work on implementing large page support for non-

Can you point out which promise Intel have gave? If you mean the VT-d large page, please look the whole story carefully that I never give any promise to re-enable VT-d large page for separate mode. All the promise is made by yourself and you think Intel accepted you promise because I didn't refuse it explicitly. 

Here is the background if someone wants to know the whole story: 

I send out the patch to fix a VT-d issue with VRAM and George lists four solutions to solve this issue:
A. Share EPT/IOMMU tables, only do log-dirty tracking on the buffer being tracked, and hope the guest doesn't DMA into video ram; DMA causes IOMMU fault. (This really shouldn't crash the host under normal circumstances; if it does it's a hardware bug.)
B. Never share EPT/IOMMU tables, and hope the guest doesn't DMA into video ram.  DMA causes missed update to dirty bitmap, which will hopefully just cause screen corruption.
C. Do buffer scanning rather than dirty vram tracking (SLOW)
D. Don't allow both a virtual video card and pass-through

I prefer A is better and my patch chose A. Here is my original comment:
" Actually, the first solution came to my mind is B. Then I realized that even chose B, we still cannot track the memory updating from DMA(even with A/D bit, it still a problem). Also, considering the current usage case of log dirty in Xen(only vram tracking has problem), I though A is better.: Hypervisor only need to track the vram change. If a malicious guest try to DMA to vram range, it only crashed himself (This should be reasonable).
Another problem with B is that current VT-d large paging supporting relies on the sharing EPT and VT-d page table. This means if we choose B, then we need to re-enable VT-d large page. This would be a huge performance impaction for Xen 4.4 on using VT-d solution."

You didn't object my comment, and the patch was applied to Xen.

Later, you gave the comment:
"I should also say that while I certainly understand the argumentation above, I would still want to go this route only with the promise that B is going to be worked on reasonably soon after the release, ideally with the goal of backporting the changes for 4.4.1."

You didn't explain why B is better than A, but want me to give a promise to choose B. I didn't do it and you think I was "implicity accepted it" because I didn't reject it explicitly. 

Even there is misunderstood between us at that time, but the later discussion has clarified that I'd like to do it if you can prove current implement is buggy or B is better. Unfortunately, you didn't prove it but now you are saying Intel doesn't keep its promise. You cannot give a strong reason to show B is better but how you can expect me to give a promise to implement B?

I really think you need to apology to all Intel developers for saying such irresponsible words to Intel over and over again.

> shared EPT/VT-d page tables in order to get a smaller scale fix
> accepted into 4.4. I'm not going to rely on promises of this kind
> again, and hence I'm willing to accept said patches only if they have
> no drawbacks (figuring this out is what I actually need some more time than to just write an email).
> 
> Jan


Best regards,
Yang

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 17.09.14 at 04:42, <kevin.tian@intel.com> wrote:
>>  From: Chen, Tiejun
>> Sent: Tuesday, September 16, 2014 6:01 PM
>> On 2014/9/16 9:24, Chen, Tiejun wrote:
>> > Jan,
>> >
>> > What's your last judgement?
>> >
>> > Could we apply these two separate patches in advance? I think actually
>> > they aren't blocking the remains what we tried to do in another RFC
>> > series, but now we really need them to make sure GFX passthrough can
>> > work well.
>> >
>> > For that RFC I have to take more time to cover all scenarios, so as you
>> > saw its really a slow process I can push forward.
>> 
>> Any consideration?
>> 
> 
> Given the facts that:
> 
> - earlier two patches can fix an important usage of GPU pass-through for
> Windows VM
> 
> - earlier two patches don't make things worse
> 
> - current patch set still needs quite some effort for a robust RMRR 
> implementation
> 
> I'd suggest taking earlier two patches to catch 4.5 release, while the
> big patch set is ongoing developed.

For the record: I'm intending to take another look, but as time permits.
For now my position stands that I don't look forward to take just those
two patches. This is based on the bad experience we had with the
promise by Intel to work on implementing large page support for non-
shared EPT/VT-d page tables in order to get a smaller scale fix
accepted into 4.4. I'm not going to rely on promises of this kind again,
and hence I'm willing to accept said patches only if they have no
drawbacks (figuring this out is what I actually need some more time
than to just write an email).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 17.09.14 at 03:01, <tiejun.chen@intel.com> wrote:
> On 2014/9/16 9:24, Chen, Tiejun wrote:
>> On 2014/9/13 5:26, Tim Deegan wrote:
>>> At 09:59 -0700 on 12 Sep (1410512391), Lars Kurth wrote:
>>>> On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:
>>>>> On 2014/9/12 15:19, Jan Beulich wrote:
>>>>>>>
>>>>>>> So I still hope we can issue such a vote on these two patches if
>>>>>>> possible.
>>>> There are several ways we can do this:
>>>> * Jan and Tim can ask the other committers for an opinion (either
>>>> publicly on the list - ideally related to this thread for
>>>> traceability - or privately depending on preference) and one may
>>>> change their opinion based on what the others say
>>>> * As there is also a potential risk element for the 4.5 release
>>>> Jan/Tim could ask the Release Manager for an opinion too and take
>>>> that into account
>>>> * Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in
>>>> other words as peers they both agree to disagree and ask Keir to act
>>>> as referee.
>>>
>>> Actually I don't think we need to go so far.  I'm happy to defer to Jan's
>>> judgement on this; we ought to sort out RMRRs properly.
>>
>> Jan,
>>
>> What's your last judgement?
>>
>> Could we apply these two separate patches in advance? I think actually
>> they aren't blocking the remains what we tried to do in another RFC
>> series, but now we really need them to make sure GFX passthrough can
>> work well.
>>
>> For that RFC I have to take more time to cover all scenarios, so as you
>> saw its really a slow process I can push forward.
> 
> Any consideration?

Please allow a little more time than just a day for a reply. This isn't
the most urgent thing I need to deal with. I'll get back to you in due
course.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Tuesday, September 16, 2014 6:01 PM
> 
> On 2014/9/16 9:24, Chen, Tiejun wrote:
> > On 2014/9/13 5:26, Tim Deegan wrote:
> >> At 09:59 -0700 on 12 Sep (1410512391), Lars Kurth wrote:
> >>> On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:
> >>>> On 2014/9/12 15:19, Jan Beulich wrote:
> >>>>>>
> >>>>>> So I still hope we can issue such a vote on these two patches if
> >>>>>> possible.
> >>> There are several ways we can do this:
> >>> * Jan and Tim can ask the other committers for an opinion (either
> >>> publicly on the list - ideally related to this thread for
> >>> traceability - or privately depending on preference) and one may
> >>> change their opinion based on what the others say
> >>> * As there is also a potential risk element for the 4.5 release
> >>> Jan/Tim could ask the Release Manager for an opinion too and take
> >>> that into account
> >>> * Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in
> >>> other words as peers they both agree to disagree and ask Keir to act
> >>> as referee.
> >>
> >> Actually I don't think we need to go so far.  I'm happy to defer to Jan's
> >> judgement on this; we ought to sort out RMRRs properly.
> >
> > Jan,
> >
> > What's your last judgement?
> >
> > Could we apply these two separate patches in advance? I think actually
> > they aren't blocking the remains what we tried to do in another RFC
> > series, but now we really need them to make sure GFX passthrough can
> > work well.
> >
> > For that RFC I have to take more time to cover all scenarios, so as you
> > saw its really a slow process I can push forward.
> 
> Any consideration?
> 

Given the facts that:

- earlier two patches can fix an important usage of GPU pass-through for
Windows VM

- earlier two patches don't make things worse

- current patch set still needs quite some effort for a robust RMRR 
implementation

I'd suggest taking earlier two patches to catch 4.5 release, while the
big patch set is ongoing developed.

Thanks
Kevin

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/16 9:24, Chen, Tiejun wrote:
> On 2014/9/13 5:26, Tim Deegan wrote:
>> At 09:59 -0700 on 12 Sep (1410512391), Lars Kurth wrote:
>>> On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:
>>>> On 2014/9/12 15:19, Jan Beulich wrote:
>>>>>>
>>>>>> So I still hope we can issue such a vote on these two patches if
>>>>>> possible.
>>> There are several ways we can do this:
>>> * Jan and Tim can ask the other committers for an opinion (either
>>> publicly on the list - ideally related to this thread for
>>> traceability - or privately depending on preference) and one may
>>> change their opinion based on what the others say
>>> * As there is also a potential risk element for the 4.5 release
>>> Jan/Tim could ask the Release Manager for an opinion too and take
>>> that into account
>>> * Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in
>>> other words as peers they both agree to disagree and ask Keir to act
>>> as referee.
>>
>> Actually I don't think we need to go so far.  I'm happy to defer to Jan's
>> judgement on this; we ought to sort out RMRRs properly.
>
> Jan,
>
> What's your last judgement?
>
> Could we apply these two separate patches in advance? I think actually
> they aren't blocking the remains what we tried to do in another RFC
> series, but now we really need them to make sure GFX passthrough can
> work well.
>
> For that RFC I have to take more time to cover all scenarios, so as you
> saw its really a slow process I can push forward.

Any consideration?

Thanks
Tiejun

>
> Thanks
> Tiejun
>
>>
>> My apologies for not following this thread closely enough; I hadn't
>> realised I was causing such a disagreement.  (In my defence I wasn't CC'd
>> until now.)
>>
>> Cheers,
>>
>> Tim.
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/13 5:26, Tim Deegan wrote:
> At 09:59 -0700 on 12 Sep (1410512391), Lars Kurth wrote:
>> On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:
>>> On 2014/9/12 15:19, Jan Beulich wrote:
>>>>>
>>>>> So I still hope we can issue such a vote on these two patches if possible.
>> There are several ways we can do this:
>> * Jan and Tim can ask the other committers for an opinion (either publicly on the list - ideally related to this thread for traceability - or privately depending on preference) and one may change their opinion based on what the others say
>> * As there is also a potential risk element for the 4.5 release Jan/Tim could ask the Release Manager for an opinion too and take that into account
>> * Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in other words as peers they both agree to disagree and ask Keir to act as referee.
>
> Actually I don't think we need to go so far.  I'm happy to defer to Jan's
> judgement on this; we ought to sort out RMRRs properly.

Jan,

What's your last judgement?

Could we apply these two separate patches in advance? I think actually 
they aren't blocking the remains what we tried to do in another RFC 
series, but now we really need them to make sure GFX passthrough can 
work well.

For that RFC I have to take more time to cover all scenarios, so as you 
saw its really a slow process I can push forward.

Thanks
Tiejun

>
> My apologies for not following this thread closely enough; I hadn't
> realised I was causing such a disagreement.  (In my defence I wasn't CC'd
> until now.)
>
> Cheers,
>
> Tim.
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tim Deegan]

At 09:59 -0700 on 12 Sep (1410512391), Lars Kurth wrote:
> On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:
> > On 2014/9/12 15:19, Jan Beulich wrote:
> >>> 
> >>> So I still hope we can issue such a vote on these two patches if possible.
> There are several ways we can do this: 
> * Jan and Tim can ask the other committers for an opinion (either publicly on the list - ideally related to this thread for traceability - or privately depending on preference) and one may change their opinion based on what the others say
> * As there is also a potential risk element for the 4.5 release Jan/Tim could ask the Release Manager for an opinion too and take that into account
> * Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in other words as peers they both agree to disagree and ask Keir to act as referee. 

Actually I don't think we need to go so far.  I'm happy to defer to Jan's
judgement on this; we ought to sort out RMRRs properly.

My apologies for not following this thread closely enough; I hadn't
realised I was causing such a disagreement.  (In my defence I wasn't CC'd
until now.)

Cheers,

Tim.

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 1482 bytes --]


On 12 Sep 2014, at 01:27, Chen, Tiejun <tiejun.chen@intel.com> wrote:

> On 2014/9/12 15:19, Jan Beulich wrote:
>>>>> 
>>> 
>>> So I still hope we can issue such a vote on these two patches if possible.
>> 
>> Lars, how would we go about this?
>> 


It don’t actually think that a vote is necessary in this case, as we are not paralysed and this is a relatively minor disagreement. What appears to be the case is that Jan and Tim have a different opinion on whether this patch can go in (on the basis that some things are missing). We split big features into smaller parts all the time, which then get committed in phases. So in the big scheme of things, this is really a minor issue. 

In any case, the vagueness of the process is intentional, because it encourages you to try and resolve issues rather than hide behind a process. 

There are several ways we can do this: 
* Jan and Tim can ask the other committers for an opinion (either publicly on the list - ideally related to this thread for traceability - or privately depending on preference) and one may change their opinion based on what the others say
* As there is also a potential risk element for the 4.5 release Jan/Tim could ask the Release Manager for an opinion too and take that into account
* Jan/Tim can ask the Project Lead (aka Keir) to make a decision : in other words as peers they both agree to disagree and ask Keir to act as referee. 

Does this make sense?

Regards
Lars




[-- Attachment #1.2: Type: text/html, Size: 2467 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/12 15:19, Jan Beulich wrote:
>>>> On 12.09.14 at 08:38, <tiejun.chen@intel.com> wrote:
>> On 2014/9/3 17:54, Jan Beulich wrote:
>>>>>> On 03.09.14 at 11:41, <tiejun.chen@intel.com> wrote:
>>>>>
>>>>>> Additionally, I want to know if patch v6 is fine to be acked.
>>>>>
>>>>> My position hasn't changed: I view this as correct but incomplete,
>>>>> and hence not ready for inclusion. With - iirc - Tim being of a
>>>>> different opinion, if you want to push for a decision without
>>>>> supplying the missing pieces, see the section "Conflict Resolution"
>>>>> on http://www.xenproject.org/governance.html. But of course
>>>>> I'd much prefer to avoid having to fall back to this, and instead
>>>>> have a complete patch set to commit within reasonable time.
>>>>
>>>> Looks I can't make my RFC patches to implement a complete RMRR
>>>> supporting better as you pointed while reviewing, so I have to go back
>>>> looking at this.
>>>
>>> Please just give me a little time to hand you a replacement patch on
>>> which you could then base the rest of your series. But please also
>>> realize that this isn't my top priority.
>>>
>>> Jan
>>>
>>>> So how can I issue such a private vote on these two patches? I don't see
>>>> How-to in the section "Conflict Resolution" on
>>>> http://www.xenproject.org/governance.html.
>>>>
>>
>> Jan,
>>
>> I have to admit its really hard to push this RMRR RFC with my
>> experiences and skills. Currently every new revision doesn't minimize
>> all previous comments. Instead, it always brings more comments or
>> arguments. So I don't think myself can do this very well in Xen side.
>>
>> So I still hope we can issue such a vote on these two patches if possible.
>
> Lars, how would we go about this?
>
>> Once I have enough capability to cover this I can go back or other guys
>> may do really better than me.
>
> I already helped you with the first patch. Did you ask the VT-d

Yes, this already costs more our time as you said, and this is also one 
reason why I'm saying myself can't finish this series completely.

Recently looks we have to re-concern this to cover more scenarios, 
hotplug, migration and so forth. I don't think I already have a clear 
design to address everything.

Originally that series is just as RFC. And I believe you already don't 
hope I post too much more incomplete codes to you again.

> maintainers for help with the rest?
>

Ditto.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 12.09.14 at 08:38, <tiejun.chen@intel.com> wrote:
> On 2014/9/3 17:54, Jan Beulich wrote:
>>>>> On 03.09.14 at 11:41, <tiejun.chen@intel.com> wrote:
>>>>
>>>>> Additionally, I want to know if patch v6 is fine to be acked.
>>>>
>>>> My position hasn't changed: I view this as correct but incomplete,
>>>> and hence not ready for inclusion. With - iirc - Tim being of a
>>>> different opinion, if you want to push for a decision without
>>>> supplying the missing pieces, see the section "Conflict Resolution"
>>>> on http://www.xenproject.org/governance.html. But of course
>>>> I'd much prefer to avoid having to fall back to this, and instead
>>>> have a complete patch set to commit within reasonable time.
>>>
>>> Looks I can't make my RFC patches to implement a complete RMRR
>>> supporting better as you pointed while reviewing, so I have to go back
>>> looking at this.
>>
>> Please just give me a little time to hand you a replacement patch on
>> which you could then base the rest of your series. But please also
>> realize that this isn't my top priority.
>>
>> Jan
>>
>>> So how can I issue such a private vote on these two patches? I don't see
>>> How-to in the section "Conflict Resolution" on
>>> http://www.xenproject.org/governance.html.
>>>
> 
> Jan,
> 
> I have to admit its really hard to push this RMRR RFC with my 
> experiences and skills. Currently every new revision doesn't minimize 
> all previous comments. Instead, it always brings more comments or 
> arguments. So I don't think myself can do this very well in Xen side.
> 
> So I still hope we can issue such a vote on these two patches if possible.

Lars, how would we go about this?

> Once I have enough capability to cover this I can go back or other guys 
> may do really better than me.

I already helped you with the first patch. Did you ask the VT-d
maintainers for help with the rest?

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/9/3 17:54, Jan Beulich wrote:
>>>> On 03.09.14 at 11:41, <tiejun.chen@intel.com> wrote:
>>>
>>>> Additionally, I want to know if patch v6 is fine to be acked.
>>>
>>> My position hasn't changed: I view this as correct but incomplete,
>>> and hence not ready for inclusion. With - iirc - Tim being of a
>>> different opinion, if you want to push for a decision without
>>> supplying the missing pieces, see the section "Conflict Resolution"
>>> on http://www.xenproject.org/governance.html. But of course
>>> I'd much prefer to avoid having to fall back to this, and instead
>>> have a complete patch set to commit within reasonable time.
>>
>> Looks I can't make my RFC patches to implement a complete RMRR
>> supporting better as you pointed while reviewing, so I have to go back
>> looking at this.
>
> Please just give me a little time to hand you a replacement patch on
> which you could then base the rest of your series. But please also
> realize that this isn't my top priority.
>
> Jan
>
>> So how can I issue such a private vote on these two patches? I don't see
>> How-to in the section "Conflict Resolution" on
>> http://www.xenproject.org/governance.html.
>>

Jan,

I have to admit its really hard to push this RMRR RFC with my 
experiences and skills. Currently every new revision doesn't minimize 
all previous comments. Instead, it always brings more comments or 
arguments. So I don't think myself can do this very well in Xen side.

So I still hope we can issue such a vote on these two patches if possible.

Once I have enough capability to cover this I can go back or other guys 
may do really better than me.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 03.09.14 at 11:41, <tiejun.chen@intel.com> wrote:
>> 
>>> Additionally, I want to know if patch v6 is fine to be acked.
>>
>> My position hasn't changed: I view this as correct but incomplete,
>> and hence not ready for inclusion. With - iirc - Tim being of a
>> different opinion, if you want to push for a decision without
>> supplying the missing pieces, see the section "Conflict Resolution"
>> on http://www.xenproject.org/governance.html. But of course
>> I'd much prefer to avoid having to fall back to this, and instead
>> have a complete patch set to commit within reasonable time.
> 
> Looks I can't make my RFC patches to implement a complete RMRR 
> supporting better as you pointed while reviewing, so I have to go back 
> looking at this.

Please just give me a little time to hand you a replacement patch on
which you could then base the rest of your series. But please also
realize that this isn't my top priority.

Jan

> So how can I issue such a private vote on these two patches? I don't see 
> How-to in the section "Conflict Resolution" on 
> http://www.xenproject.org/governance.html.
> 
> Thanks
> Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

>
>> Additionally, I want to know if patch v6 is fine to be acked.
>
> My position hasn't changed: I view this as correct but incomplete,
> and hence not ready for inclusion. With - iirc - Tim being of a
> different opinion, if you want to push for a decision without
> supplying the missing pieces, see the section "Conflict Resolution"
> on http://www.xenproject.org/governance.html. But of course
> I'd much prefer to avoid having to fall back to this, and instead
> have a complete patch set to commit within reasonable time.
>
> Jan

Looks I can't make my RFC patches to implement a complete RMRR 
supporting better as you pointed while reviewing, so I have to go back 
looking at this.

So how can I issue such a private vote on these two patches? I don't see 
How-to in the section "Conflict Resolution" on 
http://www.xenproject.org/governance.html.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/8/4 15:31, Jan Beulich wrote:
>>>> On 03.08.14 at 10:04, <tiejun.chen@intel.com> wrote:
>> On 2014/8/1 21:47, Jan Beulich wrote:
>>> I'm not sure - this may be an additional piece to be done for
>>> consistency (if the domain builder doesn't already call this), but
>>> since hvmloader doesn't appear to call XENMEM_memory_map it
>>> won't do on its own I'm afraid.
>>>
>>
>> Yes, current hvmloader can't do this on its own.
>>
>> But in PV case, e820_host, seems be a refereed way to our goal. Even we
>> may reuse some codes here so its a convenient approach.
>
> Only for those PV guests that actually care about e820_host. Non-
> pvops Linux, for example, doesn't, but also has no problem with
> the RMRR ranges overlapping with RAM due to the fully separated
> M and P address spaces. The only issue here would be the risk of
> assigning MMIO overlapping with them.

I will send a preliminary patches to cover this as RFC, please take a 
look at if this is good.

Thanks
Tiejun

>
>> Additionally, I want to know if patch v6 is fine to be acked.
>
> My position hasn't changed: I view this as correct but incomplete,
> and hence not ready for inclusion. With - iirc - Tim being of a
> different opinion, if you want to push for a decision without
> supplying the missing pieces, see the section "Conflict Resolution"
> on http://www.xenproject.org/governance.html. But of course
> I'd much prefer to avoid having to fall back to this, and instead
> have a complete patch set to commit within reasonable time.
>
> Jan
>
>
>

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 03.08.14 at 10:04, <tiejun.chen@intel.com> wrote:
> On 2014/8/1 21:47, Jan Beulich wrote:
>> I'm not sure - this may be an additional piece to be done for
>> consistency (if the domain builder doesn't already call this), but
>> since hvmloader doesn't appear to call XENMEM_memory_map it
>> won't do on its own I'm afraid.
>>
> 
> Yes, current hvmloader can't do this on its own.
> 
> But in PV case, e820_host, seems be a refereed way to our goal. Even we 
> may reuse some codes here so its a convenient approach.

Only for those PV guests that actually care about e820_host. Non-
pvops Linux, for example, doesn't, but also has no problem with
the RMRR ranges overlapping with RAM due to the fully separated
M and P address spaces. The only issue here would be the risk of
assigning MMIO overlapping with them.

> Additionally, I want to know if patch v6 is fine to be acked.

My position hasn't changed: I view this as correct but incomplete,
and hence not ready for inclusion. With - iirc - Tim being of a
different opinion, if you want to push for a decision without
supplying the missing pieces, see the section "Conflict Resolution"
on http://www.xenproject.org/governance.html. But of course
I'd much prefer to avoid having to fall back to this, and instead
have a complete patch set to commit within reasonable time.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 02.08.14 at 01:22, <kevin.tian@intel.com> wrote:
> I think we need expose RMRR to libxc so it can avoid allocating memory pages
> conflicting to RMRR ranges, and then to hvmloader so it can avoid allocating
> MMIO BAR conflicting to RMRR ranges and may further reserve those ranges
> from guest e820 table, regardless of whether there is a device being 
> assigned.

Right.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/8/1 21:47, Jan Beulich wrote:
>>>> On 01.08.14 at 11:50, <tiejun.chen@intel.com> wrote:
>> On 2014/8/1 15:21, Jan Beulich wrote:
>>>>>> On 01.08.14 at 09:10, <tiejun.chen@intel.com> wrote:
>>>> On 2014/8/1 14:51, Jan Beulich wrote:
>>>>>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
>>>>>> Additionally, I'm trying to figure out that solution. As I mentioned
>>>>>> previously, I think we can reserve all RMRR once when a guest call
>>>>>> XENMEM_machine_memory_map to create its own memory. What about this
>>>>>> idea? Or other better suggestions?
>>>>>
>>>>> I don't think any HVM guest would ever call this, even more so that
>>>>> the call is restricted to Dom0. The reservation needs to be done
>>>>
>>>> Thanks for your correction. Actually I'm also afraid I may not find a
>>>> correct place where I want to go indeed since I'm not familiar this
>>>> process.
>>>>
>>>>> when guest memory gets populated (and its E820 constructed).
>>>>
>>>> Could you hint me where this action is covered?
>>>
>>> Memory population happens in tools/libxc/xc_hvm_build.c:setup_guest(),
>>> the E820 for the guest gets constructed in hvmloader (just grep for
>>> [eE]820).
>>>
>>
>> Thanks for your information.
>>
>> With further looking into this, instead of XENMEM_machine_memory_map, I
>> think we can go XENMEM_set_memory_map path, right?
>
> I'm not sure - this may be an additional piece to be done for
> consistency (if the domain builder doesn't already call this), but
> since hvmloader doesn't appear to call XENMEM_memory_map it
> won't do on its own I'm afraid.
>

Yes, current hvmloader can't do this on its own.

But in PV case, e820_host, seems be a refereed way to our goal. Even we 
may reuse some codes here so its a convenient approach.

Additionally, I want to know if patch v6 is fine to be acked.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, August 01, 2014 6:47 AM
> 
> >>> On 01.08.14 at 11:50, <tiejun.chen@intel.com> wrote:
> > On 2014/8/1 15:21, Jan Beulich wrote:
> >>>>> On 01.08.14 at 09:10, <tiejun.chen@intel.com> wrote:
> >>> On 2014/8/1 14:51, Jan Beulich wrote:
> >>>>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
> >>>>> Additionally, I'm trying to figure out that solution. As I mentioned
> >>>>> previously, I think we can reserve all RMRR once when a guest call
> >>>>> XENMEM_machine_memory_map to create its own memory. What
> about this
> >>>>> idea? Or other better suggestions?
> >>>>
> >>>> I don't think any HVM guest would ever call this, even more so that
> >>>> the call is restricted to Dom0. The reservation needs to be done
> >>>
> >>> Thanks for your correction. Actually I'm also afraid I may not find a
> >>> correct place where I want to go indeed since I'm not familiar this
> >>> process.
> >>>
> >>>> when guest memory gets populated (and its E820 constructed).
> >>>
> >>> Could you hint me where this action is covered?
> >>
> >> Memory population happens in tools/libxc/xc_hvm_build.c:setup_guest(),
> >> the E820 for the guest gets constructed in hvmloader (just grep for
> >> [eE]820).
> >>
> >
> > Thanks for your information.
> >
> > With further looking into this, instead of XENMEM_machine_memory_map,
> I
> > think we can go XENMEM_set_memory_map path, right?
> 
> I'm not sure - this may be an additional piece to be done for
> consistency (if the domain builder doesn't already call this), but
> since hvmloader doesn't appear to call XENMEM_memory_map it
> won't do on its own I'm afraid.
> 

I think we need expose RMRR to libxc so it can avoid allocating memory pages
conflicting to RMRR ranges, and then to hvmloader so it can avoid allocating
MMIO BAR conflicting to RMRR ranges and may further reserve those ranges
from guest e820 table, regardless of whether there is a device being assigned.

Thanks
Kevin

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 01.08.14 at 11:50, <tiejun.chen@intel.com> wrote:
> On 2014/8/1 15:21, Jan Beulich wrote:
>>>>> On 01.08.14 at 09:10, <tiejun.chen@intel.com> wrote:
>>> On 2014/8/1 14:51, Jan Beulich wrote:
>>>>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
>>>>> Additionally, I'm trying to figure out that solution. As I mentioned
>>>>> previously, I think we can reserve all RMRR once when a guest call
>>>>> XENMEM_machine_memory_map to create its own memory. What about this
>>>>> idea? Or other better suggestions?
>>>>
>>>> I don't think any HVM guest would ever call this, even more so that
>>>> the call is restricted to Dom0. The reservation needs to be done
>>>
>>> Thanks for your correction. Actually I'm also afraid I may not find a
>>> correct place where I want to go indeed since I'm not familiar this
>>> process.
>>>
>>>> when guest memory gets populated (and its E820 constructed).
>>>
>>> Could you hint me where this action is covered?
>>
>> Memory population happens in tools/libxc/xc_hvm_build.c:setup_guest(),
>> the E820 for the guest gets constructed in hvmloader (just grep for
>> [eE]820).
>>
> 
> Thanks for your information.
> 
> With further looking into this, instead of XENMEM_machine_memory_map, I 
> think we can go XENMEM_set_memory_map path, right?

I'm not sure - this may be an additional piece to be done for
consistency (if the domain builder doesn't already call this), but
since hvmloader doesn't appear to call XENMEM_memory_map it
won't do on its own I'm afraid.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/8/1 15:21, Jan Beulich wrote:
>>>> On 01.08.14 at 09:10, <tiejun.chen@intel.com> wrote:
>> On 2014/8/1 14:51, Jan Beulich wrote:
>>>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
>>>> Additionally, I'm trying to figure out that solution. As I mentioned
>>>> previously, I think we can reserve all RMRR once when a guest call
>>>> XENMEM_machine_memory_map to create its own memory. What about this
>>>> idea? Or other better suggestions?
>>>
>>> I don't think any HVM guest would ever call this, even more so that
>>> the call is restricted to Dom0. The reservation needs to be done
>>
>> Thanks for your correction. Actually I'm also afraid I may not find a
>> correct place where I want to go indeed since I'm not familiar this
>> process.
>>
>>> when guest memory gets populated (and its E820 constructed).
>>
>> Could you hint me where this action is covered?
>
> Memory population happens in tools/libxc/xc_hvm_build.c:setup_guest(),
> the E820 for the guest gets constructed in hvmloader (just grep for
> [eE]820).
>

Thanks for your information.

With further looking into this, instead of XENMEM_machine_memory_map, I 
think we can go XENMEM_set_memory_map path, right?

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 01.08.14 at 09:10, <tiejun.chen@intel.com> wrote:
> On 2014/8/1 14:51, Jan Beulich wrote:
>>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
>>> Additionally, I'm trying to figure out that solution. As I mentioned
>>> previously, I think we can reserve all RMRR once when a guest call
>>> XENMEM_machine_memory_map to create its own memory. What about this
>>> idea? Or other better suggestions?
>>
>> I don't think any HVM guest would ever call this, even more so that
>> the call is restricted to Dom0. The reservation needs to be done
> 
> Thanks for your correction. Actually I'm also afraid I may not find a 
> correct place where I want to go indeed since I'm not familiar this 
> process.
> 
>> when guest memory gets populated (and its E820 constructed).
> 
> Could you hint me where this action is covered?

Memory population happens in tools/libxc/xc_hvm_build.c:setup_guest(),
the E820 for the guest gets constructed in hvmloader (just grep for
[eE]820).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/8/1 14:51, Jan Beulich wrote:
>>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
>> Additionally, I'm trying to figure out that solution. As I mentioned
>> previously, I think we can reserve all RMRR once when a guest call
>> XENMEM_machine_memory_map to create its own memory. What about this
>> idea? Or other better suggestions?
>
> I don't think any HVM guest would ever call this, even more so that
> the call is restricted to Dom0. The reservation needs to be done

Thanks for your correction. Actually I'm also afraid I may not find a 
correct place where I want to go indeed since I'm not familiar this 
process.

> when guest memory gets populated (and its E820 constructed).

Could you hint me where this action is covered?

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 31.07.14 at 11:45, <tiejun.chen@intel.com> wrote:
> Additionally, I'm trying to figure out that solution. As I mentioned 
> previously, I think we can reserve all RMRR once when a guest call 
> XENMEM_machine_memory_map to create its own memory. What about this 
> idea? Or other better suggestions?

I don't think any HVM guest would ever call this, even more so that
the call is restricted to Dom0. The reservation needs to be done
when guest memory gets populated (and its E820 constructed).

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/8/1 6:44, Tian, Kevin wrote:
>> From: Chen, Tiejun
>> Sent: Thursday, July 31, 2014 2:46 AM
>>
>> On 2014/7/30 18:47, Jan Beulich wrote:
>>>>>> On 30.07.14 at 12:40, <tiejun.chen@intel.com> wrote:
>>>> On 2014/7/30 18:25, Jan Beulich wrote:
>>>>>>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
>>>>>>     From what those codes mean, it just return regardless whether they
>>>>>> really conflict. And this is just a good assumption, so if I'm
>>>>>> understanding this properly, actually our patches do this thing
>>>>>> precisely because we further check if this assumption is true, then take
>>>>>> necessary actions.
>>>>>
>>>>> Except that the pointed out check prevents the code you modify
>>>>> from being reached at all, i.e. as long as that check is there it
>>>>> doesn't matter (for any passed through USB device) what action
>>>>> rmrr_identity_mapping() takes.
>>>>>
>>>>
>>>> Sorry, what do you mean?
>>>>
>>>>    From my point of view these two patches should be better than drop
>>>> simply RMRR for any PT USB device no matter if its really necessary.
>>>
>>> I mean that for USB devices your patches change nothing without
>>> said check also getting removed.
>>>
>>
>> Jan,
>>
>> For USB instance, I think currently we still keep that until we have a
>> complete solution since this is always safe.
>>
>> Additionally, I'm trying to figure out that solution. As I mentioned
>> previously, I think we can reserve all RMRR once when a guest call
>> XENMEM_machine_memory_map to create its own memory. What about this
>> idea? Or other better suggestions?
>>
>
> It's not guest to create its own memory. It's control panel to create guest
> memory layout. We need reserve RMRRs there, and if doing that maybe we

I mean here we can reserve those RMRR memory in e820 as long as we 
create one VM.

> can leave user space to setup identity mapping for RMRR instead of doing
> that in Xen?
>

 From my point of view that may be complex. And I'm not sure if this can 
work out hotplug case.  And especially, the memory range covered by RMRR 
should not be big, right? So I think its acceptable to reserve them even 
guest always don't use them.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Thursday, July 31, 2014 2:46 AM
> 
> On 2014/7/30 18:47, Jan Beulich wrote:
> >>>> On 30.07.14 at 12:40, <tiejun.chen@intel.com> wrote:
> >> On 2014/7/30 18:25, Jan Beulich wrote:
> >>>>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
> >>>>    From what those codes mean, it just return regardless whether they
> >>>> really conflict. And this is just a good assumption, so if I'm
> >>>> understanding this properly, actually our patches do this thing
> >>>> precisely because we further check if this assumption is true, then take
> >>>> necessary actions.
> >>>
> >>> Except that the pointed out check prevents the code you modify
> >>> from being reached at all, i.e. as long as that check is there it
> >>> doesn't matter (for any passed through USB device) what action
> >>> rmrr_identity_mapping() takes.
> >>>
> >>
> >> Sorry, what do you mean?
> >>
> >>   From my point of view these two patches should be better than drop
> >> simply RMRR for any PT USB device no matter if its really necessary.
> >
> > I mean that for USB devices your patches change nothing without
> > said check also getting removed.
> >
> 
> Jan,
> 
> For USB instance, I think currently we still keep that until we have a
> complete solution since this is always safe.
> 
> Additionally, I'm trying to figure out that solution. As I mentioned
> previously, I think we can reserve all RMRR once when a guest call
> XENMEM_machine_memory_map to create its own memory. What about this
> idea? Or other better suggestions?
> 

It's not guest to create its own memory. It's control panel to create guest
memory layout. We need reserve RMRRs there, and if doing that maybe we
can leave user space to setup identity mapping for RMRR instead of doing
that in Xen? 

Thanks
Kevin

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/30 18:47, Jan Beulich wrote:
>>>> On 30.07.14 at 12:40, <tiejun.chen@intel.com> wrote:
>> On 2014/7/30 18:25, Jan Beulich wrote:
>>>>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
>>>>    From what those codes mean, it just return regardless whether they
>>>> really conflict. And this is just a good assumption, so if I'm
>>>> understanding this properly, actually our patches do this thing
>>>> precisely because we further check if this assumption is true, then take
>>>> necessary actions.
>>>
>>> Except that the pointed out check prevents the code you modify
>>> from being reached at all, i.e. as long as that check is there it
>>> doesn't matter (for any passed through USB device) what action
>>> rmrr_identity_mapping() takes.
>>>
>>
>> Sorry, what do you mean?
>>
>>   From my point of view these two patches should be better than drop
>> simply RMRR for any PT USB device no matter if its really necessary.
>
> I mean that for USB devices your patches change nothing without
> said check also getting removed.
>

Jan,

For USB instance, I think currently we still keep that until we have a 
complete solution since this is always safe.

Additionally, I'm trying to figure out that solution. As I mentioned 
previously, I think we can reserve all RMRR once when a guest call 
XENMEM_machine_memory_map to create its own memory. What about this 
idea? Or other better suggestions?

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.07.14 at 12:40, <tiejun.chen@intel.com> wrote:
> On 2014/7/30 18:25, Jan Beulich wrote:
>>>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
>>>   From what those codes mean, it just return regardless whether they
>>> really conflict. And this is just a good assumption, so if I'm
>>> understanding this properly, actually our patches do this thing
>>> precisely because we further check if this assumption is true, then take
>>> necessary actions.
>>
>> Except that the pointed out check prevents the code you modify
>> from being reached at all, i.e. as long as that check is there it
>> doesn't matter (for any passed through USB device) what action
>> rmrr_identity_mapping() takes.
>>
> 
> Sorry, what do you mean?
> 
>  From my point of view these two patches should be better than drop 
> simply RMRR for any PT USB device no matter if its really necessary.

I mean that for USB devices your patches change nothing without
said check also getting removed.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/30 18:25, Jan Beulich wrote:
>>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
>>   From what those codes mean, it just return regardless whether they
>> really conflict. And this is just a good assumption, so if I'm
>> understanding this properly, actually our patches do this thing
>> precisely because we further check if this assumption is true, then take
>> necessary actions.
>
> Except that the pointed out check prevents the code you modify
> from being reached at all, i.e. as long as that check is there it
> doesn't matter (for any passed through USB device) what action
> rmrr_identity_mapping() takes.
>

Sorry, what do you mean?

 From my point of view these two patches should be better than drop 
simply RMRR for any PT USB device no matter if its really necessary.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.07.14 at 11:40, <tiejun.chen@intel.com> wrote:
>  From what those codes mean, it just return regardless whether they 
> really conflict. And this is just a good assumption, so if I'm 
> understanding this properly, actually our patches do this thing 
> precisely because we further check if this assumption is true, then take 
> necessary actions.

Except that the pointed out check prevents the code you modify
from being reached at all, i.e. as long as that check is there it
doesn't matter (for any passed through USB device) what action
rmrr_identity_mapping() takes.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/30 17:23, Jan Beulich wrote:
>>>> On 30.07.14 at 10:59, <tiejun.chen@intel.com> wrote:
>> On 2014/7/30 16:36, Jan Beulich wrote:
>>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>>> --- 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_identity_p2m_entry(d, base_pfn) )
>>>> +                return -1;
>>>> +        }
>>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>>>                return -1;
>>>>            base_pfn++;
>>>>        }
>>>
>>> So I wonder how this plays together with
>>>
>>>       /* FIXME: Because USB RMRR conflicts with guest bios region,
>>>        * ignore USB RMRR temporarily.
>>>        */
>>>       seg = pdev->seg;
>>>       bus = pdev->bus;
>>>       if ( is_usb_device(seg, bus, pdev->devfn) )
>>>       {
>>>           ret = 0;
>>>           goto done;
>>>       }
>>>
>>> later in the same file (in intel_iommu_assign_device()). I.e. the
>>> improvement you claim won't be achieved for passed through USB
>>
>> I think we can remove this chunk of codes since these two patches
>> already check if they conflicts.
>
> Causing an apparent regression for anyone wanting to pass through
> a USB devices associated with an RMRR? I agree this is the more
> correct thing to do, but I'm not sure all our users would agree.
>

 From what those codes mean, it just return regardless whether they 
really conflict. And this is just a good assumption, so if I'm 
understanding this properly, actually our patches do this thing 
precisely because we further check if this assumption is true, then take 
necessary actions.

If you're fine to this point, I'd like to remove this with a separate 
patch. Or lets wait one day to look for more eyes.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.07.14 at 10:59, <tiejun.chen@intel.com> wrote:
> On 2014/7/30 16:36, Jan Beulich wrote:
>>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>>> --- 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_identity_p2m_entry(d, base_pfn) )
>>> +                return -1;
>>> +        }
>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>>               return -1;
>>>           base_pfn++;
>>>       }
>>
>> So I wonder how this plays together with
>>
>>      /* FIXME: Because USB RMRR conflicts with guest bios region,
>>       * ignore USB RMRR temporarily.
>>       */
>>      seg = pdev->seg;
>>      bus = pdev->bus;
>>      if ( is_usb_device(seg, bus, pdev->devfn) )
>>      {
>>          ret = 0;
>>          goto done;
>>      }
>>
>> later in the same file (in intel_iommu_assign_device()). I.e. the
>> improvement you claim won't be achieved for passed through USB
> 
> I think we can remove this chunk of codes since these two patches 
> already check if they conflicts.

Causing an apparent regression for anyone wanting to pass through
a USB devices associated with an RMRR? I agree this is the more
correct thing to do, but I'm not sure all our users would agree.

Jan

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Chen, Tiejun]

On 2014/7/30 16:36, Jan Beulich wrote:
>>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
>> --- 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_identity_p2m_entry(d, base_pfn) )
>> +                return -1;
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +                                       IOMMUF_readable|IOMMUF_writable) )
>>               return -1;
>>           base_pfn++;
>>       }
>
> So I wonder how this plays together with
>
>      /* FIXME: Because USB RMRR conflicts with guest bios region,
>       * ignore USB RMRR temporarily.
>       */
>      seg = pdev->seg;
>      bus = pdev->bus;
>      if ( is_usb_device(seg, bus, pdev->devfn) )
>      {
>          ret = 0;
>          goto done;
>      }
>
> later in the same file (in intel_iommu_assign_device()). I.e. the
> improvement you claim won't be achieved for passed through USB

I think we can remove this chunk of codes since these two patches 
already check if they conflicts.

> devices afaict. One more aspect supporting my view that this
> needs fully addressing rather than any such partial solution.

If we're talking about fixing all RMRR issues completely, these patches 
should not be enough. But I think these patches shouldn't block that 
complete solution because they're just checking to make sure we can 
create those RMRR mapping if possible. I mean no matter if we address 
more on RMRR, we still need these two patches to double-check/create 
RMRR mapping in shared EPT case, even we will reserve those ranges in 
guest as some guys recommended previously.

Thanks
Tiejun

Re: [v6][PATCH 2/2] xen:vtd: missing RMRR mapping while share EPT

[Jan Beulich]

>>> On 30.07.14 at 03:36, <tiejun.chen@intel.com> wrote:
> --- 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_identity_p2m_entry(d, base_pfn) )
> +                return -1;
> +        }
> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> +                                       IOMMUF_readable|IOMMUF_writable) )
>              return -1;
>          base_pfn++;
>      }

So I wonder how this plays together with

    /* FIXME: Because USB RMRR conflicts with guest bios region,
     * ignore USB RMRR temporarily.
     */
    seg = pdev->seg;
    bus = pdev->bus;
    if ( is_usb_device(seg, bus, pdev->devfn) )
    {
        ret = 0;
        goto done;
    }

later in the same file (in intel_iommu_assign_device()). I.e. the
improvement you claim won't be achieved for passed through USB
devices afaict. One more aspect supporting my view that this
needs fully addressing rather than any such partial solution.

Jan

[v6][PATCH 2/2] 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(-)

v6:

* Nothing

v5:

* Don't pass mfn directly to set_identity_p2m_entry() since it can get that.

v4:

* After introduce set_rmrr_p2m_entry we can go there directly.

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..bc50793 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_identity_p2m_entry(d, 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