[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH V2 1/25] DOMCTL: Introduce new DOMCTL commands for vIOMMU support
Hi Roger,
On 23/08/17 08:22, Roger Pau Monné wrote:
On Wed, Aug 23, 2017 at 02:06:17PM +0800, Lan Tianyu wrote:
Hi Roger:
Thanks for your review.
On 2017年08月22日 22:32, Roger Pau Monné wrote:
On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote:
+
+/* 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
+#define XEN_DOMCTL_query_viommu_caps 2
+ union {
+ struct {
+ /* IN - vIOMMU type */
+ uint64_t viommu_type;
+ /*
+ * IN - MMIO base address of vIOMMU. vIOMMU device models
+ * are in charge of to check base_address and length.
+ */
+ uint64_t base_address;
+ /* IN - Length of MMIO region */
+ uint64_t length;
It seems weird that you can specify the length, is that something
that a user would like to set? Isn't the length of the IOMMU MMIO
region fixed by the hardware spec?
Different vendor may have different IOMMU register region sizes. (e.g,
VTD has one page size for register region). The length field is to make
vIOMMU device model not to abuse address space. Some registers' offsets
are reported by other register and these offsets are emulated by vIOMMU
device model. If it's not necessary, we can remove it and add it when
there is real such requirement.
So from my understanding the size of the IOMMU MMIO region is implicit
in the IOMMU type that the user chooses. I don't think this field is
needed.
To me, it makes more sense to care both the base and the size rather
than only the former.
The toolstack is in charge of the address space and should be aware of
the size of everything. This address space may not be static and it
makes sense to give this information to Xen and verify we had the same
assumption.
+ /* IN - Capabilities with which we want to create */
+ uint64_t capabilities;
+ /* OUT - vIOMMU identity */
+ uint32_t viommu_id;
+ } create_viommu;
+
+ struct {
+ /* IN - vIOMMU identity */
+ uint32_t viommu_id;
+ } destroy_viommu;
+
+ struct {
+ /* IN - vIOMMU type */
+ uint64_t viommu_type;
+ /* OUT - vIOMMU Capabilities */
+ uint64_t capabilities;
+ } query_caps;
This also seems weird, shouldn't you query the capabilities of an
already created vIOMMU, rather than a vIOMMU type? Shouldn't the first
field be viommu_id?
Query interface here is to check what capabilities the vIOMMU device
model specified by viommu_type can support before create vIOMMU (suppose
user may select different capabilities). If capabilities returned by
query interface doesn't meet user configuration, tool stack should
return error. So it just accepts viommu_type.
I don't think that's needed, if the chosen capabilities are not
supported by the selected IOMMU type simply return error in
XEN_DOMCTL_create_viommu.
The capabilities of each IOMMU type should be listed in the man page,
and the user should select a supported set or else
XEN_DOMCTL_create_viommu will fail. Doing the checks both in the
toolstack and in XEN_DOMCTL_create_viommu seems pointless and prone to
errors.
What if the some capabilities depends on host IOMMU? How are you going
to report that to the user?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
