Re: [Xen-devel] [PATCH V3 1/29] Xen/doc: Add Xen virtual IOMMU doc

2017-10-24 Thread Lan Tianyu 
On 2017年10月19日 19:28, Jan Beulich wrote:
 On 19.10.17 at 10:49,  wrote:
>> On Thu, Oct 19, 2017 at 10:26:36AM +0800, Lan Tianyu wrote:
>>> Hi Roger:
>>>  Thanks for review.
>>>
>>> On 2017年10月18日 21:26, Roger Pau Monné wrote:
 On Thu, Sep 21, 2017 at 11:01:42PM -0400, Lan Tianyu wrote:
> +Xen hypervisor vIOMMU command
> +=
> +Introduce vIOMMU command "viommu=1" to enable vIOMMU function in 
>> hypervisor.
> +It's default disabled.

 Hm, I'm not sure we really need this. At the end viommu will be
 disabled by default for guests, unless explicitly enabled in the
 config file.
>>>
>>> This is according to Jan's early comments on RFC patch
>>> https://patchwork.kernel.org/patch/9733869/.
>>>
>>> "It's actually a question whether in our current scheme a Kconfig
>>> option is appropriate here in the first place. I'd rather see this be
>>> an always built feature which needs enabling on the command line
>>> for the time being."
>>
>> So if I read this correctly Jan wanted you to ditch the Kconfig option
>> and instead rely on the command line option to enable/disable it.
> 
> Yes.
> 
> Jan
> 

OK. I will remove the command in the next version. Thanks for clarification.

-- 
Best regards
Tianyu Lan

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

Re: [Xen-devel] [PATCH V3 1/29] Xen/doc: Add Xen virtual IOMMU doc

2017-10-19 Thread Jan Beulich 
>>> On 19.10.17 at 10:49,  wrote:
> On Thu, Oct 19, 2017 at 10:26:36AM +0800, Lan Tianyu wrote:
>> Hi Roger:
>>  Thanks for review.
>> 
>> On 2017年10月18日 21:26, Roger Pau Monné wrote:
>> > On Thu, Sep 21, 2017 at 11:01:42PM -0400, Lan Tianyu wrote:
>> >> +Xen hypervisor vIOMMU command
>> >> +=
>> >> +Introduce vIOMMU command "viommu=1" to enable vIOMMU function in 
> hypervisor.
>> >> +It's default disabled.
>> > 
>> > Hm, I'm not sure we really need this. At the end viommu will be
>> > disabled by default for guests, unless explicitly enabled in the
>> > config file.
>> 
>> This is according to Jan's early comments on RFC patch
>> https://patchwork.kernel.org/patch/9733869/.
>> 
>> "It's actually a question whether in our current scheme a Kconfig
>> option is appropriate here in the first place. I'd rather see this be
>> an always built feature which needs enabling on the command line
>> for the time being."
> 
> So if I read this correctly Jan wanted you to ditch the Kconfig option
> and instead rely on the command line option to enable/disable it.

Yes.

Jan

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

Re: [Xen-devel] [PATCH V3 1/29] Xen/doc: Add Xen virtual IOMMU doc

2017-10-19 Thread Roger Pau Monné 
On Thu, Oct 19, 2017 at 10:26:36AM +0800, Lan Tianyu wrote:
> Hi Roger:
>  Thanks for review.
> 
> On 2017年10月18日 21:26, Roger Pau Monné wrote:
> > On Thu, Sep 21, 2017 at 11:01:42PM -0400, Lan Tianyu wrote:
> >> +Xen hypervisor vIOMMU command
> >> +=
> >> +Introduce vIOMMU command "viommu=1" to enable vIOMMU function in 
> >> hypervisor.
> >> +It's default disabled.
> > 
> > Hm, I'm not sure we really need this. At the end viommu will be
> > disabled by default for guests, unless explicitly enabled in the
> > config file.
> 
> This is according to Jan's early comments on RFC patch
> https://patchwork.kernel.org/patch/9733869/.
> 
> "It's actually a question whether in our current scheme a Kconfig
> option is appropriate here in the first place. I'd rather see this be
> an always built feature which needs enabling on the command line
> for the time being."

So if I read this correctly Jan wanted you to ditch the Kconfig option
and instead rely on the command line option to enable/disable it.

I don't have a strong opinion here, so it's fine for me if you want to
keep both the Kconfig option and the command line one.

Roger.

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

Re: [Xen-devel] [PATCH V3 1/29] Xen/doc: Add Xen virtual IOMMU doc

2017-10-18 Thread Lan Tianyu 
Hi Roger:
 Thanks for review.

On 2017年10月18日 21:26, Roger Pau Monné wrote:
> On Thu, Sep 21, 2017 at 11:01:42PM -0400, Lan Tianyu wrote:
>> This patch is to add Xen virtual IOMMU doc to introduce motivation,
>> framework, vIOMMU hypercall and xl configuration.
>>
>> Signed-off-by: Lan Tianyu 
>> ---
>>  docs/misc/viommu.txt | 136 
>> +++
>>  1 file changed, 136 insertions(+)
>>  create mode 100644 docs/misc/viommu.txt
>>
>> diff --git a/docs/misc/viommu.txt b/docs/misc/viommu.txt
>> new file mode 100644
>> index 000..348e8c4
>> --- /dev/null
>> +++ b/docs/misc/viommu.txt
>> @@ -0,0 +1,136 @@
>> +Xen virtual IOMMU
>> +
>> +Motivation
>> +==
>> +Enable more than 128 vcpu support
>> +
>> +The current requirements of HPC cloud service requires VM with a high
>> +number of CPUs in order to achieve high performance in parallel
>> +computing.
>> +
>> +To support >128 vcpus, X2APIC mode in guest is necessary because legacy
>> +APIC(XAPIC) just supports 8-bit APIC ID. The APIC ID used by Xen is
>> +CPU ID * 2 (ie: CPU 127 has APIC ID 254, which is the last one available
>> +in xAPIC mode) and so it only can support 128 vcpus at most. x2APIC mode
>> +supports 32-bit APIC ID and it requires the interrupt remapping 
>> functionality
>> +of a vIOMMU if the guest wishes to route interrupts to all available vCPUs
>> +
>> +The reason for this is that there is no modification for existing PCI MSI
>> +and IOAPIC when introduce X2APIC.
> 
> I'm not sure the above sentence makes much sense. IMHO I would just
> remove it.

OK. Will remove.

> 
>> PCI MSI/IOAPIC can only send interrupt
>> +message containing 8-bit APIC ID, which cannot address cpus with >254
>> +APIC ID. Interrupt remapping supports 32-bit APIC ID and so it's necessary
>> +for >128 vcpus support.
>> +
>> +
>> +vIOMMU Architecture
>> +===
>> +vIOMMU device model is inside Xen hypervisor for following factors
>> +1) Avoid round trips between Qemu and Xen hypervisor
>> +2) Ease of integration with the rest of hypervisor
>> +3) HVMlite/PVH doesn't use Qemu
> 
> Just use PVH here, HVMlite == PVH now.

OK.

> 
>> +
>> +* Interrupt remapping overview.
>> +Interrupts from virtual devices and physical devices are delivered
>> +to vLAPIC from vIOAPIC and vMSI. vIOMMU needs to remap interrupt during
>> +this procedure.
>> +
>> ++---+
>> +|Qemu   |VM |
>> +|   | ++|
>> +|   | |  Device driver ||
>> +|   | ++---+|
>> +|   |  ^|
>> +|   ++  | ++---+|
>> +|   | Virtual device |  | |  IRQ subsystem ||
>> +|   +---++  | ++---+|
>> +|   |   |  ^|
>> +|   |   |  ||
>> ++---+---+
>> +|hypervisor |  | VIRQ   |
>> +|   |+-++   |
>> +|   ||  vLAPIC  |   |
>> +|   |VIRQ+-++   |
>> +|   |  ^|
>> +|   |  ||
>> +|   |+-++   |
>> +|   ||  vIOMMU  |   |
>> +|   |+-++   |
>> +|   |  ^|
>> +|   |  ||
>> +|   |+-++   |
>> +|   ||   vIOAPIC/vMSI   |   |
>> +|   |++++   |
>> +|   | ^^|
>> +|   +-+||
>> +|  ||
>> ++---+
>> +HW |IRQ
>> ++---+
>> +|   PCI Device  |
>> ++---+
>> +
>> +
>> +vIOMMU hypercall
>> +
>> +Introduce a new domctl hypercall "xen_domctl_viommu_op" to create/destroy
>> +vIOMMUs.
>> +
>> +* vIOMMU hypercall parameter structure
>> +
>> +/* vIOMMU type - specify vendor vIOMMU device model */
>> +#define VIOMMU_TYPE_INTEL_VTD  0
>> +
>> +/* vIOMMU capabilities */
>> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
>> +
>> +struct xen_domctl_viommu_op {
>> +uint32_t cmd;
>> +#define XEN_DOMCTL_create_viommu  0
>> +#define XEN_DOMCTL_destroy_viommu 1
> 
> I would invert the order of the domctl names:
> 
> #define

Re: [Xen-devel] [PATCH V3 1/29] Xen/doc: Add Xen virtual IOMMU doc

2017-10-18 Thread Roger Pau Monné 
On Thu, Sep 21, 2017 at 11:01:42PM -0400, Lan Tianyu wrote:
> This patch is to add Xen virtual IOMMU doc to introduce motivation,
> framework, vIOMMU hypercall and xl configuration.
> 
> Signed-off-by: Lan Tianyu 
> ---
>  docs/misc/viommu.txt | 136 
> +++
>  1 file changed, 136 insertions(+)
>  create mode 100644 docs/misc/viommu.txt
> 
> diff --git a/docs/misc/viommu.txt b/docs/misc/viommu.txt
> new file mode 100644
> index 000..348e8c4
> --- /dev/null
> +++ b/docs/misc/viommu.txt
> @@ -0,0 +1,136 @@
> +Xen virtual IOMMU
> +
> +Motivation
> +==
> +Enable more than 128 vcpu support
> +
> +The current requirements of HPC cloud service requires VM with a high
> +number of CPUs in order to achieve high performance in parallel
> +computing.
> +
> +To support >128 vcpus, X2APIC mode in guest is necessary because legacy
> +APIC(XAPIC) just supports 8-bit APIC ID. The APIC ID used by Xen is
> +CPU ID * 2 (ie: CPU 127 has APIC ID 254, which is the last one available
> +in xAPIC mode) and so it only can support 128 vcpus at most. x2APIC mode
> +supports 32-bit APIC ID and it requires the interrupt remapping functionality
> +of a vIOMMU if the guest wishes to route interrupts to all available vCPUs
> +
> +The reason for this is that there is no modification for existing PCI MSI
> +and IOAPIC when introduce X2APIC.

I'm not sure the above sentence makes much sense. IMHO I would just
remove it.

> PCI MSI/IOAPIC can only send interrupt
> +message containing 8-bit APIC ID, which cannot address cpus with >254
> +APIC ID. Interrupt remapping supports 32-bit APIC ID and so it's necessary
> +for >128 vcpus support.
> +
> +
> +vIOMMU Architecture
> +===
> +vIOMMU device model is inside Xen hypervisor for following factors
> +1) Avoid round trips between Qemu and Xen hypervisor
> +2) Ease of integration with the rest of hypervisor
> +3) HVMlite/PVH doesn't use Qemu

Just use PVH here, HVMlite == PVH now.

> +
> +* Interrupt remapping overview.
> +Interrupts from virtual devices and physical devices are delivered
> +to vLAPIC from vIOAPIC and vMSI. vIOMMU needs to remap interrupt during
> +this procedure.
> +
> ++---+
> +|Qemu   |VM |
> +|   | ++|
> +|   | |  Device driver ||
> +|   | ++---+|
> +|   |  ^|
> +|   ++  | ++---+|
> +|   | Virtual device |  | |  IRQ subsystem ||
> +|   +---++  | ++---+|
> +|   |   |  ^|
> +|   |   |  ||
> ++---+---+
> +|hypervisor |  | VIRQ   |
> +|   |+-++   |
> +|   ||  vLAPIC  |   |
> +|   |VIRQ+-++   |
> +|   |  ^|
> +|   |  ||
> +|   |+-++   |
> +|   ||  vIOMMU  |   |
> +|   |+-++   |
> +|   |  ^|
> +|   |  ||
> +|   |+-++   |
> +|   ||   vIOAPIC/vMSI   |   |
> +|   |++++   |
> +|   | ^^|
> +|   +-+||
> +|  ||
> ++---+
> +HW |IRQ
> ++---+
> +|   PCI Device  |
> ++---+
> +
> +
> +vIOMMU hypercall
> +
> +Introduce a new domctl hypercall "xen_domctl_viommu_op" to create/destroy
> +vIOMMUs.
> +
> +* vIOMMU hypercall parameter structure
> +
> +/* vIOMMU type - specify vendor vIOMMU device model */
> +#define VIOMMU_TYPE_INTEL_VTD   0
> +
> +/* vIOMMU capabilities */
> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
> +
> +struct xen_domctl_viommu_op {
> +uint32_t cmd;
> +#define XEN_DOMCTL_create_viommu  0
> +#define XEN_DOMCTL_destroy_viommu 1

I would invert the order of the domctl names:

#define XEN_DOMCTL_viommu_create  0
#define XEN_DOMCTL_viommu_destroy 1

It's clearer if the operation is the last part of the name.

> +union {
> +struct {
> +/* IN - vIOMMU type  */
> +uint64_t