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